[Mesa-dev] [PATCH RFC] st/mesa: check st_context in st_renderbuffer_delete()

[Olivier Fourdan]

Hi,

On Fri, Aug 3, 2018 at 7:03 PM Aaron Watry <awatry at gmail.com> wrote:
>
> Tested-by: Aaron Watry <awatry at gmail.com>
>
> Yay, I can finally use gnome/wayland with the Slack snap again without
> insta-crashing my session.
>
> --Aaron
>
> On Thu, Aug 2, 2018 at 7:29 AM, Olivier Fourdan <ofourdan at redhat.com> wrote:
> > st_renderbuffer_delete() can segfault if we get a non-NULL context
> > pointer but if the st_context is NULL:
> >
> >   Thread 1 "Xwayland" received signal SIGSEGV, Segmentation fault.
> >   in st_renderbuffer_delete () at state_tracker/st_cb_fbo.c:241
> >   241 pipe_surface_release(st->pipe, &strb->surface_srgb);
> >   (gdb) bt
> >   #0  st_renderbuffer_delete () at state_tracker/st_cb_fbo.c:241
> >   #1  _mesa_reference_renderbuffer_ () at main/renderbuffer.c:212
> >   #2  _mesa_reference_renderbuffer () at main/renderbuffer.h:72
> >   #3  _mesa_free_framebuffer_data (0) at main/framebuffer.c:229
> >   #4  _mesa_destroy_framebuffer () at main/framebuffer.c:207
> >   #5  _mesa_reference_framebuffer_ () at main/framebuffer.c:265
> >   #6  _mesa_reference_framebuffer () at main/framebuffer.h:63
> >   #7  _mesa_free_context_data () at main/context.c:1326
> >   #8  st_destroy_context () at state_tracker/st_context.c:653
> >   #9  dri_destroy_context () at dri_context.c:239
> >   #10 driDestroyContext () at dri_util.c:524
> >   #11 __glXDRIcontextDestroy () at glxdriswrast.c:132
> >   #12 __glXFreeContext () at glxext.c:190
> >   #13 ContextGone () at glxext.c:82
> >   #14 doFreeResource () at resource.c:880
> >   #15 FreeResourceByType () at resource.c:941
> >   #16 __glXDisp_DestroyContext () at glxcmds.c:437
> >   #17 dispatch_DestroyContext () at vnd_dispatch_stubs.c:82
> >   #18 Dispatch () at dispatch.c:478
> >   #19 dix_main () at main.c:276
> >   #20 __libc_start_main () from /lib64/libc.so.6
> >   #21 _start () at glxcmds.c:125
> >
> >   (gdb) p st
> >   $1 = (struct st_context *) 0x0
> >
> > Check for a non-NULL st_context pointer as well to avoid the crash.
> >
> > Bugzilla: https://bugzilla.redhat.com/1611140
> > Signed-off-by: Olivier Fourdan <ofourdan at redhat.com>
> > ---
> >  Note: This fixes several bug reported downstream, like:
> >   https://bugzilla.redhat.com/1611140
> >   https://bugs.launchpad.net/ubuntu/+source/xorg-server/+bug/1762971
> >   https://bugs.launchpad.net/ubuntu/+source/mesa/+bug/1754693
> >   etc.
> >  I don't know what this client actually does, but whatever it is it should
> >  not crash Xwayland because of Mesa...
> >  I tested this fix against the given reproducer (run snap on Wayland/Xwayland)
> >  and it works.
> >
> >  src/mesa/state_tracker/st_cb_fbo.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/mesa/state_tracker/st_cb_fbo.c b/src/mesa/state_tracker/st_cb_fbo.c
> > index 73414fdfa1..856d213b73 100644
> > --- a/src/mesa/state_tracker/st_cb_fbo.c
> > +++ b/src/mesa/state_tracker/st_cb_fbo.c
> > @@ -238,8 +238,10 @@ st_renderbuffer_delete(struct gl_context *ctx, struct gl_renderbuffer *rb)
> >     struct st_renderbuffer *strb = st_renderbuffer(rb);
> >     if (ctx) {
> >        struct st_context *st = st_context(ctx);
> > -      pipe_surface_release(st->pipe, &strb->surface_srgb);
> > -      pipe_surface_release(st->pipe, &strb->surface_linear);
> > +      if (st) {
> > +         pipe_surface_release(st->pipe, &strb->surface_srgb);
> > +         pipe_surface_release(st->pipe, &strb->surface_linear);
> > +      }
> >        strb->surface = NULL;
> >     }
> >     pipe_resource_reference(&strb->texture, NULL);

To better understand why this crash occurs, I filed:

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107508

With a simple reproducer program. This is not affecting only Xwayland,
but also Xephyr with glamor backend as well.

Cheers,
Olivier