sys/net/grnc: fix NULL ptr dereferencing #20660
Conversation
maribu
added
the
Type: bug
| @@ -95,8 +95,6 @@ void _snd_rtr_advs(gnrc_netif_t *netif, const ipv6_addr_t *dst, bool final) | |||
| sizeof(addr_str))); | |||
| _snd_ra(netif, dst, final, abr); | |||
| } | |||
| } else { | |||
| _snd_ra(netif, dst, final, NULL); | |||
There was a problem hiding this comment.
I think this is the wrong line to remove / to fix. As you pointed out in chat: This can still be reached by a 6LR that does not have multihop prefix and 6LoWPAN context dissemination (MULTIHOP_P6C) activated.
There was a problem hiding this comment.
I am a little bit confused, why GCC then comes to this conclusion though. Maybe it helps to make the IS_ACTIVE() check here
RIOT/sys/net/gnrc/network_layer/ipv6/nib/_nib-router.c
Lines 247 to 248 in 08cdf37
a C-conditional to help GCC figure this out correctly?
There was a problem hiding this comment.
GCC did figure this out correctly: If CONFIG_GNRC_IPV6_NIB_MULTIHOP_P6C is used and a netif is 6ln but not 6lr, the NULL pointer dereferencing would very much happen.
There was a problem hiding this comment.
GCC did figure this out correctly: If
CONFIG_GNRC_IPV6_NIB_MULTIHOP_P6Cis used and a netif is 6ln but not 6lr, theNULLpointer dereferencing would very much happen.
Ok, but your line removal also removes the “netif is a 6lr but does not use the CONFIG_GNRC_IPV6_NIB_MULTIHOP_P6C” case, which is a correct scenario. Then I guess, the check for CONFIG_GNRC_IPV6_NIB_MULTIHOP_P6C check either needs a check for the netif also being a 6lr (which would be the right check spec-wise) or if abr is NULL (which would be the safer check code-wise).
There was a problem hiding this comment.
So, maybe this is the correct fix? Checking for 6lr in both cases?
GCC's static analysis does indeed notice that the check then is the same in both cases.
There was a problem hiding this comment.
check for the
netifalso being a 6lr (which would be the right check spec-wise) or ifabris NULL (which would be the safer check code-wise).
OK, I did both now with the check for abr being an assert().
maribu
force-pushed
the
sys/net/grnc/fix-compilation-with-gcc-14-1-0
branch
2 times, most recently
from
8c9cb2e to
15a84eb
Compare
benpicco
added
the
CI: ready for build
This bug was spotted by GCC 14.1.0:
In file included from /home/maribu/Repos/software/RIOT/master/sys/net/gnrc/network_layer/ipv6/nib/_nib-internal.h:27,
from /home/maribu/Repos/software/RIOT/master/sys/net/gnrc/network_layer/ipv6/nib/_nib-arsm.h:33,
from /home/maribu/Repos/software/RIOT/master/sys/net/gnrc/network_layer/ipv6/nib/_nib-6ln.h:30,
from /home/maribu/Repos/software/RIOT/master/sys/net/gnrc/network_layer/ipv6/nib/_nib-router.c:28:
In function 'bf_isset',
inlined from '_build_ext_opts' at /home/maribu/Repos/software/RIOT/master/sys/net/gnrc/network_layer/ipv6/nib/_nib-router.c:256:17,
inlined from '_snd_ra' at /home/maribu/Repos/software/RIOT/master/sys/net/gnrc/network_layer/ipv6/nib/_nib-router.c:368:20,
inlined from '_snd_rtr_advs' at /home/maribu/Repos/software/RIOT/master/sys/net/gnrc/network_layer/ipv6/nib/_nib-router.c:99:9:
/home/maribu/Repos/software/RIOT/master/sys/include/bitfield.h:130:18: error: array subscript 0 is outside array bounds of 'uint8_t[0]' {aka 'unsigned char[]'} [-Werror=array-bounds=]
130 | return (field[idx / 8] & (1u << (7 - (idx % 8))));
| ~~~~~^~~~~~~~~
In function '_snd_rtr_advs':
cc1: note: source object is likely at address zero
cc1: all warnings being treated as errors
maribu
force-pushed
the
sys/net/grnc/fix-compilation-with-gcc-14-1-0
branch
from
15a84eb to
abd7205
Compare
|
Thanks for the fix! |
Contribution description
This bug was spotted by GCC 14.1.0:
Testing procedure
Apps that use GNRC and send router advertisements should still work.
Issues/PRs references
None