Bug 65173 - segfault in _mesa_get_format_datatype and _mesa_get_color_read_type when state dumping with glretrace
Summary: segfault in _mesa_get_format_datatype and _mesa_get_color_read_type when stat...
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Mesa core (show other bugs)
Version: git
Hardware: All All
: medium normal
Assignee: mesa-dev
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-30 12:10 UTC by Vedran Rodic
Modified: 2013-06-03 00:12 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
fix (1.28 KB, text/plain)
2013-05-30 12:10 UTC, Vedran Rodic
Details
gdb full backtrace of the crash (3.96 KB, text/plain)
2013-05-30 12:11 UTC, Vedran Rodic
Details
fix-v2 (915 bytes, text/plain)
2013-05-30 21:51 UTC, Vedran Rodic
Details
patch which generates GL_INVALID_ENUM (3.07 KB, patch)
2013-05-31 14:56 UTC, Brian Paul
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Vedran Rodic 2013-05-30 12:10:37 UTC
Created attachment 80036 [details]
fix

when using glretrace to dump the GL state, sometimes _mesa_get_color_read_format and _mesa_get_color_read_type can access uninitialized ctx->ReadBuffer->_ColorReadBuffer

To reproduce run:

glretrace -D 137078 wine-preloader.137078.trace

trace file is here: 
http://mjesec.ffzg.hr/~vrodic/dota/wine-preloader.137078.trim.trace.bz2

I've attached a sample patch to fix the bug (probably incorrect) and a gdb backtrace .
Comment 1 Vedran Rodic 2013-05-30 12:11:10 UTC
Created attachment 80037 [details]
gdb full backtrace of the crash
Comment 2 Vedran Rodic 2013-05-30 12:13:46 UTC
nVidia proprietary driver doesn't crash with the same command.
Comment 3 Brian Paul 2013-05-30 15:09:15 UTC
Which version of Mesa, and which driver?

I tested with Mesa/master and the 9.1 branch and didn't see this problem.

However, I did find a problem when the libtxc_dxtn.so library wasn't present.  I'll post a patch for that to the mesa-dev list.
Comment 4 Vedran Rodic 2013-05-30 16:16:12 UTC
mesa git version, i'll check if libtxc_dxtn.so was present here

On Thu, May 30, 2013 at 5:09 PM,  <bugzilla-daemon@freedesktop.org> wrote:
> Comment # 3 on bug 65173 from Brian Paul
>
> Which version of Mesa, and which driver?
>
> I tested with Mesa/master and the 9.1 branch and didn't see this problem.
>
> However, I did find a problem when the libtxc_dxtn.so library wasn't
> present.
> I'll post a patch for that to the mesa-dev list.
>
> ________________________________
> You are receiving this mail because:
>
> You are the assignee for the bug.
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
Comment 5 Vedran Rodic 2013-05-30 17:01:19 UTC
I'm using glretrace from latest apitrace git, mesa from latest git.

