On 10/12/22 08:10, Colin King (gmail) wrote:
Hi,
Static analysis with cppcheck has found an issue with a function that returns an
int value but there is a code path that does not return a value causing
undefined behaviour:
Source: drivers/net/wireless/realtek/rtlwifi/usb.c function _rtl_rx_get_padding
- introduced by commit:
commit 354d0f3c40fb40193213e40f3177ff528798ca8d
Author: Larry Finger <Larry.Finger@xxxxxxxxxxxx>
Date: Wed Sep 25 12:57:47 2013 -0500
rtlwifi: Fix smatch warnings in usb.c
The issue occurs when NET_IP_ALIGN is zero and when len >= sizeof(*hdr), then
the following return is *not* taken:
/* make function no-op when possible */
if (NET_IP_ALIGN == 0 || len < sizeof(*hdr))
return 0
and then execution reaches the end of the function where no return value is
returned because the #if NET_IP_ALIGN != 0 is false so the return padding hunk
of the code is not compiled in.
Colin,
The no-op code snippet you quote above returns zero if NET_IP_ALIGN is zero. In
that case, the value of len is irrelevant. If NET_IP_ALIGN is not zero, then the
function returns a value in every case.
I admit that the logic is overly convoluted and clearly it overwhelmed cppcheck,
but as I see it, the problem is with the tool, not with the code.
Obviously, a patch such as shown below would let your tool come to the correct
conclusion, but it is not wrong now.
diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c
b/drivers/net/wireless/realtek/rtlwifi/usb.c
index a8eebafb9a7e..7b7277b33a7e 100644
--- a/drivers/net/wireless/realtek/rtlwifi/usb.c
+++ b/drivers/net/wireless/realtek/rtlwifi/usb.c
@@ -554,13 +554,11 @@ static unsigned int _rtl_rx_get_padding(struct
ieee80211_hdr *hdr,
{
#if NET_IP_ALIGN != 0
unsigned int padding = 0;
-#endif
/* make function no-op when possible */
- if (NET_IP_ALIGN == 0 || len < sizeof(*hdr))
+ if (len < sizeof(*hdr))
return 0;
-#if NET_IP_ALIGN != 0
/* alignment calculation as in lbtf_rx() / carl9170_rx_copy_data() */
/* TODO: deduplicate common code, define helper function instead? */
@@ -581,6 +579,8 @@ static unsigned int _rtl_rx_get_padding(struct ieee80211_hdr
*hdr,
padding ^= NET_IP_ALIGN;
return padding;
+#else
+ return 0;
#endif
}
I happen to believe that fixing bugs in tools is more important than needlessly
modifying the kernel so that the tool gets the right answer. I say that as
someone that has been burned in the past by a faulty tool.
Larry
