nanocoap: defuse footgun in coap_get_uri_query() #20195
Conversation
Free up the name for a function that will do something actually useful
github-actions
bot
added
Area: network
benpicco
added
Type: cleanup
| * @returns nr of bytes written to @p target (including '\0') | ||
| */ | ||
| static inline ssize_t coap_get_uri_query(coap_pkt_t *pkt, uint8_t *target) | ||
| static inline ssize_t coap_get_uri_query_string(coap_pkt_t *pkt, char *target, |
There was a problem hiding this comment.
Changing this to char * is also a huge improvement. Since callers have to change the function call anyway, I agree that this quality of life improvement can also be added just as well.
There was a problem hiding this comment.
I don't really agree with this being a footgun (it's C; you pass a mutable pointer, you better know how large a buffer the callee expexte there, or you pass it as uint_t target[CONFIG_NANOCOAP_URIMAX], which would be compatible), but I have no objections to making that more explicit either.
| @@ -847,18 +847,18 @@ static inline ssize_t coap_get_uri_path(coap_pkt_t *pkt, uint8_t *target) | |||
| * This function decodes the pkt's URI_QUERY option into a "&"-separated and | |||
| * '\0'-terminated string. | |||
| * | |||
| * Caller must ensure @p target can hold at least CONFIG_NANOCOAP_URI_MAX bytes! | |||
| * | |||
| * @param[in] pkt pkt to work on | |||
There was a problem hiding this comment.
| * @param[in] pkt pkt to work on | |
| * @note This function is mainly intended to get easy debug output. | |
| * It is needlessly complex and error prone for parsing parameters out of the query, | |
| * which is better done by @ref coap_opt_get_opaque with increasing values of @p opt_num, | |
| * until that returns `-ENOENT`. | |
| * @param[in] pkt pkt to work on |
Part of the footgun (IMO the bigger footgun even) is that this function drives people toward assembling and later parsing this string, which inevitably leads to trouble once the first & appears in a query parameter (and if that were fixed, the string would go through percent encoding and then back again).
There was a problem hiding this comment.
Scratch that -- we'll still need such a note, but #20197 adds something that would be pointed to instead.
Contribution description
coap_get_uri_query()takes no size parameter and assumes the buffer supplied by the caller will be sufficiently large.This is a huge foot-gun and we can actually supply the max length to the underlying
coap_opt_get_string()function, so just do that.Also that function is not very useful. It's nice for debugging, but when actually wanting to obtain the query parameters, we certainly don't want to first convert them into a string only to then parse the string. Rename the function to
coap_get_uri_query_string()to better reflect what it does and to make the name available for a more useful query iterator function.Testing procedure
The
tests-nanocoaptest intests/unittestsstill works.Issues/PRs references
includes #20195 because I don't like merge conflicts.