nanocoap: added function for finding options by haukepetersen · Pull Request #18 · kaspar030/sock

haukepetersen · 2017-07-26T17:17:57Z

After receiving a packet, I needed to be able to read out certain options (COAP_OPT_LOCATION_PATH in my case), and to copy their content into some user defined buffers. This was not able with the current interface, so I added a function that allows for finding certain options.

The usage is quite easy (I hope):

coap_opt_t *opt;

// for finding the first occurrence of a certain option:
uint8_t *bufpos = coap_find_opt(pdu, pdu->options, &opt, COAP_OPT_LOCATION_PATH);

// for finding the next occurrence of the same option:
while (bufpos) {
    bufpos = coap_find_opt(pdu, bufpos, &opt, 0);
    ...
}

With this function one can read ANY option and do with its values whatever they like...

haukepetersen · 2017-08-07T08:26:51Z

@kaspar030: ping

kaspar030

minor comments...

kaspar030 · 2017-08-15T11:57:41Z

nanocoap/nanocoap.c

    return coap_build_reply(pkt, COAP_CODE_205, buf, len, payload_len);
 }
+
+size_t get_lenval(uint8_t *buf, uint16_t *val)


make static, add underscore, maybe rename to "_decode_optlen" to be in line with the other coap decode functions?

yes, forgot the static. Will do both.

kaspar030 · 2017-08-15T11:58:05Z

nanocoap/nanocoap.c

+    return len;
+}
+
+int parse_opt(uint8_t *optpos, coap_opt_t *opt)


static, too?

kaspar030 · 2017-08-15T12:01:39Z

nanocoap/nanocoap.c

+
+    uint16_t delta = 0;
+
+    do {


Can we add packet length checks here? Seems like a bogus header leads to crashes here.

kaspar030 · 2017-08-15T12:04:09Z

nanocoap/nanocoap.h

 size_t coap_put_option_ct(uint8_t *buf, uint16_t lastonum, uint16_t content_type);
 size_t coap_put_option_url(uint8_t *buf, uint16_t lastonum, const char *url);

+uint8_t *coap_find_opt(coap_pkt_t *pkt, uint8_t *bufpos, coap_opt_t *opt, uint16_t optnum);


the other public functions carry "option" written out, so maybe rename?

bergzand · 2017-08-15T22:40:44Z

I'm trying to use your coap_find_opt function for #21 and I'm running into some issues with the arguments where I want to find a previously inserted option to readjust the value of the option, but a coap_pkt_t variable is not yet created for the packet.

For the context, I'm implementing block2 support (rfc7959). Before constructing the packet payload I insert a block2 header. This header consists of a block number (known), a size exponent (known) and a "more" bit flag. This more bit flag is set if there are more blocks following the current block. I currently set this flag if the payload of the reply exceeds the current block. This flag thus have to be adjusted after the payload is added and I would like to use the coap_find_opt function to retrieve the header from the headers.

So at that moment a coap_pkt_t struct of the reply is not yet available (it is created in the coap_build_reply. Would this not be more flexible if the coap_pkt_t argument would be an uint8_t* to the payload, seeing that it is only used for the payload position. This way the function can also be used to retrieve previously set options in the construction of a packet.

haukepetersen · 2017-08-16T11:38:26Z

@kaspar030 fixed your remarks, better now?

haukepetersen · 2017-08-16T11:40:10Z

@bergzand yes, that makes sense ineed: see last commit

kaspar030 · 2017-08-16T21:38:01Z

@kaspar030 fixed your remarks, better now?

yes! (I guess we leave the size validity check for later?)

haukepetersen · 2017-08-17T07:28:10Z

I guess we leave the size validity check for later?

why? It should be fixed now, is it not?

haukepetersen · 2017-08-17T07:32:49Z

I thought the check in line 428

        if (bufpos >= payload_pos) {
            return NULL;
        }

is doing that?!

kaspar030 · 2017-08-17T08:09:21Z

nanocoap/nanocoap.c

+        len = 1;
+    }
+    else if (*val == 14) {
+        memcpy(val, buf, 2);


this might read past packet length

kaspar030 · 2017-08-17T08:10:12Z

nanocoap/nanocoap.c

+        return -1;
+    }
+
+    opt->val += _decode_optlen(opt->val, &opt->delta);


through _decode_optlen(), this might read past the packet

kaspar030 · 2017-08-17T08:15:38Z

For _decode_value() I added the packet end as parameter in order to be able to check if the decoding would read past end.

haukepetersen · 2017-08-17T14:28:00Z

Ah, I see...

emmanuelsearch · 2017-09-08T09:55:14Z

@haukepetersen @kaspar030 would love to see this in ;)
(and it would bring us closer to blockwise support, if I understand correctly ?)

haukepetersen · 2018-04-12T07:07:51Z

closing in favor of #8920 and #8772

kb2ma mentioned this pull request Jul 30, 2017

net/gcoap: add Uri-Query strings for requests RIOT-OS/RIOT#7402

Merged

bergzand mentioned this pull request Aug 1, 2017

nanocoap: Add blockwise transfer block2 support #21

Closed

kaspar030 requested changes Aug 15, 2017

View reviewed changes

haukepetersen added 2 commits August 16, 2017 13:37

nanocoap: added function for finding options

a6def23

fixup! nanocoap: fixed things

ca04f1e

haukepetersen force-pushed the add_nanocoap_findopt branch from 0ca72be to ca04f1e Compare August 16, 2017 11:37

fixup! pass payload position directly to find_option()

45918ad

kaspar030 reviewed Aug 17, 2017

View reviewed changes

nanocoap/nanocoap.c

len = 1;

}

else if (*val == 14) {

memcpy(val, buf, 2);

Copy link
Copy Markdown

Owner

kaspar030 Aug 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might read past packet length

kaspar030 reviewed Aug 17, 2017

View reviewed changes

haukepetersen closed this Apr 12, 2018

Conversation

haukepetersen commented Jul 26, 2017

Uh oh!

haukepetersen commented Aug 7, 2017

Uh oh!

kaspar030 left a comment

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!

bergzand commented Aug 15, 2017

Uh oh!

haukepetersen commented Aug 16, 2017

Uh oh!

haukepetersen commented Aug 16, 2017

Uh oh!

kaspar030 commented Aug 16, 2017

Uh oh!

haukepetersen commented Aug 17, 2017

Uh oh!

haukepetersen commented Aug 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Aug 17, 2017

Uh oh!

haukepetersen commented Aug 17, 2017

Uh oh!

emmanuelsearch commented Sep 8, 2017

Uh oh!

haukepetersen commented Apr 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants