Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sys/net/gnrc/pkt: fix gnrc_pkt_delete() [backport 2022.10] #18880

Merged

Conversation

maribu
Copy link
Member

@maribu maribu commented Nov 11, 2022

Backport of #18874

Contribution description

The previous implementation used creative construct for impedance mismatching between the core list API (which returns a ptr to the removed element if found) and the GNRC pkt list API (which returns a ptr to the new list head) that creates a temporary list head on the stack.

I'm not entirely sure if the previous implementation is containing undefined behavior that is used against us with GCC >= 12.x, or if this is a compiler bug. In either case, not reusing the core list API here and just having a textbook linked list delete function here is not much less readable and fixes the issue for our users.

Testing procedure

$ make BOARD=nucleo-f767zi -C tests/unittests tests-pkt flash test

This PR

READY
s
START
.......................
OK (23 tests)

In master

READY
s
START
...............
pkt_tests.test_pkt_delete (tests/unittests/tests-pkt/tests-pkt.c 195) (&pkt) == res->next
........
run 23 failures 1

Issues/PRs references

None

The previous implementation used creative construct for impedance
mismatching between the core list API (which returns a ptr to the
removed element if found) and the GNRC pkt list API (which returns a
ptr to the new list head) that creates a temporary list head on the
stack.

I'm not entirely sure if the previous implementation is containing
undefined behavior that is used against us with GCC >= 12.x, or if this
is a compiler bug. In either case, not reusing the core list API here
and just having a textbook linked list delete function here is not much
less readable and fixes the issue for our users.

(cherry picked from commit 0d5bde0)
@maribu maribu added Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Process: release backport Integration Process: The PR is a release backport of a change previously provided to master Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Nov 11, 2022
@maribu maribu removed the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Nov 11, 2022
@riot-ci
Copy link

riot-ci commented Nov 11, 2022

Murdock results

FAILED

30caf76 sys/net/gnrc/pkt: fix gnrc_pkt_delete()

Test failures (1)
Application Target Toolchain Runtime (s) Worker
tests/pkg_edhoc_c samr21-xpro gnu 30.24 pi-d5d933f9

Artifacts

This only reflects a subset of all builds from https://ci-prod.riot-os.org. Please refer to https://ci.riot-os.org for a complete build for now.

@maribu
Copy link
Member Author

maribu commented Nov 11, 2022

The test success locally. I'd just blame the worker / board - likely it is the one that regularly failed in the nightlies before.

Full log output
~/Repos/software/RIOT/tests/pkg_edhoc_c $ make BOARD=samr21-xpro flash test
Building application "tests_pkg_edhoc_c" for "samr21-xpro" with MCU "samd21".

