Network Coverity fixes #34037
Network Coverity fixes #34037
Conversation
The iface pointer might be null so check it before access. Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
Do not try to set or get the interface MTU if the interface pointer is NULL. Coverity-CID: 220541 Fixes zephyrproject-rtos#34000 Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
Simplifying the loop in order to remove dead code (ctx_up is always NULL after the slist loop. Coverity-CID: 220538 Fixes zephyrproject-rtos#34003 Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
The virtual_iface is already NULL checked by net_if_get_by_iface() at the beginning of the function so no need to do it here too. Coverity-CID: 220535 Fixes zephyrproject-rtos#34006 Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
| static inline uint16_t net_if_get_mtu(struct net_if *iface) | ||
| { | ||
| if (iface == NULL) { | ||
| return 0U; | ||
| } | ||
|
|
||
| return iface->if_dev->mtu; | ||
| } |
There was a problem hiding this comment.
I don't think that this fixes #34000. It might silence Coverity warning for some reason, but I don't see direct connection between this change and reported Coverity warning.
Additionally I think that net_if_get_mtu() should be used only on valid (non NULL) iface pointers. I don't see the reason why this API should be NULL "friendly" while other APIs not.
There was a problem hiding this comment.
I don't think that this fixes #34000. It might silence Coverity warning for some reason, but I don't see direct connection between this change and reported Coverity warning.
It fixes the issue because the mtu function now checks the NULL and does not crash.
The NULL check in net_if_flag_is_set() is a valid issue as I saw a crash when testing these. Adding the checks to MTU functions are useful to have as these could be called by the application and we are not calling these functions constantly so there is no performance concerns. IMHO we should add more NULL checks in various network functions as the asserts are not enough.
There was a problem hiding this comment.
Sorry for not getting back to this PR for so long.
The NULL check in
net_if_flag_is_set()is a valid issue as I saw a crash when testing these.
That is one point of view. I see it in a bit different way, i.e. any crash is a result of bug, which is not checking for iface != NULL. Such crash is quite easy to debug and fix, compared to continuing execution with buggy logic.
IMHO we should add more NULL checks in various network functions as the asserts are not enough.
Why are asserts not enough? Because they are not compiled in by default?
Simply checking for NULL parameters and returning arbitrary values in case of NULL doesn't solve all issues. It solves crashes in net_if code (like in net_if_flag_is_set()), but it doesn't make higher level code that executes net_if functions any better. Functions like net_if_flag_is_set() are in most cases called with other net_if functions. Should we check iface argument in all net_if functions? Additionally net_if_flag_is_set(NULL, flag) == 0 logic doesn't have to be always correct. Take into account this fragment:
Lines 460 to 467 in facd0f5
If someone will use if (!net_if_flag_is_set(iface, FLAG)) (notice negation !), then we need to make sure to not dereference iface later on, as it might still be NULL. I think that it is easier to make sure iface != NULL from the beginning and leave net_if_flag_is_set() API as is, without adding NULL check resulting in (slightly, but still) increased flash size and (small) performance penalty. IMO crashes because of NULL pointer dereferencing are often easier to debug than some issues in logic, which could happen after treating NULL in functions like net_if_flag_is_set() as a "valid" interface.
Misc Coverity fixes
Fixes #34000
Fixes #34003
Fixes #34006