[PATCH DDX 0/7] Some issues found with Klockwork

[PATCH DDX 0/7] Some issues found with Klockwork

[Martin Peres]

The DDX clearly is making Klockwork unhappy but I found those potential
issues. Let's use them as a basis for discussion on the real code that
should be written.

Martin Peres (7):
  device: prevent a NULL pointer dereference in __intel_peek_fd
  display: prevent a NULL pointer dereference in
    intel_set_scanout_pixmap
  sna: check for NULL before dereferencing a pointer
  sna: assert a pointer is non-NULL before dereferencing it
  sna/cursor: add an assert on cursor->image
  sna_video_sprite: add asserts to catch invalid pipe#
  sna: add an assert in the lengthy sna_drawable_use_bo

 src/intel_device.c         | 5 ++++-
 src/sna/sna_accel.c        | 5 ++++-
 src/sna/sna_display.c      | 3 ++-
 src/sna/sna_video_sprite.c | 3 +++
 src/uxa/intel_display.c    | 5 +++++
 5 files changed, 18 insertions(+), 3 deletions(-)

-- 
2.7.1

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

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

[Martin Peres]

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);
 
-	return dev->fd;
+	if (!dev)
+		return -1;
+	else
+		return dev->fd;
 }
 
 int intel_has_render_node(struct intel_device *dev)
-- 
2.7.1

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

[PATCH 2/7] display: prevent a NULL pointer dereference in intel_set_scanout_pixmap

[Martin Peres]

Caught by Klockwork.

Signed-off-by: Martin Peres <martin.peres@linux.intel.com>
---
 src/uxa/intel_display.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/uxa/intel_display.c b/src/uxa/intel_display.c
index 8bf0184..44744a5 100644
--- a/src/uxa/intel_display.c
+++ b/src/uxa/intel_display.c
@@ -688,6 +688,11 @@ intel_set_scanout_pixmap(xf86CrtcPtr crtc, PixmapPtr ppix)
 	}
 
 	bo = intel_get_pixmap_bo(ppix);
+	if (!bo) {
+		ErrorF("pixmap is not backed by a BO\n");
+		return FALSE;
+	}
+
 	if (intel->front_buffer) {
 		ErrorF("have front buffer\n");
 	}
-- 
2.7.1

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

[PATCH 3/7] sna: check for NULL before dereferencing a pointer

[Martin Peres]

Caught by Klockwork and I genuinely can't tell if it is safe without
it, especially since all the surrounding code is checking for NULL.

Signed-off-by: Martin Peres <martin.peres@linux.intel.com>
---
 src/sna/sna_accel.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/sna/sna_accel.c b/src/sna/sna_accel.c
index 71a6207..8ceb1e1 100644
--- a/src/sna/sna_accel.c
+++ b/src/sna/sna_accel.c
@@ -3688,7 +3688,8 @@ sna_drawable_use_bo(DrawablePtr drawable, unsigned flags, const BoxRec *box,
 	     __FUNCTION__, priv->flush, priv->shm, priv->cpu, flags));
 
 	if ((flags & PREFER_GPU) == 0 &&
-	    (flags & (REPLACES | IGNORE_DAMAGE) || !priv->gpu_damage || !kgem_bo_is_busy(priv->gpu_bo))) {
+	    (flags & (REPLACES | IGNORE_DAMAGE) || !priv->gpu_damage ||
+	     (priv->gpu_bo && !kgem_bo_is_busy(priv->gpu_bo)))) {
 		DBG(("%s: try cpu as GPU bo is idle\n", __FUNCTION__));
 		goto use_cpu_bo;
 	}
-- 
2.7.1

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

[PATCH 4/7] sna: assert a pointer is non-NULL before dereferencing it

[Martin Peres]

Caught by Klockwork.

Signed-off-by: Martin Peres <martin.peres@linux.intel.com>
---
 src/sna/sna_accel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/sna/sna_accel.c b/src/sna/sna_accel.c
index 8ceb1e1..f1c136a 100644
--- a/src/sna/sna_accel.c
+++ b/src/sna/sna_accel.c
@@ -4376,6 +4376,7 @@ sna_pixmap_move_to_gpu(PixmapPtr pixmap, unsigned flags)
 
 	if (priv->shm) {
 		assert(!priv->flush);
+		assert(priv->cpu_bo);
 		sna_add_flush_pixmap(sna, priv, priv->cpu_bo);
 	}
 
-- 
2.7.1

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

[PATCH 5/7] sna/cursor: add an assert on cursor->image

[Martin Peres]

Caught by Klockwork, but it was a false positive. However, better safe
than sorry.

Signed-off-by: Martin Peres <martin.peres@linux.intel.com>
---
 src/sna/sna_display.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/sna/sna_display.c b/src/sna/sna_display.c
index 9215b23..78937c2 100644
--- a/src/sna/sna_display.c
+++ b/src/sna/sna_display.c
@@ -5573,8 +5573,9 @@ static struct sna_cursor *__sna_get_cursor(struct sna *sna, xf86CrtcPtr crtc)
 	argb = get_cursor_argb(sna->cursor.ref);
 	pitch = BitmapBytePad(width);
 
+	assert(cursor->image);
 	image = cursor->image;
