Bug 101829 - read-after-free in st_framebuffer_validate
Summary: read-after-free in st_framebuffer_validate
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/Gallium/swr (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: mesa-dev
QA Contact: mesa-dev
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-07-18 14:30 UTC by Brad King
Modified: 2017-07-25 12:04 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
apitrace of test that crashes (175.62 KB, application/octet-stream)
2017-07-18 14:30 UTC, Brad King
Details
Assign a unique ID to the framebuffer interface object created in xm_st.c (1.39 KB, patch)
2017-07-18 19:34 UTC, charmainel
Details | Splinter Review

Description Brad King 2017-07-18 14:30:15 UTC
Created attachment 132745 [details]
apitrace of test that crashes

Since commit 147d7fb772 (st/mesa: add a winsys buffers list in st_context, 2017-07-10) one of VTK's tests crashes with Mesa.  Here is output from valgrind's memcheck tool:

Invalid read of size 4
   at 0xE986121: st_framebuffer_validate (st_manager.c:180)
   by 0xE9876C8: st_api_make_current (st_manager.c:851)
   by 0xE600FBA: XMesaMakeCurrent2 (xm_api.c:1307)
   by 0xE5FBD01: glXMakeContextCurrent (glx_api.c:1239)
   by 0x4034FAF: ??? (in /usr/lib/x86_64-linux-gnu/qt5/plugins/xcbglintegrations/libqxcb-glx-integration.so)
   by 0x8D1ECB7: QOpenGLContext::makeCurrent(QSurface*) (in /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5.7.1)
   by 0x8751910: QOpenGLWidget::makeCurrent() (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.7.1)
   by 0x8751EB7: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.7.1)
   by 0x8752722: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.7.1)
   by 0x93CF876: QObject::~QObject() (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.7.1)
   by 0x872D922: QWidget::~QWidget() (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.7.1)
   by 0x522767F: QVTKOpenGLWidget::~QVTKOpenGLWidget() (QVTKOpenGLWidget.cxx:136)
 Address 0x295d09b0 is 0 bytes inside a block of size 40 free'd
   at 0x4C2CDDB: free (vg_replace_malloc.c:530)
   by 0xE602156: xmesa_destroy_st_framebuffer (xm_st.c:324)
   by 0xE5FFEC1: xmesa_free_buffer (xm_api.c:601)
   by 0xE600E19: XMesaDestroyBuffer (xm_api.c:1241)
   by 0xE6013C0: XMesaGarbageCollect (xm_api.c:1447)
   by 0xE5FC137: glXDestroyContext (glx_api.c:1426)
   by 0x4033200: ??? (in /usr/lib/x86_64-linux-gnu/qt5/plugins/xcbglintegrations/libqxcb-glx-integration.so)
   by 0x4033228: ??? (in /usr/lib/x86_64-linux-gnu/qt5/plugins/xcbglintegrations/libqxcb-glx-integration.so)
   by 0x8D202CA: QOpenGLContext::destroy() (in /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5.7.1)
   by 0x8D205F6: QOpenGLContext::~QOpenGLContext() (in /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5.7.1)
   by 0x8D20608: QOpenGLContext::~QOpenGLContext() (in /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5.7.1)
   by 0x8722097: QWidgetPrivate::deleteTLSysExtra() (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.7.1)
 Block was alloc'd at
   at 0x4C2DBC5: calloc (vg_replace_malloc.c:711)
   by 0xE601FB5: xmesa_create_st_framebuffer (xm_st.c:285)
   by 0xE5FFD9E: create_xmesa_buffer (xm_api.c:543)
   by 0xE600A67: XMesaCreateWindowBuffer (xm_api.c:1100)
   by 0xE5FBBD7: glXMakeContextCurrent (glx_api.c:1200)
   by 0xE5FBDE6: glXMakeCurrent (glx_api.c:1273)
   by 0x4034517: ??? (in /usr/lib/x86_64-linux-gnu/qt5/plugins/xcbglintegrations/libqxcb-glx-integration.so)
   by 0x40328B6: ??? (in /usr/lib/x86_64-linux-gnu/qt5/plugins/xcbglintegrations/libqxcb-glx-integration.so)
   by 0x40F9040: QXcbIntegration::createPlatformOpenGLContext(QOpenGLContext*) const (in /usr/lib/x86_64-linux-gnu/libQt5XcbQpa.so.5.7.1)
   by 0x8D208CC: QOpenGLContext::create() (in /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5.7.1)
   by 0x8750CFD: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.7.1)
   by 0x8751129: QOpenGLWidget::resizeEvent(QResizeEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.7.1)
Comment 1 Brian Paul 2017-07-18 14:51:02 UTC
Hi Brad,

I'm not able to repro the problem so far.  Valgrind shows no errors with your trace.  I've both tried Mesa @ 147d7fb772 and ToT as of this morning (a522ce997779).  I tested with the llvmpipe driver (though I don't think the driver should matter in this case).  Any ideas?

There have been a couple bug-fix follow-on commits since 147d7fb772 so maybe you can re-test with latest Mesa.
Comment 2 Brad King 2017-07-18 15:00:41 UTC
I have a script that updates Mesa every night.  After noticing the problem I bisected back to 147d7fb772.  I just tried with current master (a522ce9977) and it still happens.  For reference, I'm building Mesa as follows:

    ./autogen.sh \
      --prefix="$prefix" \
      --enable-debug \
      --disable-dri \
      --disable-egl \
      --disable-gbm \
      --disable-gles1 \
      --disable-gles2 \
      --disable-shared-glapi \
      --with-platforms=x11 \
      --enable-glx=gallium-xlib \
      --enable-gallium-osmesa \
      --with-gallium-drivers=swrast \
      --enable-gallium-llvm=yes \
        LLVM_CONFIG=/usr/bin/llvm-config-3.8 \
      --enable-llvm-shared-libs \
      --with-gl-lib-name=MesaGL \
      --with-osmesa-lib-name=MesaOSMesa &&
Comment 3 Gert Wollny 2017-07-18 19:25:12 UTC
I can confirm that the trace results in a sigsegv, but with gltrace on r600g I get a different backtrace (9ee67467c9ea + a patchset related to register merging that shouldn't have to do anything with the bug) 

valgrind glretrace Downloads/example.trace 
==8227== Memcheck, a memory error detector
==8227== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==8227== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==8227== Command: glretrace Downloads/example.trace
==8227== 
==8227== Invalid read of size 4
==8227==    at 0x9A9AC88: st_framebuffers_purge (st_manager.c:509)
==8227==    by 0x9A9AC88: st_api_make_current (st_manager.c:872)
==8227==    by 0x9C457CD: dri_make_current (dri_context.c:278)
==8227==    by 0x9C44283: driBindContext (dri_util.c:559)
==8227==    by 0x77425EA: dri2_bind_context (dri2_glx.c:154)
==8227==    by 0x771930B: MakeContextCurrent (glxcurrent.c:228)
==8227==    by 0x40A406: glws::makeCurrentInternal(glws::Drawable*, glws::Context*) (glws_glx.cpp:370)
==8227==    by 0x412C3E: makeCurrent (glws.hpp:213)
==8227==    by 0x412C3E: glretrace::makeCurrent(trace::Call&, glws::Drawable*, glretrace::Context*) (glretrace_ws.cpp:170)
==8227==    by 0x40C8BC: retrace::retraceCall(trace::Call*) (retrace_main.cpp:233)
==8227==    by 0x40CE2F: runLeg (retrace_main.cpp:386)
==8227==    by 0x40CE2F: runRace (retrace_main.cpp:364)
==8227==    by 0x40CE2F: retrace::RelayRace::run() (retrace_main.cpp:505)
==8227==    by 0x407D97: mainLoop (retrace_main.cpp:565)
==8227==    by 0x407D97: main (retrace_main.cpp:880)
==8227==  Address 0x1604d964 is 4 bytes inside a block of size 480 free'd
==8227==    at 0x4C2BD2B: free (vg_replace_malloc.c:530)
==8227==    by 0x9C44F3D: dri_put_drawable.part.3 (dri_util.c:642)
==8227==    by 0x7741337: dri2DestroyDrawable (dri2_glx.c:343)
==8227==    by 0x773EEC9: driReleaseDrawables (dri_common.c:452)
==8227==    by 0x77425C1: dri2_bind_context (dri2_glx.c:142)
==8227==    by 0x771930B: MakeContextCurrent (glxcurrent.c:228)
==8227==    by 0x40A406: glws::makeCurrentInternal(glws::Drawable*, glws::Context*) (glws_glx.cpp:370)
==8227==    by 0x412C3E: makeCurrent (glws.hpp:213)
==8227==    by 0x412C3E: glretrace::makeCurrent(trace::Call&, glws::Drawable*, glretrace::Context*) (glretrace_ws.cpp:170)
==8227==    by 0x40C8BC: retrace::retraceCall(trace::Call*) (retrace_main.cpp:233)
==8227==    by 0x40CE2F: runLeg (retrace_main.cpp:386)
==8227==    by 0x40CE2F: runRace (retrace_main.cpp:364)
==8227==    by 0x40CE2F: retrace::RelayRace::run() (retrace_main.cpp:505)
==8227==    by 0x407D97: mainLoop (retrace_main.cpp:565)
==8227==    by 0x407D97: main (retrace_main.cpp:880)
==8227==  Block was alloc'd at
==8227==    at 0x4C2CB0D: calloc (vg_replace_malloc.c:711)
==8227==    by 0x9C46199: dri_create_buffer (dri_drawable.c:139)
==8227==    by 0x9C49D83: dri2_create_buffer (dri2.c:2196)
==8227==    by 0x9C450A3: driCreateNewDrawable (dri_util.c:671)
==8227==    by 0x774127C: dri2CreateDrawable (dri2_glx.c:405)
==8227==    by 0x773ED9F: driFetchDrawable (dri_common.c:410)
==8227==    by 0x77425A8: dri2_bind_context (dri2_glx.c:139)
==8227==    by 0x771930B: MakeContextCurrent (glxcurrent.c:228)
==8227==    by 0x40A406: glws::makeCurrentInternal(glws::Drawable*, glws::Context*) (glws_glx.cpp:370)
==8227==    by 0x412C3E: makeCurrent (glws.hpp:213)
==8227==    by 0x412C3E: glretrace::makeCurrent(trace::Call&, glws::Drawable*, glretrace::Context*) (glretrace_ws.cpp:170)
==8227==    by 0x40C8BC: retrace::retraceCall(trace::Call*) (retrace_main.cpp:233)
==8227==    by 0x40CE2F: runLeg (retrace_main.cpp:386)
==8227==    by 0x40CE2F: runRace (retrace_main.cpp:364)
==8227==    by 0x40CE2F: retrace::RelayRace::run() (retrace_main.cpp:505)
739: message: api issue 1: FBO incomplete: no attachments and default width or height is 0 [-1]
==8227== Conditional jump or move depends on uninitialised value(s)
==8227==    at 0x4C327D2: __memcmp_sse4_1 (vg_replace_strmem.c:1099)
==8227==    by 0x9F12F2F: r600_set_vertex_buffers (r600_state_common.c:550)
==8227==    by 0x9D4EDE0: u_vbuf_set_driver_vertex_buffers (u_vbuf.c:1116)
==8227==    by 0x9D52394: u_vbuf_draw_vbo (u_vbuf.c:1140)
==8227==    by 0x9A6018B: st_draw_vbo (st_draw.c:222)
==8227==    by 0x9A0A379: vbo_validated_drawrangeelements (vbo_exec_array.c:918)
==8227==    by 0x9A0AB05: vbo_exec_DrawRangeElementsBaseVertex (vbo_exec_array.c:1019)
==8227==    by 0x9A0AD6A: vbo_exec_DrawRangeElements (vbo_exec_array.c:1039)
==8227==    by 0x9938B6F: _mesa_unmarshal_DrawRangeElements (marshal_generated.c:21699)
==8227==    by 0x9938B6F: _mesa_unmarshal_dispatch_cmd (marshal_generated.c:41346)
==8227==    by 0x98ED96C: glthread_unmarshal_batch (glthread.c:53)
==8227==    by 0x98EDC54: _mesa_glthread_finish (glthread.c:209)
==8227==    by 0x98FF573: _mesa_marshal_GetError (marshal_generated.c:12286)
==8227== 
==8227== Conditional jump or move depends on uninitialised value(s)
==8227==    at 0x9F15A4D: r600_draw_vbo (r600_state_common.c:1806)
==8227==    by 0x9D521DB: u_vbuf_draw_vbo (u_vbuf.c:1143)
==8227==    by 0x9A6018B: st_draw_vbo (st_draw.c:222)
==8227==    by 0x9A0A379: vbo_validated_drawrangeelements (vbo_exec_array.c:918)
==8227==    by 0x9A0AB05: vbo_exec_DrawRangeElementsBaseVertex (vbo_exec_array.c:1019)
==8227==    by 0x9A0AD6A: vbo_exec_DrawRangeElements (vbo_exec_array.c:1039)
==8227==    by 0x9938B6F: _mesa_unmarshal_DrawRangeElements (marshal_generated.c:21699)
==8227==    by 0x9938B6F: _mesa_unmarshal_dispatch_cmd (marshal_generated.c:41346)
==8227==    by 0x98ED96C: glthread_unmarshal_batch (glthread.c:53)
==8227==    by 0x98EDC54: _mesa_glthread_finish (glthread.c:209)
==8227==    by 0x98FF573: _mesa_marshal_GetError (marshal_generated.c:12286)
==8227==    by 0x412530: glretrace::checkGlError(trace::Call&) (glretrace_main.cpp:94)
==8227==    by 0x4C2256: retrace_glDrawRangeElements(trace::Call&) (glretrace_gl.cpp:10574)
==8227== 
==8227== Conditional jump or move depends on uninitialised value(s)
==8227==    at 0x9F15A4D: r600_draw_vbo (r600_state_common.c:1806)
==8227==    by 0x9D521DB: u_vbuf_draw_vbo (u_vbuf.c:1143)
==8227==    by 0x9A6018B: st_draw_vbo (st_draw.c:222)
==8227==    by 0x9A0986F: vbo_draw_arrays (vbo_exec_array.c:486)
==8227==    by 0x9A09DE9: vbo_exec_DrawArrays (vbo_exec_array.c:641)
==8227==    by 0x993476D: _mesa_unmarshal_DrawArrays (marshal_generated.c:26211)
==8227==    by 0x993476D: _mesa_unmarshal_dispatch_cmd (marshal_generated.c:41754)
==8227==    by 0x98ED96C: glthread_unmarshal_batch (glthread.c:53)
==8227==    by 0x98EDC54: _mesa_glthread_finish (glthread.c:209)
==8227==    by 0x98FF573: _mesa_marshal_GetError (marshal_generated.c:12286)
==8227==    by 0x412530: glretrace::checkGlError(trace::Call&) (glretrace_main.cpp:94)
==8227==    by 0x4C51FE: retrace_glDrawArrays(trace::Call&) (glretrace_gl.cpp:9435)
==8227==    by 0x40C8BC: retrace::retraceCall(trace::Call*) (retrace_main.cpp:233)
==8227== 
==8227== Invalid read of size 8
==8227==    at 0x5C134E: retrace_glXMakeContextCurrent(trace::Call&) (glretrace_glx.cpp:194)
==8227==    by 0x40C8BC: retrace::retraceCall(trace::Call*) (retrace_main.cpp:233)
==8227==    by 0x40CE2F: runLeg (retrace_main.cpp:386)
==8227==    by 0x40CE2F: runRace (retrace_main.cpp:364)
==8227==    by 0x40CE2F: retrace::RelayRace::run() (retrace_main.cpp:505)
==8227==    by 0x407D97: mainLoop (retrace_main.cpp:565)
==8227==    by 0x407D97: main (retrace_main.cpp:880)
==8227==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==8227== 
apitrace: warning: caught signal 11
4128: error: caught an unhandled exception
glretrace+0x239054
/lib64/libpthread.so.0+0x10bbf
glretrace+0x1c134e
glretrace+0xc8bc
glretrace+0xce2f
glretrace+0x7d97
/lib64/libc.so.6: __libc_start_main+0xef
glretrace: _start+0x28
?
apitrace: info: taking default action for signal 11
==8227== 
==8227== Process terminating with default action of signal 11 (SIGSEGV)
==8227==    at 0x518AA79: raise (pt-raise.c:35)
==8227==    by 0x63912B: os::signalHandler(int, siginfo_t*, void*) (os_posix.cpp:357)
==8227==    by 0x518ABBF: ??? (in /lib64/libpthread-2.23.so)
==8227==    by 0x5C134D: retrace_glXMakeContextCurrent(trace::Call&) (glretrace_glx.cpp:194)
==8227==    by 0x40C8BC: retrace::retraceCall(trace::Call*) (retrace_main.cpp:233)
==8227==    by 0x40CE2F: runLeg (retrace_main.cpp:386)
==8227==    by 0x40CE2F: runRace (retrace_main.cpp:364)
==8227==    by 0x40CE2F: retrace::RelayRace::run() (retrace_main.cpp:505)
==8227==    by 0x407D97: mainLoop (retrace_main.cpp:565)
==8227==    by 0x407D97: main (retrace_main.cpp:880)
==8227== 
==8227== HEAP SUMMARY:
==8227==     in use at exit: 4,826,139 bytes in 12,695 blocks
==8227==   total heap usage: 55,166 allocs, 42,471 frees, 17,699,948 bytes allocated
==8227== 
==8227== LEAK SUMMARY:
==8227==    definitely lost: 20,160 bytes in 3 blocks
==8227==    indirectly lost: 0 bytes in 0 blocks
==8227==      possibly lost: 112,184 bytes in 745 blocks
==8227==    still reachable: 4,693,795 bytes in 11,947 blocks
==8227==         suppressed: 0 bytes in 0 blocks
==8227== Rerun with --leak-check=full to see details of leaked memory
==8227== 
==8227== For counts of detected and suppressed errors, rerun with: -v
==8227== Use --track-origins=yes to see where uninitialised values come from
==8227== ERROR SUMMARY: 35 errors from 5 contexts (suppressed: 0 from 0)
Comment 4 charmainel 2017-07-18 19:34:10 UTC
Created attachment 132747 [details] [review]
Assign a unique ID to the framebuffer interface object created in xm_st.c
Comment 5 charmainel 2017-07-18 19:36:16 UTC
Hi Brad,

Can you try the attached patch to see if that fixes your crash?
Thanks.
Comment 6 Gert Wollny 2017-07-19 01:40:44 UTC
The patch didn't help on my side. 

I've added some debug output to see what is going on. In summary, stfb->iface in st_manager.c  is not properly updated and points  to a destroyed buffer, that is accessed in st_framebuffers_purge, see below:

valgrind  glretrace Downloads/example.trace 
==3152== Memcheck, a memory error detector
==3152== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==3152== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==3152== Command: glretrace Downloads/example.trace
==3152== 
dri_get_drawable: 0x161220b0 refcount: 1
dri_create_buffer 0x161220b0: 0x16122130
driCreateNewDrawable: 0x161220b0 refcount: 1 buffer: 0x16122130
driFetchDrawable (create): 0x16121fa0 refcount: 1
driFetchDrawable: 0x16121fa0 refcount: 2
dri_get_drawable: 0x161220b0 refcount: 2
Bind context 0x16036270 pdp 0x161220b0 prp 0x161220b0 
stdraw= 0x16122520
stread= 0x16122520
st= 0x160ecc10
stfb->iface= 0x16122130
dri_put_drawable: 0x161220b0 refcount: 1
Unbind context 0x16036270 pdp 0x161220b0 prp 0x161220b0 
dri_get_drawable: 0x162791f0 refcount: 1

dri_create_buffer 0x162791f0: 0x16279270 <==== create new buffer (2) 

driCreateNewDrawable: 0x162791f0 refcount: 1 buffer: 0x16279270
driFetchDrawable (create): 0x162790e0 refcount: 1
driFetchDrawable: 0x162790e0 refcount: 2
dri_get_drawable: 0x162791f0 refcount: 2
Bind context 0x16196340 pdp 0x162791f0 prp 0x162791f0 
stdraw= 0x16279550
stread= 0x16279550
st= 0x16246250

in st_framebuffers_purge; 
stfb->iface= 0x16279270     <============================= first use 


dri_put_drawable: 0x162791f0 refcount: 1
Unbind context 0x16196340 pdp 0x162791f0 prp 0x162791f0 
dri_get_drawable: 0x1628b1c0 refcount: 1

dri_create_buffer 0x1628b1c0: 0x1628b240   <==== create new buffer (3)

driCreateNewDrawable: 0x1628b1c0 refcount: 1 buffer: 0x1628b240
driFetchDrawable: 0x1628b0b0 refcount: 1
driFetchDrawable: 0x1628b0b0 refcount: 2
driReleaseDrawables; Drawable: 0x162790e0 refcount: 2
driReleaseDrawables; Readable: 0x162790e0 refcount: 1
dri2DestroyDrawable 0x162791f0
driDestroyDrawable: 0x162791f0 refcount: 1
dri_put_drawable: 0x162791f0 refcount: 0
   --- Destroy

dri_destroy_buffer 0x162791f0: 0x16279270      <====== buffer (2) destroyed 


dri_get_drawable: 0x1628b1c0 refcount: 2
Bind context 0x16196340 pdp 0x1628b1c0 prp 0x1628b1c0 
stdraw= 0x1628d770
stread= 0x1628d770
st= 0x16246250

stfb->iface= 0x16279270     <====== still pointing to the destroyed buffer 
                                    should have been updated to (3) 0x1628b240

==3152== Invalid read of size 4
==3152==    at 0x9CC5D04: st_framebuffers_purge (st_manager.c:510)
==3152==    by 0x9CC5D04: st_api_make_current (st_manager.c:876)
==3152==    by 0x9E709CD: dri_make_current (dri_context.c:278)
Comment 7 charmainel 2017-07-19 05:15:45 UTC
Hi Gert,

The original patch was indeed accessing the dangling pointers.
I'll have a patch tomorrow to fix the problem.
Thanks for looking into it.
Comment 8 Emil Velikov 2017-07-25 12:04:14 UTC
Should be fixed with

commit 5124bf982393114862f44ee62fa361027faa7c29
Author: Charmaine Lee <charmainel@vmware.com>
Date:   Thu Jul 20 11:04:14 2017 -0700

    st/mesa: add destroy_drawable interface


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.