gcoap: fix underflow when correcting ETag from cache by miri64 · Pull Request #19968 · RIOT-OS/RIOT

miri64 · 2023-10-10T12:55:44Z

Contribution description

Fixes an underflow error when getting a new ETag from cache, by returning early.

Testing procedure

I repeated the testing procedures from #17801 (comment) (attention, the expected input for examples/gcoap changed slightly in the last year and a half)

diff --git a/aiocoap/cli/fileserver.py b/aiocoap/cli/fileserver.py
index 6af2b84..f5a9e71 100644
--- a/aiocoap/cli/fileserver.py
+++ b/aiocoap/cli/fileserver.py
@@ -154,6 +154,7 @@ class FileServer(Resource, aiocoap.interfaces.ObservableResource):
             elif S_ISREG(st.st_mode):
                 response = await self.render_get_file(request, path)
 
+        response.opt.max_age = 10
         response.opt.etag = etag
         return response

)

$ sudo ./dist/tool/tapsetup/tapsetup
$ PORT=tap1 USEMODULE="gcoap_forward_proxy nanocoap_cache" make -C examples/gcoap -j flash term
...
> ifconfig
ifconfig
Iface  6  HWaddr: DA:27:1D:A8:64:24 
          L2-PDU:1500  MTU:1500  HL:64  Source address length: 6
          Link type: wired
          inet6 addr: fe80::d827:1dff:fea8:6424  scope: link  VAL
          inet6 group: ff02::1
          inet6 group: ff02::1:ffa8:6424

> 
[second terminal in aiocoap repo]
$ ip addr show dev tapbr0 scope link
5: tapbr0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000
    link/ether de:1a:a8:09:45:b3 brd ff:ff:ff:ff:ff:ff
    inet6 fe80::dc1a:a8ff:fe09:45b3/64 scope link proto kernel_ll 
       valid_lft forever preferred_lft forever
$ mkdir -p test
$ echo "foobar" > test/a.txt
$ ./aiocoap-fileserver --bind "[$(ip addr show dev tapbr0 scope link | grep -o 'inet6 [0-9a-f:]\+' | sed 's/inet6 //')%tapbr0]" test/
...
[third terminal]
$ make -C examples/gcoap term
All up, running the shell now
> coap proxy set [fe80::d827:1dff:fea8:6424]:5683
coap proxy set [fe80::d827:1dff:fea8:6424]:5683
> coap get [fe80::dc1a:a8ff:fe09:45b3]:5683 /a.txt
coap get [fe80::dc1a:a8ff:fe09:45b3]:5683 /a.txt
gcoap_cli: sending msg ID 17604, 63 bytes
> --- blockwise start ---
gcoap: response Success, code 2.05, 7 bytes
foobar

--- blockwise complete ---
coap get [fe80::dc1a:a8ff:fe09:45b3]:5683 /a.txt
coap get [fe80::dc1a:a8ff:fe09:45b3]:5683 /a.txt
gcoap_cli: sending msg ID 17605, 63 bytes
--- blockwise start ---
gcoap: response Success, code 2.05, 7 bytes
foobar

--- blockwise complete ---

[wait >10 sec]

> coap get [fe80::dc1a:a8ff:fe09:45b3]:5683 /a.txt
coap get [fe80::dc1a:a8ff:fe09:45b3]:5683 /a.txt
gcoap_cli: sending msg ID 17606, 63 bytes
> --- blockwise start ---
gcoap: response Success, code 2.05, 7 bytes
foobar

--- blockwise complete ---
coap get [fe80::dc1a:a8ff:fe09:45b3]:5683 /a.txt
coap get [fe80::dc1a:a8ff:fe09:45b3]:5683 /a.txt
gcoap_cli: sending msg ID 17607, 63 bytes
--- blockwise start ---
gcoap: response Success, code 2.05, 7 bytes
foobar

--- blockwise complete ---
> ^C
native: exiting

Sniffing on tapbr0 should show something like.

Issues/PRs references

Reported off-band.

riot-ci · 2023-10-10T13:10:20Z

Murdock results

✔️ PASSED

8d1cb1b gcoap: fix underflow when correcting ETag from cache

Success	Failures	Total	Runtime
7937	0	7937	14m:31s

Artifacts

Documentation preview

chrysn

I find it weird that we allow responses with invalid ETag values into our caches in the first place, or that opt_get_opaque can even return values outside the buffer, but that'd probably be out of scope for the bug fix here.

Looks good to me.

Note for other reviewers: The example given in the report is not showing errant behavior, it merely demonstrates that things work fine before and after when given valid inputs.

chrysn · 2023-10-13T15:16:29Z

sys/net/application_layer/gcoap/gcoap.c

+                    DEBUG("gcoap: invalid calculated padding length (%lu) for ETag injection "
+                          "during cache lookup.\n", (long unsigned)rem_len);
+                    /* something fishy happened in the request. Better don't return cache entry */
+                    *cache_hit = false;


