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

[Martin Peres <martin.peres@linux.intel.com>]

On 16/02/16 10:54, Dave Gordon wrote:
> 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

That sounds like a good idea, yes.

Here is the current definition: 
https://cgit.freedesktop.org/xorg/driver/xf86-video-intel/tree/src/sna/xassert.h

>
> 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.

We don't, because there is a test on NDEBUG which Klokwork cannot make 
assumptions on, as David said.

I will hold on to this idea a little as there are talks internally on to 
which static analysis tool needs to be used.

Thanks for your feedback,
Martin
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx