net/nanocoap: Build message with coap_pkt_t by kb2ma · Pull Request #9085 · RIOT-OS/RIOT

kb2ma · 2018-05-05T12:36:41Z

Contribution description

Adds an API to nanocoap to use a coap_pkt_t to build a message, including options. Builds on existing buffer-based functions. Use of the coap_pkt_t struct makes it easier to manage options.

Includes:

coap_pkt_init()
coap_opt_add_string()
coap_opt_add_uint()
coap_opt_finish()

This approach also allows integration by gcoap, bringing the APIs closer together.

Issues/PRs references

Implements ideas described in #9048 and #8831. Provides an API useful to #8920.

kb2ma · 2018-05-05T13:25:23Z

I plan to add a couple more tests, including for the Content-Format uint option. I think that then I can remove the WIP label.

The coap_opt_get_string() function can be added to #8920. We can create a PR like #8920 for uint options. We also can create a follow-on PR for sorting options in coap_opt_finish() that were added provisionally. Certainly the gcoap adaptation will need this.

kb2ma · 2018-05-10T05:46:15Z

Squashed WIP commits; ready for review. The unit tests provide decent coverage. To give the work some live testing, I created a CLI app based on the gcoap example. See my riot-nano-cli repo.

As I pulled this together, a couple of issues arose in my mind:

Do we need to create some kind of #ifdef to distinguish use of pkt-based functions to build a message vs. the traditional buffer based functions? There will be a transitional period until the buffer-based functions, like coap_put_option_block1() can be replaced.
We should take advantage of the information in the packet struct to update/replace coap_put_option(). The change is to inform the function of the maximum buffer length available, and return -ENOSPC if not sufficient. The option header length can be more than 1, so the caller really can't predict if there is enough space. I've added asserts for now.

kb2ma · 2018-05-12T11:59:15Z

@kaspar030, I assume that the basic approach here is acceptable. I am starting on a PR based on this one to adapt gcoap to it. This will allow us to remove the gcoap-specific #ifdefs added in #8772 and to reverse the stack size increase in #9000.

kb2ma · 2018-05-16T02:51:45Z

Squashed. No one has commented, and this simplifies a follow-on PR to sort options that I'm creating.

bergzand · 2018-05-25T13:23:53Z

sys/include/net/nanocoap.h

+ * @post pkt.payload advanced to first byte after option(s)
+ * @post pkt.payload_len reduced by option(s) length
+ *
+ * @param[inout]  pkt         pkt referencing target buffer


This should be @param[in,out] according to doxygen

Sure thing.

bergzand · 2018-05-25T13:24:20Z

sys/include/net/nanocoap.h

 /** @} */

+/**
+ * @name coap_opt_finish() flag parameter values


A small description of the purpose of these flags would be helpful here.

bergzand · 2018-05-25T13:25:14Z

sys/include/net/nanocoap.h

+ * @post pkt.payload advanced to first byte after option
+ * @post pkt.payload_len reduced by option length
+ *
+ * @param[inout]  pkt         pkt referencing target buffer


Same as above

bergzand · 2018-05-25T13:39:31Z

sys/include/net/nanocoap.h

+ *
+ * @return        number of bytes written to buffer
+ */
+ssize_t coap_opt_add_string(coap_pkt_t *pkt, uint16_t optnum, const char *string, char separator);


Is it possible for this function to return error codes? If not, can we change the return type to size_t instead of ssize_t?

This is an interesting API design question. These coap_opt_add...() functions rely on _add_opt_pkt(), which in turn relies on coap_put_option(). IMO the latter can be improved to return -ENOSPC because the user can't always determine in advance if the option will be too long for the available buffer space. See the questions I pose in a comment above.

So, I'm sort of anticipating that we will update these functions eventually to return an error code in that case. Does it make sense to update the API now for these new functions so the user does not need to update their code in the future? Besides, use of a signed value will never really be a problem in this case. Why box in the API if you don't need to?

OTOH, maybe this whole idea of overrunning the buffer when writing options is so unlikely that the present use of an assert after the option is added is the best/simplest solution.

ssize_t sounds good to me. I agree with you reason that these function might start returning errors. This error could not only be caused by a buffer overflow with the pdu buffer space, but also with the option array being completely filled.

I tend to favor a "soft" error return code here over an assertion. Mostly because it is possible for external CoAP requests to influence these options, and it would be bad if an assertion could be remotely triggered with some specific request (causing a packet of death vulnerability).

I think it would be best to add an @return negative on error to the doxygen both here and with the other "coap_opt_add" functions to indicate the need for error checking.

Please see the exact wording and let me know if any concerns. Also I updated the implementation of coap_opt_add_string() to return -ENOSPC. The input string may contain multiple separators, and it might be difficult for the caller to determine this in advance.

Mostly because it is possible for external CoAP requests to influence these options

Can you be more specific? I'm not sure that I follow you.

bergzand · 2018-05-25T13:39:54Z

sys/include/net/nanocoap.h

+ *
+ * @return        number of bytes written to buffer
+ */
+ssize_t coap_opt_add_uint(coap_pkt_t *pkt, uint16_t optnum, uint32_t value);


Same with the return type as for coap_opt_add_string

bergzand · 2018-05-25T13:40:37Z

sys/include/net/nanocoap.h

+ * @post pkt.payload advanced to first available byte after options
+ * @post pkt.payload_len is maximum bytes available for payload
+ *
+ * @param[inout]  pkt         pkt to update


bergzand · 2018-05-25T13:40:55Z

sys/include/net/nanocoap.h

+ *
+ * @return        total number of bytes written to buffer
+ */
+ssize_t coap_opt_finish(coap_pkt_t *pkt, uint16_t flags);


How many flags are we expecting here, would uint8_t suffice?

Might we need more than eight flags? Another interesting API question. We already have used two (add-payload, and sort-options in another PR) I rounded up here figuring it didn't really matter in terms of space, and it allows for future flexibility. Conceivably we also could use the high-order byte if some future flag requires an associated value.

Ok. uint16_t sounds good then. Could you please reflect this in the flag defines, so 0x0001 instead of 0x1 to have the data size a bit more explicit there.

bergzand · 2018-05-25T13:47:56Z

sys/net/application_layer/nanocoap/nanocoap.c

+    uint16_t lastonum = (pkt->options_len)
+            ? pkt->options[pkt->options_len - 1].opt_num : 0;
+    assert(optnum >= lastonum);
+


I think you should check here whether there is still an empty position in the options array, to prevent array-out-of-bounds bugs

Excellent idea!

bergzand · 2018-05-25T14:09:23Z

sys/include/net/nanocoap.h

+ * @param[in]    len        length of buf
+ * @param[in]    header_len length of header in buf, including token
+ */
+void coap_pkt_init(coap_pkt_t *pkt, uint8_t *buf, size_t len, size_t header_len);


I think an explicit @pre to indicate that there should be an initialized coap_hdr_t in the buffer would be nice. Somebody is going to miss the note with the buf param there :)

I agree completely.

kb2ma · 2018-05-27T11:54:52Z

Thanks for the review, @bergzand. Pushed fixups.

emmanuelsearch · 2018-06-05T13:35:45Z

@bergzand @kb2ma are there a lot of to dos left here?

kb2ma · 2018-06-05T14:29:26Z

I believe I've addressed @bergzand's comments. It would be nice to at least have a high-level "ok with the approach" confirmation from @kaspar030 since this change is a significant addition to nanocoap. It is a new API to build a message instead of the historical, purely buffer-based approach.

bergzand · 2018-06-08T13:15:29Z

@kaspar030 On a samr21-xpro, this patch adds 26 bytes of flash usage to the nanocoap-server example, that is still unmodified without any usage of these functions.
(measured with cosy)

kaspar030 · 2018-06-08T14:10:11Z

@kb2ma The approach looks good to me -> high-level ACK.

kb2ma · 2018-06-08T14:16:30Z

@bergzand, thanks for measuring. I plan to review and report. I haven't tried cosy yet.

@kaspar030, thanks for confirmation.

bergzand · 2018-06-08T14:18:09Z

@kb2ma I'd like to test this a bit more and then ACK it. Feel free to squash in the mean time.

Includes string and uint options.

kb2ma · 2018-06-10T14:33:09Z

Rebased and squashed.

Relative to master, I see the difference in size of the text sections as 52 bytes. I attribute this to

realization of _encode_uint(), which is called from coap_opt_add_uint() as well as the historical coap_put_option_block()
the longer branch name