That's already false from an earlier check, but it doesn't hurt to be explicit either (the compiler will remove it anyway).

chrysn · 2023-10-13T15:25:09Z

bors merge

19968: gcoap: fix underflow when correcting ETag from cache r=chrysn a=miri64 Co-authored-by: Martine Lenders <[email protected]>

bors · 2023-10-13T15:49:22Z

Build failed:

static-tests

miri64 · 2023-10-13T16:55:21Z

Build failed:
* static-tests

Seems completely unrelated. :-(

miri64 · 2023-10-17T08:25:46Z

bors merge

19968: gcoap: fix underflow when correcting ETag from cache r=miri64 a=miri64 Co-authored-by: Martine Lenders <[email protected]>

bors · 2023-10-17T12:50:42Z

Build failed:

Murdock

miri64 · 2023-10-17T14:20:24Z

bors merge

bors · 2023-10-17T18:41:40Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

19987: gcoap: fix underflow when correcting ETag from cache [backport 2023.10] r=miri64 a=MrKevinWeiss # Backport of #19968 Co-authored-by: Martine Lenders <[email protected]>

19984: dist/testbed-support: Add openmote board [backport 2023.10] r=MrKevinWeiss a=miri64 # Backport of #19979 ### Contribution description As part of fixing the automated release specs test we will need support for another `cc2538` based board. I was able to get the tests passing with adaption here and to the release specs. ### Testing procedure ### Issues/PRs references 19987: gcoap: fix underflow when correcting ETag from cache [backport 2023.10] r=miri64 a=MrKevinWeiss # Backport of #19968 Co-authored-by: MrKevinWeiss <[email protected]> Co-authored-by: Martine Lenders <[email protected]>

gcoap: fix underflow when correcting ETag from cache

8d1cb1b

miri64 requested a review from MrKevinWeiss October 10, 2023 12:55

miri64 requested review from PeterKietzmann, cgundogan and haukepetersen as code owners October 10, 2023 12:55

github-actions bot added the Area: sys Area: System label Oct 10, 2023

benpicco requested a review from chrysn October 10, 2023 13:52

chrysn approved these changes Oct 13, 2023

View reviewed changes

bors bot added a commit that referenced this pull request Oct 13, 2023

Merge #19968

eb2940f

19968: gcoap: fix underflow when correcting ETag from cache r=chrysn a=miri64 Co-authored-by: Martine Lenders <[email protected]>

MrKevinWeiss added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Oct 16, 2023

bors bot added a commit that referenced this pull request Oct 17, 2023

Merge #19968

58821e4

19968: gcoap: fix underflow when correcting ETag from cache r=miri64 a=miri64 Co-authored-by: Martine Lenders <[email protected]>

bors bot merged commit b6772c8 into RIOT-OS:master Oct 17, 2023

miri64 deleted the gcoap/fix/underflow branch October 18, 2023 06:47

This was referenced Oct 18, 2023

gcoap: fix underflow when correcting ETag from cache [backport 2023.01] #19982

Closed

gcoap: fix underflow when correcting ETag from cache [backport 2023.10] #19987

Merged

bors bot added a commit that referenced this pull request Oct 18, 2023

Merge #19987

bab7208

19987: gcoap: fix underflow when correcting ETag from cache [backport 2023.10] r=miri64 a=MrKevinWeiss # Backport of #19968 Co-authored-by: Martine Lenders <[email protected]>

MrKevinWeiss added this to the Release 2024.01 milestone Feb 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gcoap: fix underflow when correcting ETag from cache#19968

gcoap: fix underflow when correcting ETag from cache#19968
bors[bot] merged 1 commit intoRIOT-OS:masterfrom
miri64:gcoap/fix/underflow

miri64 commented Oct 10, 2023

Uh oh!

riot-ci commented Oct 10, 2023

Uh oh!

chrysn left a comment

Uh oh!

chrysn Oct 13, 2023

Uh oh!

chrysn commented Oct 13, 2023

Uh oh!

bors bot commented Oct 13, 2023

Uh oh!

miri64 commented Oct 13, 2023

Uh oh!

miri64 commented Oct 17, 2023

Uh oh!

bors bot commented Oct 17, 2023

Uh oh!

miri64 commented Oct 17, 2023

Uh oh!

bors bot commented Oct 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

miri64 commented Oct 10, 2023

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

riot-ci commented Oct 10, 2023

Murdock results

Artifacts

Uh oh!

chrysn left a comment

Choose a reason for hiding this comment

Uh oh!

chrysn Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

chrysn commented Oct 13, 2023

Uh oh!

bors bot commented Oct 13, 2023

Uh oh!

miri64 commented Oct 13, 2023

Uh oh!

miri64 commented Oct 17, 2023

Uh oh!

bors bot commented Oct 17, 2023

Uh oh!

miri64 commented Oct 17, 2023

Uh oh!

bors bot commented Oct 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants