Re: [PATCH] staging: rtl8188eu: os_dep: usb_intf.c: Fix for possible null pointer dereference

[Rickard Strandqvist]

Hi

Exactly as you say Dan, cppcheck has found that there is a null check
at some point while the others in this case are missed. So I'm not for
unnecessary checks.
But we can instead agree that I make a patch that will remove all null
checks padapter?

Dan: I'll check on the scripts/checkpatch.pl

Best regards
Rickard Strandqvist


2014-05-21 9:24 GMT+02:00 Dan Carpenter <dan.carpenter@xxxxxxxxxx>:
> On Tue, May 20, 2014 at 04:57:56PM -0700, josh@xxxxxxxxxxxxxxxx wrote:
>> On Tue, May 20, 2014 at 06:26:51PM -0500, Larry Finger wrote:
>> > On 05/20/2014 04:31 PM, Rickard Strandqvist wrote:
>> > >There is otherwise a risk of a possible null pointer dereference.
>> > >
>> > >Was largely found by using a static code analysis program called cppcheck.
>> > >
>> > >Signed-off-by: Rickard Strandqvist <rickard_strandqvist@xxxxxxxxxxxxxxxxxx>
>> > >---
>> > >  drivers/staging/rtl8188eu/os_dep/usb_intf.c |  127 ++++++++++++++-------------
>> > >  1 file changed, 66 insertions(+), 61 deletions(-)
>> >
>> > Although cppcheck's analysis is correct, pointer padapter can never
>> > be null in any of these routines. In routine rtw_drv_init(),
>> > rtw_usb_if1_init() is called to allocate memory for struct adapter
>> > for the driver. If that fails, none of these other routines will
>> > ever be called as the driver will not be loaded.
>> >
>> > If it is deemed better to fill the code with needless tests because
>> > some static checker points out a place like this, that is OK with
>> > me, but I do not see the point.
>>
>> If it's an invariant of the code that padapter cannot ever be NULL, then
>> that seems perfectly fine to rely on, and the tool just needs fixing or
>> silencing.  At most, it might be helpful to add annotations like GCC's
>> "nonnull", if that helps the checker and the compiler generate more
>> useful warnings.
>
> The tool is complaining that the existing checks for NULL don't make
> sense.  Rickard changed them to cover all the dereferences but really we
> should just remove the checks.
>
> Btw, speaking of GCC, it now silently removes inconsistent NULL
> checking.  It takes the opposite of the normall kernel programmer
> approach of adding checks everywhere.  :P  The glibc people recently
> annotated qsort() to say that the function pointer can't be NULL even
> though this "worked" in the past.  Hillarity!
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61236
>
> regards,
> dan carpenter
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/