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 13:43, David Weinehall wrote:
> On Mon, Feb 15, 2016 at 12:24:04PM +0000, 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.
>
> My guess is that klockwork recognises that assert() can be a no-op
> if NDEBUG is defined and in such case won't generate code.
> In such a case neither of those two checks are performed.
>
> Kind regards, David

Klocwork is a static analysis tool, it doesn't generate code at all.
It's supposed to recognise assert() and similar macros as constraints on 
what values particular expressions may have; in particular, it knows 
that if a code path ends with a call to abort(), that path *should* 
never be taken and it will assume (for static analysis purposes) that it 
*will* not be taken. Thus any combination of values that leads to an 
abort() is considered "impossible" and need not be further checked. Then 
assert() is typically defined as:

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

and then

     int foo(char *s)
     {
         assert(s);
         return *s;
     }

should not cause Klocwork to complain, whereas without the assert() it 
should say that 's' might be NULL when dereferenced. If it's getting 
false positives it may be that Klocwork hasn't been told the proper 
(conditional) definition for assert()?

.Dave.

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