I was able to reproduce this on nouveau driver, intel driver and gallium swrast driver.
Comment 6 Brian Paul 2013-05-30 17:12:07 UTC
Hmm, could we get another person (Jose?) to try this too?
Comment 7 Jose Fonseca 2013-05-30 21:35:01 UTC
(In reply to comment #6)
> Hmm, could we get another person (Jose?) to try this too?

Yep, it happens here too. ctx->ReadBuffer->_ColorReadBuffer is NULL in _mesa_get_color_read_type. This is the full stack backtrace (this time debug build):

Program received signal SIGSEGV, Segmentation fault.
_mesa_get_color_read_type (ctx=0xbed310) at src/mesa/main/framebuffer.c:905
905	   const GLenum data_type = _mesa_get_format_datatype(
(gdb) bt
#0  _mesa_get_color_read_type (ctx=0xbed310) at src/mesa/main/framebuffer.c:905
#1  0x00007ffff4ed67f8 in find_custom_value (ctx=0xbed310, d=0x7ffff608ca88, v=0x7fffffffb550) at src/mesa/main/get.c:696
#2  0x00007ffff4ed75ef in find_value (func=0x7ffff5a0328c "glGetIntegerv", pname=35738, p=0x7fffffffb548, v=0x7fffffffb550)
    at src/mesa/main/get.c:1099
#3  0x00007ffff4ed7d69 in _mesa_GetIntegerv (pname=35738, params=0x7fffffffce88) at src/mesa/main/get.c:1299
#4  0x000000000043255a in _glGetIntegerv (pname=35738, params=0x7fffffffce88)
    at /home/jfonseca/projects/apitrace/dispatch/glproc.hpp:4802
#5  0x000000000045bb28 in glstate::dumpParameters (json=..., context=...)
    at /home/jfonseca/projects/apitrace/retrace/glstate_params.cpp:14881
#6  0x000000000042e3ee in glstate::dumpCurrentContext (os=...) at /home/jfonseca/projects/apitrace/retrace/glstate.cpp:169
#7  0x000000000042c86a in GLDumper::dumpState (this=0x909d10, os=...)
    at /home/jfonseca/projects/apitrace/retrace/glretrace_main.cpp:445
#8  0x000000000041fb9d in retrace::retraceCall (call=0xbd8f40) at /home/jfonseca/projects/apitrace/retrace/retrace_main.cpp:197
#9  0x00000000004219ec in retrace::RelayRunner::runLeg (this=0x98a720, call=0xbd8f40)
    at /home/jfonseca/projects/apitrace/retrace/retrace_main.cpp:342
#10 0x00000000004218d2 in retrace::RelayRunner::runRace (this=0x98a720)
    at /home/jfonseca/projects/apitrace/retrace/retrace_main.cpp:313
#11 0x000000000041fe9a in retrace::RelayRace::run (this=0x7fffffffde30)
    at /home/jfonseca/projects/apitrace/retrace/retrace_main.cpp:480
#12 0x0000000000420072 in retrace::mainLoop () at /home/jfonseca/projects/apitrace/retrace/retrace_main.cpp:541
#13 0x0000000000420879 in main (argc=4, argv=0x7fffffffe068) at /home/jfonseca/projects/apitrace/retrace/retrace_main.cpp:804

The proposed patch fixes it.

I don't know if the ctx->ReadBuffer check is necessary but I suppose it doesn't hurt.

In summary: looks good to me.
Comment 8 Vedran Rodic 2013-05-30 21:51:51 UTC
Created attachment 80070 [details]
fix-v2

I'm happier with this new version of the patch, but I'm still not sure if returning 0 is a correct thing to do.
Comment 9 Jose Fonseca 2013-05-30 22:09:58 UTC
(In reply to comment #8)
> Created attachment 80070 [details]
> fix-v2
> 
> I'm happier with this new version of the patch, 

I'm afraid that version 2 assumes C99 and will break MSVC.

> but I'm still not sure if
> returning 0 is a correct thing to do.

Anything is better than crashing.

GL_READ_BUFFER is GL_NONE here, so it doesn't matter.

What does nVidia proprietary driver return for IMPLEMENTATION_COLOR_READ_{FORMAT,TYPE} on that call? That is, what is the output of

   glretrace -D 137078 wine-preloader.137078.trim.trace | grep IMPLEMENTATION_COLOR_READ

on nvidia?
Comment 10 Vedran Rodic 2013-05-30 22:28:44 UTC
>What does nVidia proprietary driver return for 
>IMPLEMENTATION_COLOR_READ_{FORMAT,TYPE} on that call? That is, what is the 
>output of
> glretrace -D 137078 wine-preloader.137078.trim.trace | grep 
>IMPLEMENTATION_COLOR_READ

Nothing, the part of the state dump where mesa version crashes looks like this (I've put stderr part in <stderr> tags):

 "GL_MAX_VERTEX_UNIFORM_COMPONENTS": 4096,
    "GL_MAX_VARYING_COMPONENTS": 60,
    "GL_MAX_VERTEX_TEXTURE_IMAGE_UNITS": 32,
    "GL_MAX_COMBINED_TEXTURE_IMAGE_UNITS": 96,
    "GL_SHADING_LANGUAGE_VERSION": "3.30 NVIDIA via Cg compiler",
    "GL_CURRENT_PROGRAM": 18 <stderr>137078: glDebugOutputCallback: High severity API error 1282, GL_INVALID_OPERATION error generated. Invalid query on incomplete or unselected read framebuffer
137078: glDebugOutputCallback: High severity API error 1282, GL_INVALID_OPERATION error generated. Invalid query on incomplete or unselected read framebuffer</stderr>
,
    "GL_MAX_GEOMETRY_TEXTURE_IMAGE_UNITS": 32,
    "GL_TEXTURE_BUFFER": 0,
Comment 11 Jose Fonseca 2013-05-31 06:35:43 UTC
(In reply to comment #10)
> >What does nVidia proprietary driver return for 
> >IMPLEMENTATION_COLOR_READ_{FORMAT,TYPE} on that call? That is, what is the 
> >output of
> > glretrace -D 137078 wine-preloader.137078.trim.trace | grep 
> >IMPLEMENTATION_COLOR_READ
> 
> Nothing, the part of the state dump where mesa version crashes looks like
> this (I've put stderr part in <stderr> tags):
> 
>  "GL_MAX_VERTEX_UNIFORM_COMPONENTS": 4096,
>     "GL_MAX_VARYING_COMPONENTS": 60,
>     "GL_MAX_VERTEX_TEXTURE_IMAGE_UNITS": 32,
>     "GL_MAX_COMBINED_TEXTURE_IMAGE_UNITS": 96,
>     "GL_SHADING_LANGUAGE_VERSION": "3.30 NVIDIA via Cg compiler",
>     "GL_CURRENT_PROGRAM": 18 <stderr>137078: glDebugOutputCallback: High
> severity API error 1282, GL_INVALID_OPERATION error generated. Invalid query
> on incomplete or unselected read framebuffer
> 137078: glDebugOutputCallback: High severity API error 1282,
> GL_INVALID_OPERATION error generated. Invalid query on incomplete or
> unselected read framebuffer</stderr>
> ,
>     "GL_MAX_GEOMETRY_TEXTURE_IMAGE_UNITS": 32,
>     "GL_TEXTURE_BUFFER": 0,

Thanks for checking. It seems it nvidia driver emits a GL_INVALID_OPERATION error.  We could do the same.

BTW, I did read the relevant specs for this query, but they don't really describe what happens in this situation.
Comment 12 Brian Paul 2013-05-31 14:56:50 UTC
Created attachment 80099 [details] [review]
patch which generates GL_INVALID_ENUM

How about this patch?  It generates GL_INVALID_OPERATION if there's no color read buffer.  It also cleans up both functions in general.
Comment 13 Jose Fonseca 2013-05-31 16:07:26 UTC
(In reply to comment #12)
> How about this patch?  It generates GL_INVALID_OPERATION if there's no color
> read buffer.  It also cleans up both functions in general.

Patch looks good to me!
Comment 14 Vedran Rodic 2013-06-02 10:49:27 UTC
If you were waiting for me to confirm the bug fixed, here's the confirmation :)
Comment 15 Brian Paul 2013-06-03 00:12:39 UTC
Thanks for testing.  Pushed as commit e20a2df4017ab10dd7199936948c6ac809bfacb6


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.