make[1]: Nothing to be done for 'prepare'.
"make" -C /home/maribu/Repos/software/RIOT/pkg/c25519/ 
"make" -C /home/maribu/Repos/software/RIOT/build/pkg/c25519/src -f /home/maribu/Repos/software/RIOT/Makefile.base MODULE=c25519
"make" -C /home/maribu/Repos/software/RIOT/pkg/edhoc-c/ 
"make" -C /home/maribu/Repos/software/RIOT/build/pkg/EDHOC-C/src/cbor -f /home/maribu/Repos/software/RIOT/Makefile.base MODULE=edhoc-c_cbor_nanocbor SRC=nanocbor.c
"make" -C /home/maribu/Repos/software/RIOT/build/pkg/EDHOC-C/src/crypto -f /home/maribu/Repos/software/RIOT/Makefile.base MODULE=edhoc-c_crypto_tinycrypt SRC=tinycrypt.c
"make" -C /home/maribu/Repos/software/RIOT/build/pkg/EDHOC-C/src -f /home/maribu/Repos/software/RIOT/Makefile.base MODULE=edhoc-c
"make" -C /home/maribu/Repos/software/RIOT/pkg/nanocbor/ 
"make" -C /home/maribu/Repos/software/RIOT/build/pkg/nanocbor/src -f /home/maribu/Repos/software/RIOT/Makefile.base MODULE=nanocbor
"make" -C /home/maribu/Repos/software/RIOT/pkg/tinycrypt/ 
"make" -C /home/maribu/Repos/software/RIOT/build/pkg/tinycrypt/lib/source/ -f /home/maribu/Repos/software/RIOT/pkg/tinycrypt/Makefile.tinycrypt -f /home/maribu/Repos/software/RIOT/Makefile.base MODULE=tinycrypt
"make" -C /home/maribu/Repos/software/RIOT/boards/common/init
"make" -C /home/maribu/Repos/software/RIOT/boards/samr21-xpro
"make" -C /home/maribu/Repos/software/RIOT/core
"make" -C /home/maribu/Repos/software/RIOT/core/lib
"make" -C /home/maribu/Repos/software/RIOT/cpu/samd21
"make" -C /home/maribu/Repos/software/RIOT/cpu/cortexm_common
"make" -C /home/maribu/Repos/software/RIOT/cpu/cortexm_common/periph
"make" -C /home/maribu/Repos/software/RIOT/cpu/sam0_common
"make" -C /home/maribu/Repos/software/RIOT/cpu/sam0_common/periph
"make" -C /home/maribu/Repos/software/RIOT/cpu/samd21/periph
"make" -C /home/maribu/Repos/software/RIOT/cpu/samd21/vectors
"make" -C /home/maribu/Repos/software/RIOT/drivers
"make" -C /home/maribu/Repos/software/RIOT/drivers/at86rf2xx
"make" -C /home/maribu/Repos/software/RIOT/drivers/edbg_eui
"make" -C /home/maribu/Repos/software/RIOT/drivers/netdev
"make" -C /home/maribu/Repos/software/RIOT/drivers/periph_common
"make" -C /home/maribu/Repos/software/RIOT/sys
"make" -C /home/maribu/Repos/software/RIOT/sys/auto_init
"make" -C /home/maribu/Repos/software/RIOT/sys/div
"make" -C /home/maribu/Repos/software/RIOT/sys/event
"make" -C /home/maribu/Repos/software/RIOT/sys/evtimer
"make" -C /home/maribu/Repos/software/RIOT/sys/fmt
"make" -C /home/maribu/Repos/software/RIOT/sys/frac
"make" -C /home/maribu/Repos/software/RIOT/sys/iolist
"make" -C /home/maribu/Repos/software/RIOT/sys/isrpipe
"make" -C /home/maribu/Repos/software/RIOT/sys/libc
"make" -C /home/maribu/Repos/software/RIOT/sys/luid
"make" -C /home/maribu/Repos/software/RIOT/sys/malloc_thread_safe
"make" -C /home/maribu/Repos/software/RIOT/sys/net/application_layer/nanocoap
"make" -C /home/maribu/Repos/software/RIOT/sys/net/crosslayer/inet_csum
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc/netapi
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc/netif
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc/netif/hdr
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc/netif/ieee802154
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc/netif/init_devs
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc/netreg
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc/network_layer/icmpv6
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc/network_layer/icmpv6/echo
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc/network_layer/ipv6
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc/network_layer/ipv6/hdr
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc/network_layer/ipv6/nib
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc/network_layer/ndp
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc/network_layer/sixlowpan
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc/network_layer/sixlowpan/ctx
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc/network_layer/sixlowpan/frag
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc/network_layer/sixlowpan/frag/fb
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc/network_layer/sixlowpan/frag/rb
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc/network_layer/sixlowpan/iphc
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc/network_layer/sixlowpan/nd
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc/pkt
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc/pktbuf
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc/pktbuf_static
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc/sock
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc/sock/udp
"make" -C /home/maribu/Repos/software/RIOT/sys/net/gnrc/transport_layer/udp
"make" -C /home/maribu/Repos/software/RIOT/sys/net/link_layer/eui_provider
"make" -C /home/maribu/Repos/software/RIOT/sys/net/link_layer/ieee802154
"make" -C /home/maribu/Repos/software/RIOT/sys/net/link_layer/l2util
"make" -C /home/maribu/Repos/software/RIOT/sys/net/netif
"make" -C /home/maribu/Repos/software/RIOT/sys/net/netutils
"make" -C /home/maribu/Repos/software/RIOT/sys/net/network_layer/icmpv6
"make" -C /home/maribu/Repos/software/RIOT/sys/net/network_layer/ipv6/addr
"make" -C /home/maribu/Repos/software/RIOT/sys/net/network_layer/ipv6/hdr
"make" -C /home/maribu/Repos/software/RIOT/sys/net/network_layer/sixlowpan
"make" -C /home/maribu/Repos/software/RIOT/sys/net/sock
"make" -C /home/maribu/Repos/software/RIOT/sys/net/transport_layer/udp
"make" -C /home/maribu/Repos/software/RIOT/sys/newlib_syscalls_default
"make" -C /home/maribu/Repos/software/RIOT/sys/pm_layered
"make" -C /home/maribu/Repos/software/RIOT/sys/posix/inet
"make" -C /home/maribu/Repos/software/RIOT/sys/ps
"make" -C /home/maribu/Repos/software/RIOT/sys/random
"make" -C /home/maribu/Repos/software/RIOT/sys/shell
"make" -C /home/maribu/Repos/software/RIOT/sys/shell/cmds
"make" -C /home/maribu/Repos/software/RIOT/sys/stdio_uart
"make" -C /home/maribu/Repos/software/RIOT/sys/test_utils/interactive_sync
"make" -C /home/maribu/Repos/software/RIOT/sys/test_utils/print_stack_usage
"make" -C /home/maribu/Repos/software/RIOT/sys/tsrb
"make" -C /home/maribu/Repos/software/RIOT/sys/ztimer
   text	  data	   bss	   dec	   hex	filename
 108880	   188	 28248	137316	 21864	/home/maribu/Repos/software/RIOT/tests/pkg_edhoc_c/bin/samr21-xpro/tests_pkg_edhoc_c.elf
