obgm/libcoap

Clarify required buffer size for coap_uri_into_options() and coap_split_uri()

anyc opened this issue · 10 comments

anyc commented

coap_uri_into_options() and coap_split_path() both require a user-provided buffer to operate. In the examples I found, the buffer seems to have an arbitrary size. In the description of coap_uri_into_options() I found: "buf needs to be as big as the path or query lengths."

From the latter I assume that the buffer needs to be at least as long as the path and query string. Is this right? And for coap_split_path() the length of the complete URI that is passed to it?

Thank you!

Edit: Sorry, I meant coap_split_path not coap_split_uri

The buffer size only needs to be the maximum of the size of the path and the size pf the query, not the sum of their lengths.

I will get the documentation updated.

anyc commented

Thank you. Isn't this something libcoap could do itself? I see myself just doing a strdup() and free() around these functions.

Good question. For coap_split_path() and coap_split_query() the application needs to deal with the buffer, so here buffer cannot be temporarily allocated within the functions.

Where possible, the Public API should not be changed. So, coap_uri_into_options() parameters cannot be changed. Otherwise, there is no reason as to why buffer needs to be defined as it can temporarily be allocated / deallocated internally around the calls to coap_split_path() and coap_split_query(). A suggestion is to create coap_uri_into_options2() which does not require a predefined buffer.

The buffer size only needs to be the maximum of the size of the path and the size pf the query,
not the sum of their lengths.

Further testing indicates this may not be the case. For every segment, taking the path variant as an example, the / is replaced with a dummy option of type 0 and length when copied across to the temporary buffer. If length is > 12 bytes, then the dummy option is more than 1 byte long - hence the output buffer needs to be bigger than the input buffer.

So, arbitrary size is true. The scratch buffer needs to be at least the length of the input buffer with more (one or two bytes) depending on the length of the individual segments. Need to think this through as to how to best to do it for user applications.

anyc commented

Thanks for looking into this. How about a coap_path_into_options function? Couldn't we change coap_uri_into_options so it will allocate the buffer itself if the buf argument is NULL?

Sorry about slowness in response. I am thinking of doing it as coap_path_into_optlist() (and coap_query_into_optlist()) which actually does not require any scratch buffer allocation (and better describes what it is doing). I have found a path definition of /%2E/ is not being treated as /./ in coap_split_path() which I am also looking at.

Likewise, I am thinking of coap_uri_into_options() which no longer needs a scratch buffer. coap_uri_into_options() would then ignore any buf / buflen arguments.

@anyc It would be good if you are able to checkout #1448 which removes the need to define any scratch buffers when any of the 3 new options (coap_uri_into_optlist(), coap_path_into_optlist() and coap_query_into_optlist()) are used which replace the old equivalent functions (coap_uri_into_options(), coap_split_path() and coap_split_query()).

anyc commented

Sorry, I was a bit busy. It works here, thank you!

In my case, one additional function would be helpful, I think. To enable observe I use the following code:

coap_insert_optlist(&optlist,
   coap_new_optlist(COAP_OPTION_OBSERVE,
      coap_encode_var_safe(scratch, sizeof(scratch), COAP_OBSERVE_ESTABLISH)));

While the required buffer size is likely easy to identify here, something like coap_var_into_optlist() would make my code easier to grasp. What do you think?

By the way, I am testing this with my ctypes-based Python wrapper libcoapy around libcoap. It is a bit easier for me to handle than aiocoap and also supports unix domain sockets thanks to libcoap. Maybe it is helpful for someone else too.

coap_encode_var_safe() ideally should have a buffer that is sizeof(unsigned int) and coap_encode_var_safe8() should have a buffer that is sizeof(size_t).

coap_var_into_optlist(size_t, coap_option_num_t, coap_optlist_t **) could be helpful. It needs to be coded, documented and used in the appropriate places in the libcoap code and examples.

anyc commented

Ok, I will try to add this but probably not in the next days. Thank you!