Re: [PATCH 1/7] device: prevent a NULL pointer dereference in __intel_peek_fd

[Dave Gordon <david.s.gordon@intel.com>]

On 15/02/16 15:56, Martin Peres wrote:
> On 15/02/16 15:47, Dave Gordon wrote:
>> On 15/02/16 13:40, Martin Peres wrote:
>>> On 15/02/16 14:24, Dave Gordon wrote:
>>>> On 12/02/16 16:31, Martin Peres wrote:
>>>>> This is not a big issue to return -1 since the only codepath that uses
>>>>> it is for display purposes.
>>>>>
>>>>> Caught by Klockwork.
>>>>>
>>>>> Signed-off-by: Martin Peres <martin.peres@linux.intel.com>
>>>>> ---
>>>>>   src/intel_device.c | 5 ++++-
>>>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/src/intel_device.c b/src/intel_device.c
>>>>> index 54c1443..35e652a 100644
>>>>> --- a/src/intel_device.c
>>>>> +++ b/src/intel_device.c
>>>>> @@ -650,7 +650,10 @@ int __intel_peek_fd(ScrnInfoPtr scrn)
>>>>>       dev = intel_device(scrn);
>>>>>       assert(dev && dev->fd != -1);
>>>>
>>>> Doesn't Klocwork recognise the assert() above?
>>>> I thought that would tell it that dev can't be NULL.
>>>
>>> It does not, I had to close many false positives related to this...
>>
>> Hmmm .. elsewhere (e.g. [4/7]) you have /added/ an assert, which I
>> thought must be so that Klocwork stops complaining that something might
>> be NULL ... maybe it can't handle the composite assertion? Does it
>> silence the complaint if you change:
>>      assert(dev && dev->fd != -1);
>> into:
>>      assert(dev);
>>      assert(dev->fd != -1);
>> ?
>
> Sure, I added an assert, but not to silence patchwork, just to make sure
> we have no problem. I cannot run klokwork myself and my goal was not to
> silence but instead to check the reported issues.
>
> David is right, I think Klokwork only cares about runtime checks and
> wants to make sure that we never de-reference a NULL pointer.
>
> Martin

Klocwork is trying (by static analysis) to find all reachable code, with 
all possible parameter values at each point. It's configured with 
various checkers that examine each expression reached for things such as 
dereferencing a possibly-NULL pointer, or indexing beyond the bounds of 
an array, or integer overflow, or many other things ...

The standard definition of assert() is something like:

     #define assert(x) do { if(!(x)) abort(); } while (0)

and Klocwork knows that abort() doesn't return, so in the block

     dev = intel_device(scrn);
     assert(dev);
     return dev->fd;

it can deduce that the 'return' is reached only if the abort() was not, 
hence only if 'dev' is non-NULL. Therefore, this doesn't produce a 
complaint about a possibly-NULL pointer, because Klocwork knows it isn't 
because of the assert().

Of course there are potentially multiple definitions of assert(), 
typically including a null one, for production code, and a debug version 
that gives more detail. So the usual thing is to ensure that there's a 
Klocwork-specific version that allows KW to do the analysis above, even 
if that version isn't something you would ever run:

#if    defined(__KLOCWORK__)
#define assert(x) do { if(!(x)) abort(); } while (0)
#elif  defined(NO_DEBUG)
#define assert(x) do { /* nothing */ ; } while (0)
#elif  defined(EXTRA_DEBUG)
#define assert(x) do { my_assert(x, #x, __LINE__, __FILE__); } while (0)
#else
// ... etc ...
#endif

If we don't have something like this, Klocwork may not be able to make 
effective deductions about the possible values of variables at specific 
points, so it would be worth checking that we're using macros that it 
understands.

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx