[Mesa-dev] i965: Fix crash when calling glViewport with no surface bound

Submitted by Neil Roberts on Dec. 8, 2015, 4:35 p.m.

Details

Message ID 1449592557-16335-1-git-send-email-neil@linux.intel.com
State Accepted
Commit 8c5310da9d1cbf2272f72d3ed4264544456a4683
Headers show
Series "i965: Fix crash when calling glViewport with no surface bound" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Neil Roberts Dec. 8, 2015, 4:35 p.m.
If EGL_KHR_surfaceless_context is used then glViewport can be called
with NULL for the draw and read surfaces. This was previously causing
a crash because the i965 driver tries to use this point to invalidate
the surfaces and it was derferencing the NULL pointer.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93257
Cc: Nanley Chery <nanley.g.chery@intel.com>
Cc: "11.1" <mesa-stable@lists.freedesktop.org>
---

I've also posted a Piglit test for this here:

http://patchwork.freedesktop.org/patch/67356/

 src/mesa/drivers/dri/i965/brw_context.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/i965/brw_context.c
index 7d7643c..5374bad 100644
--- a/src/mesa/drivers/dri/i965/brw_context.c
+++ b/src/mesa/drivers/dri/i965/brw_context.c
@@ -159,8 +159,10 @@  intel_viewport(struct gl_context *ctx)
    __DRIcontext *driContext = brw->driContext;
 
    if (_mesa_is_winsys_fbo(ctx->DrawBuffer)) {
-      dri2InvalidateDrawable(driContext->driDrawablePriv);
-      dri2InvalidateDrawable(driContext->driReadablePriv);
+      if (driContext->driDrawablePriv)
+         dri2InvalidateDrawable(driContext->driDrawablePriv);
+      if (driContext->driReadablePriv)
+         dri2InvalidateDrawable(driContext->driReadablePriv);
    }
 }
 

Comments

On 8 December 2015 at 16:35, Neil Roberts <neil@linux.intel.com> wrote:
> If EGL_KHR_surfaceless_context is used then glViewport can be called
> with NULL for the draw and read surfaces. This was previously causing
> a crash because the i965 driver tries to use this point to invalidate
> the surfaces and it was derferencing the NULL pointer.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93257
> Cc: Nanley Chery <nanley.g.chery@intel.com>
> Cc: "11.1" <mesa-stable@lists.freedesktop.org>
Worth throwing in 11.0 as well ?