[INFO] edbg binary not found - building it from source now
CC= CFLAGS= make -C /home/maribu/Repos/software/RIOT/dist/tools/edbg
[INFO] edbg binary successfully built!
/home/maribu/Repos/software/RIOT/dist/tools/edbg/edbg.sh flash /home/maribu/Repos/software/RIOT/tests/pkg_edhoc_c/bin/samr21-xpro/tests_pkg_edhoc_c.bin
### Flashing Target ###
Debugger: ATMEL EDBG CMSIS-DAP ATML2127031800009225 01.1A.00FB (S)
Clock frequency: 16.0 MHz
Target: SAM R21G18 (Rev C)
Verification...
at address 0x4 expected 0xc9, read 0xcd
Error: verification failed
Debugger: ATMEL EDBG CMSIS-DAP ATML2127031800009225 01.1A.00FB (S)
Clock frequency: 16.0 MHz
Target: SAM R21G18 (Rev C)
Programming................. done.
Verification................. done.
Done flashing


/home/maribu/Repos/software/RIOT/dist/tools/pyterm/pyterm -p "/dev/ttyACM1" -b "115200" --no-reconnect --noprefix --no-repeat-command-on-empty-line 
Twisted not available, please install it if you want to use pyterm's JSON capabilities
Connect to serial port /dev/ttyACM1
/home/maribu/Repos/software/RIOT/dist/tools/pyterm/pyterm:289: DeprecationWarning: setDaemon() is deprecated, set the daemon attribute instead
  receiver_thread.setDaemon(1)
Welcome to pyterm!
Type '/exit' to exit.

> 
> ifconfig
ifconfig
Iface  5  HWaddr: 42:29  Channel: 26  Page: 0  NID: 0x23  PHY: O-QPSK 
          
          Long HWaddr: 00:04:25:19:18:01:C2:29 
           TX-Power: 0dBm  State: IDLE  max. Retrans.: 3  CSMA Retries: 4 
          AUTOACK  ACK_REQ  CSMA  L2-PDU:102  MTU:1280  HL:64  RTR  
          6LO  IPHC  
          Source address length: 8
          Link type: wireless
          inet6 addr: fe80::204:2519:1801:c229  scope: link  VAL
init handshake fe80::204:2519:1801:c229%5 5683
          inet6 group: ff02::2
          inet6 group: ff02::1
          inet6 group: ff02::1:ff01:c229
          
