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 <[email protected]>
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
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 <[email protected]>
> 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