> ---
>
> I've also posted a Piglit test for this here:
>
> http://patchwork.freedesktop.org/patch/67356/
>
>  src/mesa/drivers/dri/i965/brw_context.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/i965/brw_context.c
> index 7d7643c..5374bad 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.c
> +++ b/src/mesa/drivers/dri/i965/brw_context.c
> @@ -159,8 +159,10 @@ intel_viewport(struct gl_context *ctx)
>     __DRIcontext *driContext = brw->driContext;
>
>     if (_mesa_is_winsys_fbo(ctx->DrawBuffer)) {
> -      dri2InvalidateDrawable(driContext->driDrawablePriv);
> -      dri2InvalidateDrawable(driContext->driReadablePriv);
> +      if (driContext->driDrawablePriv)
> +         dri2InvalidateDrawable(driContext->driDrawablePriv);
> +      if (driContext->driReadablePriv)
> +         dri2InvalidateDrawable(driContext->driReadablePriv);
 Afaict i915 could use an identical fix ?

-Emil
Emil Velikov <emil.l.velikov@gmail.com> writes:

> Worth throwing in 11.0 as well ?

Yeah, that would probably be sensible.

>>     if (_mesa_is_winsys_fbo(ctx->DrawBuffer)) {
>> -      dri2InvalidateDrawable(driContext->driDrawablePriv);
>> -      dri2InvalidateDrawable(driContext->driReadablePriv);
>> +      if (driContext->driDrawablePriv)
>> +         dri2InvalidateDrawable(driContext->driDrawablePriv);
>> +      if (driContext->driReadablePriv)
>> +         dri2InvalidateDrawable(driContext->driReadablePriv);
>
>  Afaict i915 could use an identical fix ?

Yes, I think you're right. However I don't have any way of testing it so
I feel a bit uncomfortable touching i915 driver.

Regards,
- Neil
On 9 December 2015 at 14:57, Neil Roberts <neil@linux.intel.com> wrote:
> Emil Velikov <emil.l.velikov@gmail.com> writes:
>
>> Worth throwing in 11.0 as well ?
>
> Yeah, that would probably be sensible.
>
>>>     if (_mesa_is_winsys_fbo(ctx->DrawBuffer)) {
>>> -      dri2InvalidateDrawable(driContext->driDrawablePriv);
>>> -      dri2InvalidateDrawable(driContext->driReadablePriv);
>>> +      if (driContext->driDrawablePriv)
>>> +         dri2InvalidateDrawable(driContext->driDrawablePriv);
>>> +      if (driContext->driReadablePriv)
>>> +         dri2InvalidateDrawable(driContext->driReadablePriv);
>>
>>  Afaict i915 could use an identical fix ?
>
> Yes, I think you're right. However I don't have any way of testing it so
> I feel a bit uncomfortable touching i915 driver.
>
Considering it's a null-deref fix one can just move the check in
dri2InvalidateDrawable, which would spare going through i915, i965,
radeon... ;-)

Just throwing it out there, it's up-to whichever route you want to take.
-Emil
On Tue, Dec 08, 2015 at 04:35:57PM +0000, Neil Roberts wrote:
> If EGL_KHR_surfaceless_context is used then glViewport can be called
> with NULL for the draw and read surfaces. This was previously causing
> a crash because the i965 driver tries to use this point to invalidate
> the surfaces and it was derferencing the NULL pointer.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93257
> Cc: Nanley Chery <nanley.g.chery@intel.com>
> Cc: "11.1" <mesa-stable@lists.freedesktop.org>
> ---

Thanks for the fix! I'll be able to give this a better look next week, so
feel free to push if you receive an Rb before then.

Tested-by: Nanley Chery <nanley.g.chery@intel.com>

>
> I've also posted a Piglit test for this here:
>
> http://patchwork.freedesktop.org/patch/67356/
>
> src/mesa/drivers/dri/i965/brw_context.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_context.c
b/src/mesa/drivers/dri/i965/brw_context.c
> index 7d7643c..5374bad 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.c
> +++ b/src/mesa/drivers/dri/i965/brw_context.c
> @@ -159,8 +159,10 @@ intel_viewport(struct gl_context *ctx)
> __DRIcontext *driContext = brw->driContext;
>
> if (_mesa_is_winsys_fbo(ctx->DrawBuffer)) {
> - dri2InvalidateDrawable(driContext->driDrawablePriv);
> - dri2InvalidateDrawable(driContext->driReadablePriv);
> + if (driContext->driDrawablePriv)
> + dri2InvalidateDrawable(driContext->driDrawablePriv);
> + if (driContext->driReadablePriv)
> + dri2InvalidateDrawable(driContext->driReadablePriv);
> }
> }
>
> --
> 1.9.3
>
> _______________________________________________
> mesa-stable mailing list
> mesa-stable@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-stable
On Thu, Dec 10, 2015 at 01:08:37PM -0800, Nanley Chery wrote:
> On Tue, Dec 08, 2015 at 04:35:57PM +0000, Neil Roberts wrote:
> > If EGL_KHR_surfaceless_context is used then glViewport can be called
> > with NULL for the draw and read surfaces. This was previously causing
> > a crash because the i965 driver tries to use this point to invalidate
> > the surfaces and it was derferencing the NULL pointer.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93257
> > Cc: Nanley Chery <nanley.g.chery@intel.com>
> > Cc: "11.1" <mesa-stable@lists.freedesktop.org>
> > ---
> 
> Thanks for the fix! I'll be able to give this a better look next week, so
> feel free to push if you receive an Rb before then.
> 
> Tested-by: Nanley Chery <nanley.g.chery@intel.com>


While I can't say that I know all the interactions with framebuffers,
I can't see a use-case for segfaulting on a NULL surface.

Reviewed-by: Nanley Chery <nanley.g.chery@intel.com>

> 
> >
> > I've also posted a Piglit test for this here:
> >
> > http://patchwork.freedesktop.org/patch/67356/
> >
> > src/mesa/drivers/dri/i965/brw_context.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_context.c
> b/src/mesa/drivers/dri/i965/brw_context.c
> > index 7d7643c..5374bad 100644
> > --- a/src/mesa/drivers/dri/i965/brw_context.c
> > +++ b/src/mesa/drivers/dri/i965/brw_context.c
> > @@ -159,8 +159,10 @@ intel_viewport(struct gl_context *ctx)
> > __DRIcontext *driContext = brw->driContext;
> >
> > if (_mesa_is_winsys_fbo(ctx->DrawBuffer)) {
> > - dri2InvalidateDrawable(driContext->driDrawablePriv);
> > - dri2InvalidateDrawable(driContext->driReadablePriv);
> > + if (driContext->driDrawablePriv)
> > + dri2InvalidateDrawable(driContext->driDrawablePriv);
> > + if (driContext->driReadablePriv)
> > + dri2InvalidateDrawable(driContext->driReadablePriv);
> > }
> > }
> >
> > --
> > 1.9.3
> >
> > _______________________________________________
> > mesa-stable mailing list
> > mesa-stable@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-stable