In the proposed module doc for nanocoap, I include macros to exclude the code (COAP_WRITE_OPTIONS_STRUCT), which clears up this problem. If this is important to do now, I can add to this PR, or we can put it in a follow-on PR.

bergzand

Ack

bergzand · 2018-06-11T08:00:58Z

If this is important to do now, I can add to this PR, or we can put it in a follow-on PR.

Let's do this in a follow up.

kb2ma added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: new feature The issue requests / The PR implemements a new feature for RIOT labels May 5, 2018

kb2ma requested review from bergzand, haukepetersen and kaspar030 May 5, 2018 12:36

kb2ma mentioned this pull request May 6, 2018

nanocoap_sock: split generic request function of from nanocoap_get #9086

Merged

kb2ma force-pushed the nanocoap/build_request_pkt branch from 68f3916 to 3aba9b3 Compare May 10, 2018 05:28

kb2ma removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label May 10, 2018

kb2ma changed the title ~~[WIP] net/nanocoap: Build message with coap_pkt_t~~ net/nanocoap: Build message with coap_pkt_t May 10, 2018

kb2ma force-pushed the nanocoap/build_request_pkt branch from 634a7b1 to a13c0ec Compare May 16, 2018 02:49

This was referenced May 16, 2018

net/nanocoap: sort Options #9144

Closed

net/gcoap: Use nanocoap pkt/options API #9156

Merged

bergzand added the Area: network Area: Networking label May 19, 2018

kb2ma mentioned this pull request May 20, 2018

net/nanocoap: add generic handling for string-based options #8920

Merged

bergzand requested changes May 25, 2018

View reviewed changes

bergzand added the Area: CoAP Area: Constrained Application Protocol implementations label May 25, 2018

bergzand reviewed May 25, 2018

View reviewed changes

kb2ma mentioned this pull request Jun 8, 2018

CoAP struct-based Options API #9309

Closed

net/nanocoap: create pkt-based request

43db271

Includes string and uint options.

net/nanocoap: unit tests for pkt-based request

aa0d02e

kb2ma force-pushed the nanocoap/build_request_pkt branch from ca3da41 to aa0d02e Compare June 10, 2018 13:52

kb2ma added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 10, 2018

bergzand approved these changes Jun 11, 2018

View reviewed changes

bergzand merged commit 1c46829 into RIOT-OS:master Jun 11, 2018

kb2ma mentioned this pull request Jul 2, 2018

net/nanocoap: add nanocoap_opt2 module #9474

Closed

cladmi added this to the Release 2018.07 milestone Jul 10, 2018

kb2ma mentioned this pull request Aug 30, 2018

nanocoap: add server-side block2 support #8932

Merged

kb2ma mentioned this pull request Oct 18, 2018

[RFC] CoAP API updates for options #9048

Closed

kb2ma mentioned this pull request Jan 18, 2019

net/nanocoap: validate option length #10823

Merged

kb2ma deleted the nanocoap/build_request_pkt branch February 4, 2019 17:28

kb2ma mentioned this pull request Nov 28, 2019

net/gcoap: deprecate gcoap_finish() #12838

Merged

Conversation

kb2ma commented May 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Issues/PRs references

Uh oh!

kb2ma commented May 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kb2ma commented May 10, 2018

Uh oh!

kb2ma commented May 12, 2018

Uh oh!

kb2ma commented May 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kb2ma May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kb2ma commented May 27, 2018

Uh oh!

emmanuelsearch commented Jun 5, 2018

Uh oh!

kb2ma commented Jun 5, 2018

Uh oh!

bergzand commented Jun 8, 2018

Uh oh!

kaspar030 commented Jun 8, 2018

Uh oh!

kb2ma commented Jun 8, 2018

Uh oh!

bergzand commented Jun 8, 2018

Uh oh!

kb2ma commented Jun 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bergzand left a comment

Choose a reason for hiding this comment

Uh oh!

bergzand commented Jun 11, 2018

Uh oh!

Reviewers

Assignees

Labels

kb2ma commented May 5, 2018 •

edited

Loading

kb2ma commented May 5, 2018 •

edited

Loading

kb2ma May 28, 2018 •

edited

Loading

kb2ma commented Jun 10, 2018 •

edited

Loading