Last Comment Bug 785734 - (CVE-2012-5833) Mesa crashes on certain texImage2D calls involving level>0
(CVE-2012-5833)
: Mesa crashes on certain texImage2D calls involving level>0
Status: VERIFIED FIXED
[asan][adv-track-main17+][adv-track-e...
: sec-critical, sec-vector
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: mozilla19
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-26 11:23 PDT by miaubiz
Modified: 2014-07-24 13:43 PDT (History)
20 users (show)
rforbes: sec‑bounty+
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
affected
+
verified
+
verified
verified
17+
verified
verified


Attachments
repro (350 bytes, text/plain)
2012-08-26 11:23 PDT, miaubiz
no flags Details
asan log linux (902 bytes, text/plain)
2012-08-26 11:25 PDT, miaubiz
no flags Details
chromium asan log on linux (3.65 KB, text/plain)
2012-08-26 11:25 PDT, miaubiz
no flags Details
minifuzzer to look for possible values (808 bytes, text/plain)
2012-08-28 00:38 PDT, miaubiz
no flags Details
work around the mesa teximage2d crasher by rejecting certain legal, but rare, teximage2d calls (1.32 KB, patch)
2012-08-28 12:30 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
Details | Diff | Review
variant testcase with copyTexImage2D (327 bytes, text/html)
2012-09-13 13:03 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details
new testcase showing a crash even though all mipmap levels were defined (864 bytes, text/html)
2012-09-14 07:18 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details
work around the mesa teximage2d crasher by rejecting certain legal, but rare, teximage2d and copyTexImage2d calls (3.96 KB, patch)
2012-09-14 07:47 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review-
Details | Diff | Review
work around the mesa teximage2d crasher by rejecting certain legal, but rare, teximage2d and copyTexImage2d calls (3.96 KB, patch)
2012-09-21 12:46 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Review
correct patch (Gregg's idea) (2.11 KB, patch)
2012-10-05 18:57 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Review
correct patch (Gregg's idea) (3.84 KB, patch)
2012-10-05 23:02 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review
esr10 patch (2.29 KB, patch)
2012-10-26 11:16 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
akeybl: approval‑mozilla‑esr10+
Details | Diff | Review

Description miaubiz 2012-08-26 11:23:58 PDT
Created attachment 655441 [details]
repro

<html>
  <head>
    <script>
      var gl = document.createElement('canvas').getContext('experimental-webgl')
      var texture = gl.createTexture()
      gl.bindTexture(gl.TEXTURE_2D, texture)
      gl.texImage2D(gl.TEXTURE_2D, 6, gl.RGBA, 512, 2, 0, gl.RGBA, gl.UNSIGNED_BYTE, null)
      gl.deleteTexture(texture)
    </script>
  </head>
</html>

crashes on linux with mesa like:

==32234== ERROR: AddressSanitizer crashed on unknown address 0x000100000011 (pc 0x7fffc5321690 sp 0x7fffffff4988 bp 0x000000000006 T0)
AddressSanitizer can not provide additional info. ABORTING
    #0 0x7fffc5321690 in ?? ??:0

also crashes chrome
Comment 2 miaubiz 2012-08-26 11:25:09 PDT
Created attachment 655442 [details]
asan log linux
Comment 3 miaubiz 2012-08-26 11:25:40 PDT
Created attachment 655443 [details]
chromium asan log on linux
Comment 4 Benoit Jacob [:bjacob] (mostly away) 2012-08-27 13:36:54 PDT
I a debug build of Mesa, I get an assertion failure:

src/gallium/drivers/llvmpipe/lp_texture.c:395:llvmpipe_resource_map: Assertion `level == 0' failed.

Here, the value of level is 6 instead of the expected 0.


Program received signal SIGTRAP, Trace/breakpoint trap.
0x00007fffcc5bfb92 in _debug_assert_fail (expr=0x7fffccd9d3b0 "level == 0", file=0x7fffccd9d200 "src/gallium/drivers/llvmpipe/lp_texture.c", line=395, function=0x7fffccd9db30 "llvmpipe_resource_map")
    at src/gallium/auxiliary/util/u_debug.c:278
278           os_abort();
(gdb) bt
#0  0x00007fffcc5bfb92 in _debug_assert_fail (expr=0x7fffccd9d3b0 "level == 0", file=0x7fffccd9d200 "src/gallium/drivers/llvmpipe/lp_texture.c", line=395, function=0x7fffccd9db30 "llvmpipe_resource_map")
    at src/gallium/auxiliary/util/u_debug.c:278
#1  0x00007fffcc26a985 in llvmpipe_resource_map (resource=0x7fffd3bca000, level=6, layer=0, tex_usage=LP_TEX_USAGE_READ_WRITE, layout=LP_TEX_LAYOUT_LINEAR) at src/gallium/drivers/llvmpipe/lp_texture.c:395
#2  0x00007fffcc26b39d in llvmpipe_transfer_map (pipe=0x7fffde04c000, transfer=0x7fffde06bc00) at src/gallium/drivers/llvmpipe/lp_texture.c:699
#3  0x00007fffcc417088 in pipe_transfer_map (context=0x7fffde04c000, transfer=0x7fffde06bc00) at src/gallium/auxiliary/util/u_inlines.h:428
#4  0x00007fffcc417715 in st_texture_image_map (st=0x7fffd4e67000, stImage=0x7fffd44f3320, zoffset=0, usage=258, x=0, y=0, w=512, h=2) at src/mesa/state_tracker/st_texture.c:238
#5  0x00007fffcc40d76e in st_MapTextureImage (ctx=0x7fffd3a6d000, texImage=0x7fffd44f3320, slice=0, x=0, y=0, w=512, h=2, mode=6, mapOut=0x7fffffff8c08, rowStrideOut=0x7fffffff8c1c) at src/mesa/state_tracker/st_cb_texture.c:187
#6  0x00007fffcc3cafe3 in store_texsubimage (ctx=0x7fffd3a6d000, texImage=0x7fffd44f3320, xoffset=0, yoffset=0, zoffset=0, width=512, height=2, depth=1, format=6408, type=5121, pixels=0x7fffdda7d000, packing=0x7fffd3a73888, 
    caller=0x7fffccdec94a "glTexImage") at src/mesa/main/texstore.c:4352
#7  0x00007fffcc3cb206 in _mesa_store_teximage (ctx=0x7fffd3a6d000, dims=2, texImage=0x7fffd44f3320, format=6408, type=5121, pixels=0x7fffdda7d000, packing=0x7fffd3a73888) at src/mesa/main/texstore.c:4408
#8  0x00007fffcc40e13d in st_TexImage (ctx=0x7fffd3a6d000, dims=2, texImage=0x7fffd44f3320, format=6408, type=5121, pixels=0x7fffdda7d000, unpack=0x7fffd3a73888) at src/mesa/state_tracker/st_cb_texture.c:537
#9  0x00007fffcc3b6183 in teximage (ctx=0x7fffd3a6d000, dims=2, target=3553, level=6, internalFormat=6408, width=512, height=2, depth=1, border=0, format=6408, type=5121, pixels=0x7fffdda7d000) at src/mesa/main/teximage.c:2623
#10 0x00007fffcc3b6363 in _mesa_TexImage2D (target=3553, level=6, internalFormat=6408, width=512, height=2, border=0, format=6408, type=5121, pixels=0x7fffdda7d000) at src/mesa/main/teximage.c:2663
#11 0x00007ffff365a06f in mozilla::gl::GLContext::fTexImage2D (this=0x7fffd3a50000, target=3553, level=6, internalformat=6408, width=512, height=2, border=0, format=6408, type=5121, pixels=0x7fffdda7d000)
    at ../../../dist/include/GLContext.h:2672
#12 0x00007ffff3672ced in mozilla::WebGLContext::CheckedTexImage2D (this=0x7fffce86d400, target=3553, level=6, internalFormat=6408, width=512, height=2, border=0, format=6408, type=5121, data=0x7fffdda7d000)
    at /hack/mozilla-inbound/content/canvas/src/WebGLContextGL.cpp:5604
#13 0x00007ffff3673465 in mozilla::WebGLContext::TexImage2D_base (this=0x7fffce86d400, target=3553, level=6, internalformat=6408, width=512, height=2, srcStrideOrZero=0, border=0, format=6408, type=5121, data=0x0, byteLength=0, 
    jsArrayType=-1, srcFormat=mozilla::WebGLTexelConversions::Auto, srcPremultiplied=false) at /hack/mozilla-inbound/content/canvas/src/WebGLContextGL.cpp:5745
#14 0x00007ffff36736d1 in mozilla::WebGLContext::TexImage2D (this=0x7fffce86d400, cx=0x7fffd4394400, target=3553, level=6, internalformat=6408, width=512, height=2, border=0, format=6408, type=5121, pixels=0x0, rv=...)
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2012-08-27 13:41:10 PDT
Replacing 6 by 0 in the testcases avoids the crash, so this is specifically about setting a mipmap level >0 on a newly created texture.
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2012-08-27 13:48:12 PDT
Crash confirmed on Mesa/Intel driver on another machine, so it's not just LLVMpipe.
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2012-08-27 14:05:48 PDT
Obviously the fact that the Mesa debug build asserts means it can't be used to assess how security critical this is.

In a release build of mesa, in valgrind, I get an invalid write, followed by an invalid read, and then the crash:

==26671== Invalid write of size 8
==26671==    at 0x2C92A723: llvmpipe_resource_create (lp_texture.c:165)
==26671==    by 0x2CA8E71B: st_texture_create (st_texture.c:94)
==26671==    by 0x2CA879B0: guess_and_alloc_texture (st_cb_texture.c:393)
==26671==    by 0x2CA89220: st_AllocTextureImageBuffer (st_cb_texture.c:447)
==26671==    by 0x2CA52461: _mesa_store_teximage (texstore.c:4403)
==26671==    by 0x2CA3C07E: teximage (teximage.c:2623)
==26671==    by 0x2CA3D2E7: _mesa_TexImage2D (teximage.c:2663)
==26671==    by 0x72E306E: mozilla::gl::GLContext::fTexImage2D(unsigned int, int, int, int, int, int, unsigned int, unsigned int, void const*) (GLContext.h:2672)
==26671==    by 0x72FBCEC: mozilla::WebGLContext::CheckedTexImage2D(unsigned int, int, unsigned int, int, int, int, unsigned int, unsigned int, void const*) (WebGLContextGL.cpp:5604)
==26671==    by 0x72FC464: mozilla::WebGLContext::TexImage2D_base(unsigned int, int, unsigned int, int, int, int, int, unsigned int, unsigned int, void*, unsigned int, int, mozilla::WebGLTexelConversions::WebGLTexelFormat, bool) (WebGLContextGL.cpp:5745)
==26671==    by 0x72FC6D0: mozilla::WebGLContext::TexImage2D(JSContext*, unsigned int, int, unsigned int, int, int, int, unsigned int, unsigned int, mozilla::dom::TypedArray_base<unsigned char, &(JS_GetObjectAsArrayBufferView(JSContext*, JSObject*, unsigned int*, unsigned char**))>*, mozilla::ErrorResult&) (WebGLContextGL.cpp:5789)
==26671==    by 0x83B6645: mozilla::dom::WebGLRenderingContextBinding::texImage2D(JSContext*, JS::Handle<JSObject*>, mozilla::WebGLContext*, unsigned int, JS::Value*) (WebGLRenderingContextBinding.cpp:4307)
==26671==  Address 0x1a93b7d8 is 0 bytes after a block of size 664 alloc'd
==26671==    at 0x402B7FA: calloc (vg_replace_malloc.c:591)
==26671==    by 0x2C92A352: llvmpipe_resource_create (lp_texture.c:244)
==26671==    by 0x2CA8E71B: st_texture_create (st_texture.c:94)
==26671==    by 0x2CA879B0: guess_and_alloc_texture (st_cb_texture.c:393)
==26671==    by 0x2CA89220: st_AllocTextureImageBuffer (st_cb_texture.c:447)
==26671==    by 0x2CA52461: _mesa_store_teximage (texstore.c:4403)
==26671==    by 0x2CA3C07E: teximage (teximage.c:2623)
==26671==    by 0x2CA3D2E7: _mesa_TexImage2D (teximage.c:2663)
==26671==    by 0x72E306E: mozilla::gl::GLContext::fTexImage2D(unsigned int, int, int, int, int, int, unsigned int, unsigned int, void const*) (GLContext.h:2672)
==26671==    by 0x72FBCEC: mozilla::WebGLContext::CheckedTexImage2D(unsigned int, int, unsigned int, int, int, int, unsigned int, unsigned int, void const*) (WebGLContextGL.cpp:5604)
==26671==    by 0x72FC464: mozilla::WebGLContext::TexImage2D_base(unsigned int, int, unsigned int, int, int, int, int, unsigned int, unsigned int, void*, unsigned int, int, mozilla::WebGLTexelConversions::WebGLTexelFormat, bool) (WebGLContextGL.cpp:5745)
==26671==    by 0x72FC6D0: mozilla::WebGLContext::TexImage2D(JSContext*, unsigned int, int, unsigned int, int, int, int, unsigned int, unsigned int, mozilla::dom::TypedArray_base<unsigned char, &(JS_GetObjectAsArrayBufferView(JSContext*, JSObject*, unsigned int*, unsigned char**))>*, mozilla::ErrorResult&) (WebGLContextGL.cpp:5789)
==26671== 
==26671== Invalid read of size 8
==26671==    at 0x2C9940D0: xlib_displaytarget_map (xlib_sw_winsys.c:231)
==26671==    by 0x2C92B0F4: llvmpipe_transfer_map (lp_texture.c:399)
==26671==    by 0x2CA8E961: st_texture_image_map (u_inlines.h:428)
==26671==    by 0x2CA875CC: st_MapTextureImage (st_cb_texture.c:187)
==26671==    by 0x2CA472DE: store_texsubimage (texstore.c:4352)
==26671==    by 0x2CA524B7: _mesa_store_teximage (texstore.c:4408)
==26671==    by 0x2CA3C07E: teximage (teximage.c:2623)
==26671==    by 0x2CA3D2E7: _mesa_TexImage2D (teximage.c:2663)
==26671==    by 0x72E306E: mozilla::gl::GLContext::fTexImage2D(unsigned int, int, int, int, int, int, unsigned int, unsigned int, void const*) (GLContext.h:2672)
==26671==    by 0x72FBCEC: mozilla::WebGLContext::CheckedTexImage2D(unsigned int, int, unsigned int, int, int, int, unsigned int, unsigned int, void const*) (WebGLContextGL.cpp:5604)
==26671==    by 0x72FC464: mozilla::WebGLContext::TexImage2D_base(unsigned int, int, unsigned int, int, int, int, int, unsigned int, unsigned int, void*, unsigned int, int, mozilla::WebGLTexelConversions::WebGLTexelFormat, bool) (WebGLContextGL.cpp:5745)
==26671==    by 0x72FC6D0: mozilla::WebGLContext::TexImage2D(JSContext*, unsigned int, int, unsigned int, int, int, int, unsigned int, unsigned int, mozilla::dom::TypedArray_base<unsigned char, &(JS_GetObjectAsArrayBufferView(JSContext*, JSObject*, unsigned int*, unsigned char**))>*, mozilla::ErrorResult&) (WebGLContextGL.cpp:5789)
==26671==  Address 0x100000011 is not stack'd, malloc'd or (recently) free'd
==26671==
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2012-08-27 14:20:21 PDT
If I comment out the assert that we hit in comment 4, I get a crash in the debug build at the same location as the invalid read crash in comment 7, with some information:

Program received signal SIGSEGV, Segmentation fault.
0x00007fffcf216b14 in xlib_displaytarget_map (ws=0x7fffd3eb0ab0, dt=0x100000001, flags=3) at src/gallium/winsys/sw/xlib/xlib_sw_winsys.c:231
231        xlib_dt->mapped = xlib_dt->data;

(gdb) bt
#0  0x00007fffcf216b14 in xlib_displaytarget_map (ws=0x7fffd3eb0ab0, dt=0x100000001, flags=3) at src/gallium/winsys/sw/xlib/xlib_sw_winsys.c:231
#1  0x00007fffcf1899a8 in llvmpipe_resource_map (resource=0x7fffddf37800, level=6, layer=0, tex_usage=LP_TEX_USAGE_READ_WRITE, layout=LP_TEX_LAYOUT_LINEAR) at src/gallium/drivers/llvmpipe/lp_texture.c:399
#2  0x00007fffcf18a378 in llvmpipe_transfer_map (pipe=0x7fffd4913000, transfer=0x7fffd48e1740) at src/gallium/drivers/llvmpipe/lp_texture.c:699
#3  0x00007fffcf336068 in pipe_transfer_map (context=0x7fffd4913000, transfer=0x7fffd48e1740) at src/gallium/auxiliary/util/u_inlines.h:428
#4  0x00007fffcf3366f5 in st_texture_image_map (st=0x7fffd42f6000, stImage=0x7fffd3ebe6a0, zoffset=0, usage=258, x=0, y=0, w=512, h=2) at src/mesa/state_tracker/st_texture.c:238
#5  0x00007fffcf32c74e in st_MapTextureImage (ctx=0x7fffd2ad6000, texImage=0x7fffd3ebe6a0, slice=0, x=0, y=0, w=512, h=2, mode=6, mapOut=0x7fffffff8c08, rowStrideOut=0x7fffffff8c1c) at src/mesa/state_tracker/st_cb_texture.c:187
#6  0x00007fffcf2e9fc3 in store_texsubimage (ctx=0x7fffd2ad6000, texImage=0x7fffd3ebe6a0, xoffset=0, yoffset=0, zoffset=0, width=512, height=2, depth=1, format=6408, type=5121, pixels=0x7fffd51fa000, packing=0x7fffd2adc888, 
    caller=0x7fffcfd0b92a "glTexImage") at src/mesa/main/texstore.c:4352
#7  0x00007fffcf2ea1e6 in _mesa_store_teximage (ctx=0x7fffd2ad6000, dims=2, texImage=0x7fffd3ebe6a0, format=6408, type=5121, pixels=0x7fffd51fa000, packing=0x7fffd2adc888) at src/mesa/main/texstore.c:4408
#8  0x00007fffcf32d11d in st_TexImage (ctx=0x7fffd2ad6000, dims=2, texImage=0x7fffd3ebe6a0, format=6408, type=5121, pixels=0x7fffd51fa000, unpack=0x7fffd2adc888) at src/mesa/state_tracker/st_cb_texture.c:537
#9  0x00007fffcf2d5163 in teximage (ctx=0x7fffd2ad6000, dims=2, target=3553, level=6, internalFormat=6408, width=512, height=2, depth=1, border=0, format=6408, type=5121, pixels=0x7fffd51fa000) at src/mesa/main/teximage.c:2623
#10 0x00007fffcf2d5343 in _mesa_TexImage2D (target=3553, level=6, internalFormat=6408, width=512, height=2, border=0, format=6408, type=5121, pixels=0x7fffd51fa000) at src/mesa/main/teximage.c:2663
#11 0x00007ffff365a06f in mozilla::gl::GLContext::fTexImage2D (this=0x7fffde5f9000, target=3553, level=6, internalformat=6408, width=512, height=2, border=0, format=6408, type=5121, pixels=0x7fffd51fa000)
    at ../../../dist/include/GLContext.h:2672

(gdb) p xlib_dt
$1 = (struct xlib_displaytarget *) 0x100000001

If this 0x100000001 is really fixed and can't be manipulated, then we're safe. Is it?
Comment 11 miaubiz 2012-08-28 00:38:01 PDT
Created attachment 655907 [details]
minifuzzer to look for possible values
Comment 12 miaubiz 2012-08-28 00:40:46 PDT
on intel with attachment 655907 [details] I can get atleast:

==14260== ERROR: AddressSanitizer crashed on unknown address 0x00005c008000 (pc 0x7fffb70fc1c4 sp 0x7fffffff48e0 bp 0x000000000007 T0)
==15327== ERROR: AddressSanitizer crashed on unknown address 0x000000000008 (pc 0x7fffb78d296b sp 0x7fffffff4810 bp 0x7fffcba1d880 T0)
==10880== ERROR: AddressSanitizer crashed on unknown address 0x000016004000 (pc 0x7fffb78fc48f sp 0x7fffffff48e0 bp 0x000000000000 T0)
==7359== ERROR: AddressSanitizer crashed on unknown address 0x000017002010 (pc 0x7ffff6f37e64 sp 0x7fffffff45e8 bp 0x7fffffff4840 T0)
==8053== ERROR: AddressSanitizer crashed on unknown address 0x00002e004010 (pc 0x7ffff6f37e64 sp 0x7fffffff45d8 bp 0x7fffffff4830 T0)
==11526== ERROR: AddressSanitizer crashed on unknown address 0x000000000000 (pc 0x7fffb74a9c39 sp 0x7fffffff4848 bp 0x7ffff6b4c080 T0)
==12790== ERROR: AddressSanitizer crashed on unknown address 0x00005e004010 (pc 0x7ffff6f37e64 sp 0x7fffffff45e8 bp 0x7fffffff4840 T0)
==13532== ERROR: AddressSanitizer crashed on unknown address 0x000078010000 (pc 0x7fffb78fc17a sp 0x7fffffff48e0 bp 0x000000000003 T0)
Comment 13 Benoit Jacob [:bjacob] (mostly away) 2012-08-28 08:30:00 PDT
Hah. So this bug looks security-sensitive depending on the driver: sec-sensitive on the Intel driver at least (comment 12) but apparently not on LLVMpipe driver (comment 8).
Comment 14 Benoit Jacob [:bjacob] (mostly away) 2012-08-28 12:30:18 PDT
Created attachment 656157 [details] [diff] [review]
work around the mesa teximage2d crasher by rejecting certain legal, but rare, teximage2d calls

This fixes it at least for the present testcase.
Comment 15 Benoit Jacob [:bjacob] (mostly away) 2012-08-28 12:33:34 PDT
It's worth checking if a similar bug exists in glCopyTexImage2D.
Comment 16 Jeff Gilbert [:jgilbert] 2012-08-28 22:19:08 PDT
Comment on attachment 656157 [details] [diff] [review]
work around the mesa teximage2d crasher by rejecting certain legal, but rare, teximage2d calls

Review of attachment 656157 [details] [diff] [review]:
-----------------------------------------------------------------

Gross, but necessary. Does this show up as a failure when the conformance suite is run? If not, we should add a test to make sure it does. We don't want to forget about something subtle (and hopefully temporary!) like this.
Comment 17 Jeff Gilbert [:jgilbert] 2012-08-28 22:23:44 PDT
We should actually be able to work around this in most cases, but it's probably not worth the effort when it should instead be fixed in Mesa.
Comment 18 David Bolter [:davidb] 2012-08-29 12:16:39 PDT
Marking sec-critical during sec triage based on comment 12 addresses.
Comment 19 Benoit Jacob [:bjacob] (mostly away) 2012-08-31 14:17:40 PDT
You are right jgilbert, adding webgl-test-needed. I haven't actually checked if this is caught by the trunk test suite but let's default to assuming it isn't.

I am not convinced though that it really matters. If it does, rather than this fix, we should instead insert dummy texImage2D calls to allocate the level 0 images.
Comment 20 Jeff Gilbert [:jgilbert] 2012-08-31 16:40:49 PDT
(In reply to Benoit Jacob [:bjacob] from comment #19)
> You are right jgilbert, adding webgl-test-needed. I haven't actually checked
> if this is caught by the trunk test suite but let's default to assuming it
> isn't.
> 
> I am not convinced though that it really matters. If it does, rather than
> this fix, we should instead insert dummy texImage2D calls to allocate the
> level 0 images.

We are lucky that the workaround is simply uploading a zero'd texture. Since GLES requires level-0 to be specified for the texture to be complete, we can just emulate it being incomplete with zero'd level-0 data.
Comment 21 Benoit Jacob [:bjacob] (mostly away) 2012-09-13 13:03:15 PDT
(In reply to Benoit Jacob [:bjacob] from comment #15)
> It's worth checking if a similar bug exists in glCopyTexImage2D.

Checked: there is a similar bug with glCopyTexImage2D. Attaching corresponding testcase.
Comment 22 Benoit Jacob [:bjacob] (mostly away) 2012-09-13 13:03:51 PDT
Created attachment 660941 [details]
variant testcase with copyTexImage2D
Comment 23 Daniel Veditz [:dveditz] 2012-09-13 13:08:45 PDT
Since it's a mesa crash we're working around and these are pretty basic functions I assume it affects all old branches. If so please remove the regressionwindow-wanted flag and set the appropriate branch status flags to "affected".
Comment 24 Benoit Jacob [:bjacob] (mostly away) 2012-09-13 13:17:25 PDT
... and I have checked compressedTexImage2D too (with a DXT1 texture): it does NOT trigger the crash.

Going to do the above-discussed workaround which should be cheap enough (we should be able to get away with a 1x1 dummy texture) so we'll be able to just keep the workaround for all Mesa versions, rather than having to track Mesa versions.
Comment 25 Benoit Jacob [:bjacob] (mostly away) 2012-09-14 07:18:27 PDT
Created attachment 661214 [details]
new testcase showing a crash even though all mipmap levels were defined

This new testcase nullifies the above patch -- it crashes even with this patch, in fact, it falls outside of the scope of what that patch works around.

APItrace:

453 glGenTextures(n = 1, textures = &2)
455 glBindTexture(target = GL_TEXTURE_2D, texture = 2)
458 glTexImage2D(target = GL_TEXTURE_2D, level = 0, internalformat = GL_RGBA, width = 512, height = 2, border = 0, format = GL_RGBA, type = GL_UNSIGNED_BYTE, pixels = blob(4096))
462 glTexImage2D(target = GL_TEXTURE_2D, level = 1, internalformat = GL_RGBA, width = 512, height = 2, border = 0, format = GL_RGBA, type = GL_UNSIGNED_BYTE, pixels = blob(4096))
466 glTexImage2D(target = GL_TEXTURE_2D, level = 2, internalformat = GL_RGBA, width = 512, height = 2, border = 0, format = GL_RGBA, type = GL_UNSIGNED_BYTE, pixels = blob(4096))
470 glTexImage2D(target = GL_TEXTURE_2D, level = 3, internalformat = GL_RGBA, width = 512, height = 2, border = 0, format = GL_RGBA, type = GL_UNSIGNED_BYTE, pixels = blob(4096))
474 glTexImage2D(target = GL_TEXTURE_2D, level = 4, internalformat = GL_RGBA, width = 512, height = 2, border = 0, format = GL_RGBA, type = GL_UNSIGNED_BYTE, pixels = blob(4096))
478 glTexImage2D(target = GL_TEXTURE_2D, level = 5, internalformat = GL_RGBA, width = 512, height = 2, border = 0, format = GL_RGBA, type = GL_UNSIGNED_BYTE, pixels = blob(4096)) // incomplete
Comment 26 Benoit Jacob [:bjacob] (mostly away) 2012-09-14 07:26:39 PDT
The crash condition appears to be:

  level>0  AND width>=512 AND height>=2 AND (mipmap level 0 undefined OR mipmap level 0 has the same width)
Comment 27 Benoit Jacob [:bjacob] (mostly away) 2012-09-14 07:47:47 PDT
Created attachment 661222 [details] [diff] [review]
work around the mesa teximage2d crasher by rejecting certain legal, but rare, teximage2d and copyTexImage2d calls

This prevents crashes in all the variants of the testcase that I've been able to make.
Comment 28 Jeff Gilbert [:jgilbert] 2012-09-14 16:56:05 PDT
Comment on attachment 661222 [details] [diff] [review]
work around the mesa teximage2d crasher by rejecting certain legal, but rare, teximage2d and copyTexImage2d calls

Review of attachment 661222 [details] [diff] [review]:
-----------------------------------------------------------------

I could be misreading this, but it looks like the conditions we test are wrong.

::: content/canvas/src/WebGLContextGL.cpp
@@ +908,5 @@
> +        level > 0 &&
> +        !sub)
> +    {
> +        size_t face = WebGLTexture::FaceForTarget(target);
> +        WebGLTexture *tex = activeBoundTextureForTarget(target);

Is |tex| always going to be non-null?

@@ +910,5 @@
> +    {
> +        size_t face = WebGLTexture::FaceForTarget(target);
> +        WebGLTexture *tex = activeBoundTextureForTarget(target);
> +        if (!tex->HasImageInfoAt(0, face) ||
> +            tex->ImageInfoAt(0, face).Width() >= width)

This doesn't seem in line with the crash condition you listed: "level>0  AND width>=512 AND height>=2 AND (mipmap level 0 undefined OR mipmap level 0 has the same width)"

Also, |tex->ImageInfoAt(0, face).Width() >= width| seems like it would be very common, since level >0 should always have a smaller width than level 0.

@@ +913,5 @@
> +        if (!tex->HasImageInfoAt(0, face) ||
> +            tex->ImageInfoAt(0, face).Width() >= width)
> +        {
> +            return  ErrorInvalidOperation("%s: rejecting valid call to avoid Mesa driver crash. "
> +                                          "Please go complain on https://bugzilla.mozilla.org/show_bug.cgi?id=785734",

Why are we telling people to complain on this bug, especially since this bug is currently a sec-bug, and thus invisible to most? Should we not tell them to complain to Mesa?
Comment 29 Benoit Jacob [:bjacob] (mostly away) 2012-09-21 12:46:55 PDT
Created attachment 663519 [details] [diff] [review]
work around the mesa teximage2d crasher by rejecting certain legal, but rare, teximage2d and copyTexImage2d calls

Indeed: tex needed checking for non-null, and the inequality on width was backwards. I want to keep a reference to this bug so we get a chance to learn if the solution adopted here is harming real-world content (once this bug is un-hidden), but I simplified it.
Comment 30 Jeff Gilbert [:jgilbert] 2012-09-24 15:56:13 PDT
Comment on attachment 663519 [details] [diff] [review]
work around the mesa teximage2d crasher by rejecting certain legal, but rare, teximage2d and copyTexImage2d calls

Review of attachment 663519 [details] [diff] [review]:
-----------------------------------------------------------------

One formatting nit.

::: content/canvas/src/WebGLContextGL.cpp
@@ +678,5 @@
> +    {
> +        if (!tex->HasImageInfoAt(0, face) ||
> +            tex->ImageInfoAt(0, face).Width() <= width)
> +        {
> +            return  ErrorInvalidOperation("%s: rejecting valid call to avoid Mesa driver crash. "

You have an extra space between 'return' and 'ErrorInvalidOperation'.
Comment 31 Benoit Jacob [:bjacob] (mostly away) 2012-09-28 04:42:59 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/a0bae754ad5d
Comment 32 Ed Morley [:emorley] 2012-09-28 16:27:51 PDT
https://hg.mozilla.org/mozilla-central/rev/a0bae754ad5d
Comment 33 Benoit Jacob [:bjacob] (mostly away) 2012-10-02 19:42:29 PDT
Comment on attachment 663519 [details] [diff] [review]
work around the mesa teximage2d crasher by rejecting certain legal, but rare, teximage2d and copyTexImage2d calls

Sorry for the delayed request.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Driver bug
User impact if declined: sec-critical
Testing completed (on m-c, etc.): been on m-c since last week
Risk to taking this patch (and alternatives if risky): very low risk, worst case we'd break some WebGL page, but this has been in m-c for half a week without a report of regression
String or UUID changes made by this patch: none
Comment 34 Alex Keybl [:akeybl] 2012-10-03 14:59:54 PDT
Comment on attachment 663519 [details] [diff] [review]
work around the mesa teximage2d crasher by rejecting certain legal, but rare, teximage2d and copyTexImage2d calls

This missed our final beta, and doesn't seem critical for release. Approving for Aurora 17, however.
Comment 35 Benoit Jacob [:bjacob] (mostly away) 2012-10-04 08:15:12 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/cf222889c62f
Comment 36 Gregg Tavares 2012-10-05 17:44:59 PDT
Hey guys, are you sure a simple rejection of

  GLint maxSize = MaxSizeForTarget(target) >> level;

  if (width > maxSize || height > maxSize) {
     // generate GL error INVALID_VALUE

doesn't handle the issue as well? I tested this on Chrome and it seemed to stop the asan issues though I might have missed one.
Comment 37 Benoit Jacob [:bjacob] (mostly away) 2012-10-05 18:43:44 PDT
Hm, so here I am testing with a debug build of Mesa and I'm testing whether the Mesa assertion fails rather than whether a memory corruption happens (because I am lazy).

The condition, here, is:

  GLint maxSize = 16384 >> level;

  if (width > maxSize || height > maxSize) {
     // generate GL error INVALID_VALUE

But MAX_TEXTURE_SIZE, as returned by Mesa, is 4096.

In any case, it does seem that the problem is symmetric between height and width, so I need to amend my patch in some way.
Comment 38 Benoit Jacob [:bjacob] (mostly away) 2012-10-05 18:44:21 PDT
Reopening because the same issue occurs with height as it does with width -- not sure why I thought otherwise.
Comment 39 Benoit Jacob [:bjacob] (mostly away) 2012-10-05 18:52:36 PDT
More testing confirms comment 37 on my system; since your proposed restriction is stricter, it would indeed avoid the crash.
Comment 40 Benoit Jacob [:bjacob] (mostly away) 2012-10-05 18:57:38 PDT
Created attachment 668711 [details] [diff] [review]
correct patch (Gregg's idea)
Comment 41 Gregg Tavares 2012-10-05 20:03:37 PDT
I added a conformance test under the assumption that Daniel's interpretation of the ES spec is correct 

https://www.khronos.org/registry/webgl/sdk/tests/conformance/textures/texture-size-limit.html
Comment 42 Benoit Jacob [:bjacob] (mostly away) 2012-10-05 22:57:51 PDT
Thanks!

You're right, indeed, in the GLES 2.0.25 spec page 69 it says that we should return INVALID_VALUE in this case.

Then I'll update the patch to 1) generate INVALID_VALUE and 2) do this check unconditionally, not just as a driver work-around.
Comment 43 Benoit Jacob [:bjacob] (mostly away) 2012-10-05 23:02:14 PDT
Created attachment 668732 [details] [diff] [review]
correct patch (Gregg's idea)
Comment 44 Jeff Gilbert [:jgilbert] 2012-10-07 21:33:50 PDT
Comment on attachment 668732 [details] [diff] [review]
correct patch (Gregg's idea)

Review of attachment 668732 [details] [diff] [review]:
-----------------------------------------------------------------

Interesting.
Comment 45 Benoit Jacob [:bjacob] (mostly away) 2012-10-09 11:22:41 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/81675e760c12
Comment 46 Benoit Jacob [:bjacob] (mostly away) 2012-10-09 11:23:18 PDT
Comment on attachment 668732 [details] [diff] [review]
correct patch (Gregg's idea)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): driver bug
User impact if declined: sec-critical
Testing completed (on m-c, etc.): m-i
Risk to taking this patch (and alternatives if risky): no risk
String or UUID changes made by this patch: none
Comment 47 Ryan VanderMeulen [:RyanVM] 2012-10-09 18:41:17 PDT
https://hg.mozilla.org/mozilla-central/rev/81675e760c12

(I reset the status flags based on what I can tell happened here. Please change them back if I goofed)
Comment 49 Alex Keybl [:akeybl] 2012-10-18 16:18:48 PDT
(In reply to Benoit Jacob [:bjacob] from comment #48)
> http://hg.mozilla.org/releases/mozilla-aurora/rev/fe15e7126620
> http://hg.mozilla.org/releases/mozilla-beta/rev/6feb23ee423c

Please prepare a patch for mozilla-esr10 as well. Thanks!
Comment 50 Benoit Jacob [:bjacob] (mostly away) 2012-10-26 11:16:55 PDT
Created attachment 675620 [details] [diff] [review]
esr10 patch

I threw into this patch the fix for the same issue in copyTexImage2D, bug 802778.
Comment 51 Benoit Jacob [:bjacob] (mostly away) 2012-10-27 13:06:36 PDT
Comment on attachment 675620 [details] [diff] [review]
esr10 patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Comment 52 Benoit Jacob [:bjacob] (mostly away) 2012-10-27 13:07:47 PDT
Erm, again:

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec:critical
User impact if declined: sec:critical
Fix Landed on Version: 17+
Risk to taking this patch (and alternatives if risky): very low risk (simple, well tested already)
String or UUID changes made by this patch: none
Comment 53 Ryan VanderMeulen [:RyanVM] 2012-11-03 06:38:37 PDT
https://hg.mozilla.org/releases/mozilla-esr10/rev/331ec7c812f0
Comment 54 Matt Wobensmith [:mwobensmith][:matt:] 2013-01-16 16:14:35 PST
Can miaubiz@gmail.com take a look and verify fixed on these config(s)? I currently don't have access to these particular configs in order to confirm. Thank you.
Comment 55 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-01-16 16:51:07 PST
Swapping out verifyme for [qa-] given comment 54.
Comment 56 miaubiz 2013-01-17 08:55:30 PST
Matt Wobensmith: how do I verify? it's fixed for me atleast :|
Comment 57 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-01-17 10:00:19 PST
(In reply to miaubiz from comment #56)
> Matt Wobensmith: how do I verify? it's fixed for me atleast :|

If you cannot reproduce this crash anymore then I think it's safe to call this verified. Can you please confirm the following builds don't crash:
* Firefox 17.0.2
* Firefox 18.0.1
* Firefox 19.0b2
* Firefox 17.0.2esr
* Firefox 10.0.12esr
(listed in order of priority)

Thank you
Comment 58 miaubiz 2013-01-17 10:29:21 PST
couldn't find a firefox 17.0.2 non-esr

18.0 and 19.0b2 both say "Error: WebGL: texImage2D: the maximum texture size for level 6 is 128", as expected.

cheers
Comment 59 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-01-17 10:32:07 PST
Sorry, the remaining builds are here:
* 17.0.1: ftp://ftp.mozilla.org/pub/firefox/releases/17.0.1/
* 17.0.2esr: ftp://ftp.mozilla.org/pub/firefox/releases/17.0.2esr/
* 10.0.12esr: ftp://ftp.mozilla.org/pub/firefox/releases/10.0.12esr/

Thank you
Comment 60 miaubiz 2013-01-17 11:14:57 PST
17.0.1 and 17.0.2esr give the same correct error message

10.0.12esr can't get a webgl context, presumably because webgl was blacklisted in 10.0.12esr,  it is therefore not affected by the bug.
Comment 61 Benoit Jacob [:bjacob] (mostly away) 2013-01-17 11:29:15 PST
miaubiz, your time spent verifying this is very much appreciated!

Mesa is blacklisted in ESR10 as you noticed in comment 60.
Comment 62 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-01-17 12:09:14 PST
Thank you very much, miaubuz. Marking this verified across all branches.
Comment 63 Raymond Forbes[:rforbes] 2013-07-19 18:29:41 PDT
rforbes-bugspam-for-setting-that-bounty-flag-20130719

Note You need to log in before you can comment on or make changes to this bug.