-	if (image == NULL || transformed) {
+	if (transformed) {
 		image = sna->cursor.scratch;
 		cursor->last_width = cursor->last_height = size;
 	}
-- 
2.7.1

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

[PATCH 6/7] sna_video_sprite: add asserts to catch invalid pipe#

[Martin Peres]

Caught by Klockwork. This will be enough to catch those issues during
bringup.

Signed-off-by: Martin Peres <martin.peres@linux.intel.com>
---
 src/sna/sna_video_sprite.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/sna/sna_video_sprite.c b/src/sna/sna_video_sprite.c
index 9e85049..ae08ef7 100644
--- a/src/sna/sna_video_sprite.c
+++ b/src/sna/sna_video_sprite.c
@@ -86,6 +86,7 @@ static int sna_video_sprite_stop(ddStopVideo_ARGS)
 		int pipe;
 
 		pipe = sna_crtc_pipe(crtc);
+		assert(pipe < ARRAY_SIZE(video->bo));
 		if (video->bo[pipe] == NULL)
 			continue;
 
@@ -260,6 +261,7 @@ sna_video_sprite_show(struct sna *sna,
 		video->color_key_changed &= ~(1 << pipe);
 	}
 
+	assert(pipe < ARRAY_SIZE(video->bo));
 	if (video->bo[pipe] == frame->bo)
 		return true;
 
@@ -415,6 +417,7 @@ static int sna_video_sprite_put_image(ddPutImage_ARGS)
 		RegionIntersect(&reg, &reg, &clip);
 		if (RegionNil(&reg)) {
 off:
+			assert(pipe < ARRAY_SIZE(video->bo));
 			if (video->bo[pipe]) {
 				struct local_mode_set_plane s;
 				memset(&s, 0, sizeof(s));
-- 
2.7.1

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

[PATCH 7/7] sna: add an assert in the lengthy sna_drawable_use_bo

[Martin Peres]

I got lost in all the changes done on priv->flush and gpu_bo enough
not to be able to guarantee that if flush is non-null, so is gpu_bo.

Caught by Klockwork.

Signed-off-by: Martin Peres <martin.peres@linux.intel.com>
---
 src/sna/sna_accel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/sna/sna_accel.c b/src/sna/sna_accel.c
index f1c136a..595faa6 100644
--- a/src/sna/sna_accel.c
+++ b/src/sna/sna_accel.c
@@ -4074,6 +4074,7 @@ prefer_gpu_bo:
 	}
 	if (priv->flush) {
 		assert(!priv->shm);
+		assert(priv->gpu_bo);
 		sna_add_flush_pixmap(sna, priv, priv->gpu_bo);
 	}
 
-- 
2.7.1

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

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

[Chris Wilson]

On Fri, Feb 12, 2016 at 06:31:23PM +0200, Martin Peres wrote:
> This is not a big issue to return -1 since the only codepath that uses
> it is for display purposes.

Impossible.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] sna: assert a pointer is non-NULL before dereferencing it

[Chris Wilson]

On Fri, Feb 12, 2016 at 06:31:26PM +0200, Martin Peres wrote:
> Caught by Klockwork.

Impossible.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/7] sna_video_sprite: add asserts to catch invalid pipe#

[Chris Wilson]

On Fri, Feb 12, 2016 at 06:31:28PM +0200, Martin Peres wrote:
> Caught by Klockwork. This will be enough to catch those issues during
> bringup.

But you just said that klockwork didn't find a bug... :-p

I did contemplate having this stored on the CRTC instead.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/7] sna: add an assert in the lengthy sna_drawable_use_bo

[Chris Wilson]

On Fri, Feb 12, 2016 at 06:31:29PM +0200, Martin Peres wrote:
> I got lost in all the changes done on priv->flush and gpu_bo enough
> not to be able to guarantee that if flush is non-null, so is gpu_bo.
> 
> Caught by Klockwork.

Nope, impossible.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/7] sna/cursor: add an assert on cursor->image

[Chris Wilson]

On Fri, Feb 12, 2016 at 06:31:27PM +0200, Martin Peres wrote:
> Caught by Klockwork, but it was a false positive. However, better safe
> than sorry.

No. cursor->image can be NULL for old gen/kernels.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/7] sna: check for NULL before dereferencing a pointer

[Chris Wilson]

On Fri, Feb 12, 2016 at 06:31:25PM +0200, Martin Peres wrote:
> Caught by Klockwork and I genuinely can't tell if it is safe without
> it, especially since all the surrounding code is checking for NULL.

Impossible.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] display: prevent a NULL pointer dereference in intel_set_scanout_pixmap

[Chris Wilson]

On Fri, Feb 12, 2016 at 06:31:24PM +0200, Martin Peres wrote:
> Caught by Klockwork.
> 
> Signed-off-by: Martin Peres <martin.peres@linux.intel.com>
> ---
>  src/uxa/intel_display.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/uxa/intel_display.c b/src/uxa/intel_display.c
> index 8bf0184..44744a5 100644
> --- a/src/uxa/intel_display.c
> +++ b/src/uxa/intel_display.c
> @@ -688,6 +688,11 @@ intel_set_scanout_pixmap(xf86CrtcPtr crtc, PixmapPtr ppix)
>  	}
>  
>  	bo = intel_get_pixmap_bo(ppix);
> +	if (!bo) {
> +		ErrorF("pixmap is not backed by a BO\n");

Just return FALSE.

> +		return FALSE;
> +	}
> +
>  	if (intel->front_buffer) {
>  		ErrorF("have front buffer\n");

And let's add the return here as well instead of the ErrorF

If the caller does make the mistake of passing in an invalid Pixmap,
what's the likelihood of them checking the return? ;)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Dave Gordon]

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.

.Dave.

> -	return dev->fd;
> +	if (!dev)
> +		return -1;
> +	else
> +		return dev->fd;
>   }
>
>   int intel_has_render_node(struct intel_device *dev)

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

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

[Martin Peres]

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...
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[David Weinehall]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Dave Gordon]

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);
?

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

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

[Dave Gordon]

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

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

[Martin Peres]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Dave Gordon]

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

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

[Martin Peres]

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