> init handshake fe80::204:2519:1801:c229%5 5683
[initiator]: sending msg1 (37 bytes):
0x0d 0x00 0x58 0x20 0x8d 0x3e 0xf5 0x6d 
0x1b 0x75 0x0a 0x43 0x51 0xd6 0x8a 0xc2 
0x50 0xa0 0xe8 0x83 0x79 0x0e 0xfc 0x80 
0xa5 0x38 0xa4 0x44 0xee 0x9e 0x2b 0x57 
0xe2 0x44 0x1a 0x7c 0x21 
[responder]: received an EDHOC message (len 37):
0x0d 0x00 0x58 0x20 0x8d 0x3e 0xf5 0x6d 
0x1b 0x75 0x0a 0x43 0x51 0xd6 0x8a 0xc2 
0x50 0xa0 0xe8 0x83 0x79 0x0e 0xfc 0x80 
0xa5 0x38 0xa4 0x44 0xee 0x9e 0x2b 0x57 
0xe2 0x44 0x1a 0x7c 0x21 
[responder]: sending msg2 (46 bytes):
0x58 0x20 0x52 0xfb 0xa0 0xbd 0xc8 0xd9 
0x53 0xdd 0x86 0xce 0x1a 0xb2 0xfd 0x7c 
0x05 0xa4 0x65 0x8c 0x7c 0x30 0xaf 0xdb 
0xfc 0x33 0x01 0x04 0x70 0x69 0x45 0x1b 
0xaf 0x35 0x37 0x4a 0xa3 0xf1 0xbd 0x5d 
0x02 0x8d 0x19 0xcf 0x3c 0x99 
[initiator]: received a message (46 bytes):
0x58 0x20 0x52 0xfb 0xa0 0xbd 0xc8 0xd9 
0x53 0xdd 0x86 0xce 0x1a 0xb2 0xfd 0x7c 
0x05 0xa4 0x65 0x8c 0x7c 0x30 0xaf 0xdb 
0xfc 0x33 0x01 0x04 0x70 0x69 0x45 0x1b 
0xaf 0x35 0x37 0x4a 0xa3 0xf1 0xbd 0x5d 
0x02 0x8d 0x19 0xcf 0x3c 0x99 
[initiator]: sending msg3 (20 bytes):
0x37 0x52 0xd5 0x53 0x5f 0x31 0x47 0xe8 
0x5f 0x1c 0xfa 0xcd 0x9e 0x78 0xab 0xf9 
0xe0 0xa8 0x1b 0xbf 
[responder]: received an EDHOC message (len 20):
0x37 0x52 0xd5 0x53 0x5f 0x31 0x47 0xe8 
0x5f 0x1c 0xfa 0xcd 0x9e 0x78 0xab 0xf9 
0xe0 0xa8 0x1b 0xbf 
[responder]: finalize exchange
[responder]: handshake successfully completed
[initiator]: handshake successfully completed
[initiator]: Transcript hash 4 (32 bytes):
0x7c 0xcf 0xde 0xdc 0x2c 0x10 0xca 0x03 
0x56 0xe9 0x57 0xb9 0xf6 0xa5 0x92 0xe0 
0xfa 0x74 0xdb 0x2a 0xb5 0x4f 0x59 0x24 
0x40 0x96 0xf9 0xa2 0xac 0x56 0xd2 0x07 
init oscore

> init oscore
OSCORE secret:
0x5b 0xb2 0xae 0xe2 0x5b 0x16 0x0e 0x7c 
0x6d 0x26 0x12 0xb0 0xa6 0x01 0x09 0x16 

OSCORE salt:
0x8e 0x44 0x92 0x10 0xe0 0x3b 0xc2 0x9d 

resp oscore
> resp oscore
OSCORE secret:
0x5b 0xb2 0xae 0xe2 0x5b 0x16 0x0e 0x7c 
0x6d 0x26 0x12 0xb0 0xa6 0x01 0x09 0x16 

OSCORE salt:
0x8e 0x44 0x92 0x10 0xe0 0x3b 0xc2 0x9d 

@benpicco benpicco requested a review from miri64 November 11, 2022 11:57
@maribu maribu merged commit c114bb9 into RIOT-OS:2022.10-branch Nov 11, 2022
@maribu maribu deleted the backport/2022.10/sys/net/gnrc/pkt branch November 11, 2022 20:23
@maribu
Copy link
Member Author

maribu commented Nov 11, 2022

Thx :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: release backport Integration Process: The PR is a release backport of a change previously provided to master Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants