Last Comment Bug 737182 - 2D texture corruption on Mac/Intel with large texture sizes >= 4993
: 2D texture corruption on Mac/Intel with large texture sizes >= 4993
Status: VERIFIED FIXED
[sg:vector-high]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla14
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
Mentors:
http://www.dannijo.com/product_info.p...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-19 13:55 PDT by Hilary Hall [:hillzy]
Modified: 2012-07-12 10:45 PDT (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified
verified
12+
verified


Attachments
Image corruption image (621.35 KB, image/png)
2012-03-19 13:55 PDT, Hilary Hall [:hillzy]
no flags Details
testcase (1.95 MB, application/zip)
2012-03-20 11:23 PDT, Joe Drew (not getting mail)
no flags Details
small testcase using WebGL (4.55 KB, text/html)
2012-03-20 15:59 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details
near-minimal testcase using only canvas2D (314 bytes, text/html)
2012-03-21 03:28 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details
limit max tex size on mac/intel (4.39 KB, patch)
2012-03-21 04:44 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
limit max tex size on mac/intel (4.97 KB, patch)
2012-03-21 04:50 PDT, Benoit Jacob [:bjacob] (mostly away)
joe: review-
Details | Diff | Splinter Review
drop WebGL cube map workaround (1.57 KB, text/plain)
2012-03-21 04:54 PDT, Benoit Jacob [:bjacob] (mostly away)
jmuizelaar: review+
Details
limit max tex size on mac/intel (6.21 KB, patch)
2012-03-25 19:08 PDT, Benoit Jacob [:bjacob] (mostly away)
b56girard: review+
Details | Diff | Splinter Review
limit max tex size on mac/intel (6.37 KB, patch)
2012-03-26 09:03 PDT, Benoit Jacob [:bjacob] (mostly away)
b56girard: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Hilary Hall [:hillzy] 2012-03-19 13:55:39 PDT
Created attachment 607300 [details]
Image corruption image

When I go to: http://www.dannijo.com/product_info.php?products_id=766 and click on the image to view details, check the file I attached to see what happens:

Hardware Info:
11" Macbook Air - Mid 2011
Graphics  Intel HD Graphics 3000 384 MB
Software  Mac OS X Lion 10.7.3 (11D50b)

Running Firefox 11.0
Comment 1 Benoit Girard (:BenWa) 2012-03-19 14:06:21 PDT
STR: Click on the image

I can confirm the bug by running on my integrated gpu, not discrete:
15-inch, Early 2011
Intel Inc. -- Intel HD Graphics 3000 OpenGL Engine -- 2.1 APPLE-7.14.5

Vague regression range (between firefox 9 and 10):
http://hg.mozilla.org/releases/mozilla-release/rev/72ad46d416ce is affected
http://hg.mozilla.org/releases/mozilla-release/rev/c4405d7a95f6 is not
Comment 2 Joe Drew (not getting mail) 2012-03-19 15:35:27 PDT
This is a really big image -

(gdb) p iwidth
$1 = 5455
(gdb) p iheight
$2 = 5455

While the Intel card apparently allows 8192, this makes me suspicious....
Comment 3 Benoit Jacob [:bjacob] (mostly away) 2012-03-20 11:10:17 PDT
Our experience in bug 684882 may be worth keeping in mind here (intel/mac driver too optimistic with its max tex size)
Comment 4 Joe Drew (not getting mail) 2012-03-20 11:19:13 PDT
It's not a strict image size problem; resizing the image to 4097x4097 lets it work, as does 4500x4500, but 5000x5000 doesn't.
Comment 5 Joe Drew (not getting mail) 2012-03-20 11:20:47 PDT
Given that this is not a simple problem, I'm going to bow out of this in favour of working on higher-priority items.
Comment 6 Joe Drew (not getting mail) 2012-03-20 11:23:38 PDT
Created attachment 607633 [details]
testcase

This is a simple save-as of the website, and reproduces the bug on load.
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2012-03-20 14:38:15 PDT
Thanks for hiding this bug. uninitialized-video-memory-exposed bugs with a reproducible testcase are definitely sg:high.
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2012-03-20 15:57:34 PDT
Here is a testcase using WebGL, showing this to be a driver bug with huge 2D textures. It uses a 5455x5455 texture. This was directly adapted from the cube map testcase from bug 684882.

http://people.mozilla.org/~bjacob/webgltexture2D5k.html

STR:
1. use gfxCardStatus to force usage of the Intel GPU
2. go to above testcase

Confirmed to reproduce in both Firefox and Chrome.

Probable solution: like in bug 684882, limit texture size on Mac/Intel. Here, 4096 seems like the natural choice, unless someone can reproduce the bug at sizes <= 4096. By that occasion, we should probably move the existing cube map workaround from WebGL implementation into GLContext as well.
Comment 9 Benoit Jacob [:bjacob] (mostly away) 2012-03-20 15:59:48 PDT
Created attachment 607753 [details]
small testcase using WebGL
Comment 10 Benoit Jacob [:bjacob] (mostly away) 2012-03-21 03:28:55 PDT
Created attachment 607893 [details]
near-minimal testcase using only canvas2D

Here's a more or less minimal testcase, without WebGL, which means in particular that the Mac will naturally stay on the integrated Intel GPU i.e. no need for gfxCardStatus anymore to reproduce.

public URL:
http://people.mozilla.org/~bjacob/canvas2D5k.html

STR:
1. use Mac OSX 10.6+ (so we do accelerated layers) with Intel graphics, make sure that the integrated Intel GPU is in use.
2. click above link

Confirmed to reproduce in Firefox.
Comment 11 Benoit Jacob [:bjacob] (mostly away) 2012-03-21 04:44:32 PDT
Created attachment 607909 [details] [diff] [review]
limit max tex size on mac/intel

This should fix it, assuming that 2D texture sizes <= 4096 are safe.

I wasn't sure what was the best way to simulate a ERROR_INVALID_VALUE. In WebGLContext we supplant the native GL error system, but that's not the case in GLContext outside of debug mode at the moment. So I thought the simplest was to change the width and height to -1. Indeed, GLsizei is signed, negative values explicitly generate INVALID_VALUE, and even if they were interpreted as a huge positive value (in case of a driver bug), that would still be INVALID_VALUE (larger than max value).

I've been wondering how safe it is to let this workaround depend on the VENDOR string, given Mac GPU switching. I think it's quite safe as the GLContext is the LayerManager's, which is created when the Window is created, which is earlier than when content can force a GPU switch (by creating a WebGL context). One possible attack though would go as follows:
 1. browser starts, main window using GL layers on Intel GPU
 2. create a WebGL context -> switch all to discrete GPU
 3. create a new window. This step probably requires some social engineering i.e. I don't think that content can create new windows entirely by itself.
 4. New window's GLContext is created on discrete GPU, hence it doesn't have the texture size restriction.
 5. drop reference to WebGL context object and canvas
 6. wait for GC to happen (about 20 seconds)
 7. When the WebGL context is GC'd, the Mac switches back to Intel GPU
 8. in the second browser window created at step 3, create a very large 2d canvas as in the testcase here.

Should we worry about such a convoluted attack path?
Comment 12 Benoit Jacob [:bjacob] (mostly away) 2012-03-21 04:50:00 PDT
Created attachment 607910 [details] [diff] [review]
limit max tex size on mac/intel

Let's initialize these max-size variables in the ctor to avoid any headache if the initialization code setting them is ever not called.
Comment 13 Benoit Jacob [:bjacob] (mostly away) 2012-03-21 04:54:35 PDT
Created attachment 607912 [details]
drop WebGL cube map workaround

Once the above GLContext patch lands, we won't need the WebGL-specific tweak anymore.

Keeping this a separate patch because I want to only ask for aurora/beta approval for the GLContext patch, not for this one.
Comment 14 Benoit Jacob [:bjacob] (mostly away) 2012-03-21 07:02:32 PDT
(In reply to Benoit Girard (:BenWa) from comment #1)
> Vague regression range (between firefox 9 and 10):
> http://hg.mozilla.org/releases/mozilla-release/rev/72ad46d416ce is affected
> http://hg.mozilla.org/releases/mozilla-release/rev/c4405d7a95f6 is not

This could be explained by the fact that we used to have a bug whereby we stayed on the discrete GPU all the time (see bug 646043) and eventually fixed it.
Comment 15 Benoit Jacob [:bjacob] (mostly away) 2012-03-21 08:14:55 PDT
We bisected the minimum texture size reproducing this on Etienne's Mac. The smallest size reproducing this is 4993.
Comment 16 Benoit Girard (:BenWa) 2012-03-21 08:33:34 PDT
(In reply to Benoit Jacob [:bjacob] from comment #14)
> (In reply to Benoit Girard (:BenWa) from comment #1)
> > Vague regression range (between firefox 9 and 10):
> > http://hg.mozilla.org/releases/mozilla-release/rev/72ad46d416ce is affected
> > http://hg.mozilla.org/releases/mozilla-release/rev/c4405d7a95f6 is not
> 
> This could be explained by the fact that we used to have a bug whereby we
> stayed on the discrete GPU all the time (see bug 646043) and eventually
> fixed it.

That's true, I didn't pay attention to that.
Comment 17 Benoit Girard (:BenWa) 2012-03-21 08:40:47 PDT
Comment on attachment 607910 [details] [diff] [review]
limit max tex size on mac/intel

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

::: gfx/gl/GLContext.h
@@ +1691,5 @@
> +        // max texture size that they report. So we check ourselves against our own values
> +        // (mMax[CubeMap]TextureSize) which we tweak as needed.
> +        GLsizei maxSize = target == LOCAL_GL_TEXTURE_2D
> +                          ? mMaxTextureSize
> +                          : mMaxCubeMapTextureSize;

Why do we want to use MaxCubeMapTextureSize for non GL_TEXTURE_2D target? I know for example on mobile we want to use the external image target and I don't think it has any relationship with CubeMaps.

@@ +2399,5 @@
>          AFTER_GL_CALL;
>      }
>  
>      void fTexImage2D(GLenum target, GLint level, GLint internalformat, GLsizei width, GLsizei height, GLint border, GLenum format, GLenum type, const GLvoid *pixels) {
> +        HandleIllegalTextureSizes(target, &width, &height);

This will return w=-1,h=-1. Do we know for sure that glTexImage2D and the likes will handle this? I don't like relaying on drivers for error checking. If so I'd like to see a comment explaining why we know it's safe.
Comment 18 Benoit Jacob [:bjacob] (mostly away) 2012-03-21 08:45:24 PDT
(In reply to Benoit Girard (:BenWa) from comment #17)
> Comment on attachment 607910 [details] [diff] [review]
> limit max tex size on mac/intel
> 
> Review of attachment 607910 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLContext.h
> @@ +1691,5 @@
> > +        // max texture size that they report. So we check ourselves against our own values
> > +        // (mMax[CubeMap]TextureSize) which we tweak as needed.
> > +        GLsizei maxSize = target == LOCAL_GL_TEXTURE_2D
> > +                          ? mMaxTextureSize
> > +                          : mMaxCubeMapTextureSize;
> 
> Why do we want to use MaxCubeMapTextureSize for non GL_TEXTURE_2D target? I
> know for example on mobile we want to use the external image target and I
> don't think it has any relationship with CubeMaps.

Oh. I didn't know of other texture targets than 2D and Cube Maps. Are you referring to some extension that we are using somewhere? Link needed.

> 
> @@ +2399,5 @@
> >          AFTER_GL_CALL;
> >      }
> >  
> >      void fTexImage2D(GLenum target, GLint level, GLint internalformat, GLsizei width, GLsizei height, GLint border, GLenum format, GLenum type, const GLvoid *pixels) {
> > +        HandleIllegalTextureSizes(target, &width, &height);
> 
> This will return w=-1,h=-1. Do we know for sure that glTexImage2D and the
> likes will handle this? I don't like relaying on drivers for error checking.
> If so I'd like to see a comment explaining why we know it's safe.

True. I was just thinking that we've never seen a driver bug whereby texImage2D would fail to generate INVALID_VALUE on -1 sizes, but OK. So, if we don't want to rely on the driver to generate INVALID_VALUE for us, we have to implement our own GL error system, like we do in the WebGL impl. The patch is going to be a lot more intrusive. Do you prefer that still?
Comment 19 Benoit Jacob [:bjacob] (mostly away) 2012-03-21 08:47:54 PDT
Alternative approach: to further increase the likelihood that the GL impl will generate INVALID_VALUE on our texImage2D call, we could set level to -1.
Comment 20 Benoit Girard (:BenWa) 2012-03-21 08:57:28 PDT
I was thinking of just having an early return (We could use a macro maybe). I know that we could use GL_TEXTURE_RECTANGLE, the other target isn't used for glTexImage2D but is used for others calls like glBind.

http://www.khronos.org/registry/gles/extensions/OES/OES_EGL_image_external.txt

I don't want to implement our own GL error system. If your sure we will get INVALID_VALUE everywhere just add a comment to explain that we rely on this behavior.
Comment 21 Benoit Jacob [:bjacob] (mostly away) 2012-03-21 09:01:59 PDT
If we just early-return without somehow generating INVALID_VALUE, we break the GL spec and we'll be scratching our heads some time down the road about strange bugs ("why is this texture not correctly uploaded?").

I'd much rather keep GLContext compliant with the GL spec as much as possible.
Comment 22 Benoit Jacob [:bjacob] (mostly away) 2012-03-22 02:30:49 PDT
I don't understand why this would be only vector-high and not high. This bug directly, by itself, allows an attacker to read random video memory.
Comment 23 Benoit Jacob [:bjacob] (mostly away) 2012-03-22 02:31:39 PDT
Is it vector-high only because this is a bug in the Mac/Intel driver that's exposed by Firefox, not a bug in Firefox itself?
Comment 24 Joe Drew (not getting mail) 2012-03-22 07:18:32 PDT
Comment on attachment 607910 [details] [diff] [review]
limit max tex size on mac/intel

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

I take Benoit's review comments and adopt them as my own!
Comment 25 Jeff Muizelaar [:jrmuizel] 2012-03-23 07:24:12 PDT
Comment on attachment 607912 [details]
drop WebGL cube map workaround

Dropping review until the issues with the other patch are sorted out.
Comment 26 Benoit Jacob [:bjacob] (mostly away) 2012-03-25 19:08:24 PDT
Created attachment 609188 [details] [diff] [review]
limit max tex size on mac/intel

How about this approach instead. The helper becomes IsTextureSizeLegalForTarget() returning bool; it checks for cube map targets and assumes that anything else behaves like TEXTURE_2D, which is probably safer; and in case of an illegal size, it now also passes -1 for |level| and for |border|, maximizing chances to get an INVALID_VALUE (there are now 4 different invalid values passed to these GL functions, so the GL would have to be really wrong to fail to generate INVALID_VALUE). Also, |null| is passed for the texture data pointer so that in the worst case, we'd be derefing null instead of an arbitrary pointer.

If you don't like that, here are the other possibilities, choose one:
 1. we use a different GL function to artificially generate a INVALID_VALUE. Downside: APItraces will look very, very mysterious ("wtf does glCopyTexImage2D call glSomeUnrelatedFunc??")
 2. we implement our own GL error system cooperating with the native GL error system. There is all the code for that in the WebGL impl. Downside: more intrusive change, won't get beta approval.
 3. we fail silently without generating an error. Downside: we break the GL spec, will cause headaches.
Comment 27 Benoit Girard (:BenWa) 2012-03-25 19:48:04 PDT
Comment on attachment 609188 [details] [diff] [review]
limit max tex size on mac/intel

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

Looks good, minot comment nit.

::: gfx/gl/GLContext.cpp
@@ +446,5 @@
> +        if (mVendor == VendorIntel) {
> +            // see bug 737182 for 2D textures, bug 684822 for cube map textures.
> +            mMaxTextureSize        = NS_MIN(mMaxTextureSize,        4096);
> +            mMaxCubeMapTextureSize = NS_MIN(mMaxCubeMapTextureSize, 512);
> +            // for good measure, we align renderbuffers on what we do for 2D textures

The grammar seems off to me. How about:
For good measure we do the same for renderbuffers.

::: gfx/gl/GLContext.h
@@ +2405,5 @@
>      }
>  
>      void fTexImage2D(GLenum target, GLint level, GLint internalformat, GLsizei width, GLsizei height, GLint border, GLenum format, GLenum type, const GLvoid *pixels) {
>          BEFORE_GL_CALL;
> +        if (IsTextureSizeLegalForTarget(target, width, height)) {

If we cared about performance we could add the check only for Intel by setting fTexImage2D to a wrapper function. But we shouldn't be calling these often.
Comment 28 Benoit Jacob [:bjacob] (mostly away) 2012-03-25 21:28:13 PDT
Regarding the performance comment: maybe we can rename IsTextureSizeLegalForTarget to something slightly different and have it do the platform check.

I am not worried about texImage2D which is very expensive anyway, but it is indeed not great to add overhead to copyTexImage2D as a matter of principle (in practice, this is still negligible overhead).
Comment 29 Benoit Jacob [:bjacob] (mostly away) 2012-03-26 09:03:59 PDT
Created attachment 609343 [details] [diff] [review]
limit max tex size on mac/intel

How about this, for performance?
Comment 30 Benoit Girard (:BenWa) 2012-03-26 09:08:40 PDT
Comment on attachment 609343 [details] [diff] [review]
limit max tex size on mac/intel

LGTM
Comment 31 Benoit Jacob [:bjacob] (mostly away) 2012-03-26 09:18:28 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/7474b9e08e0d
Comment 32 Ed Morley [:emorley] 2012-03-27 05:37:10 PDT
https://hg.mozilla.org/mozilla-central/rev/7474b9e08e0d
Comment 33 Benoit Jacob [:bjacob] (mostly away) 2012-03-29 05:44:45 PDT
Comment on attachment 609343 [details] [diff] [review]
limit max tex size on mac/intel

[Approval Request Comment]
Regression caused by (bug #): ever since we enabled GL layers and WebGL on Mac (pre-Firefox 4)
User impact if declined: video memory leakage (information exposure) vulnerability on Mac. Exploitable just by creating a 2d canvas of size 5kx5k, or an image of that size, or a WebGL texture of that size.
Testing completed (on m-c, etc.): Been on m-c for a few days
Risk to taking this patch (and alternatives if risky): seems low risk. At worst  (if we were crazy unlucky) it could expose a different driver bug.
String changes made by this patch: none
Comment 34 Alex Keybl [:akeybl] 2012-03-29 15:21:17 PDT
Comment on attachment 609343 [details] [diff] [review]
limit max tex size on mac/intel

[Triage Comment]
We'll be on the lookout for driver regressions after this lands - approving for Aurora 13 and Beta 12.
Comment 36 Benoit Jacob [:bjacob] (mostly away) 2012-03-31 09:49:09 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/bd9c99b055ab
Comment 38 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-06 15:03:34 PDT
[Triage Comment]
Now that there's been some bake time on aurora/beta please go ahead and nominate for ESR landing as per https://wiki.mozilla.org/Release_Management/ESR_Landing_Process
Comment 39 Benoit Jacob [:bjacob] (mostly away) 2012-04-12 19:15:20 PDT
http://hg.mozilla.org/releases/mozilla-esr10/rev/d35ae8519966
Comment 40 Huzaifa Sidhpurwala 2012-04-19 23:20:55 PDT
Does this issue have a CVE id?
Comment 41 Benoit Jacob [:bjacob] (mostly away) 2012-04-20 06:13:02 PDT
Notice that this bug is completely Mac-specific (seeing your redhat email address ;-) )
Comment 42 Tracy Walker [:tracy] 2012-04-23 14:22:43 PDT
Verified with Fx 12 beta6
Comment 43 Benoit Jacob [:bjacob] (mostly away) 2012-05-17 20:39:45 PDT
Can we unhide this bug now? It's fixed in Release and in ESR.
Comment 44 Daniel Veditz [:dveditz] 2012-05-18 12:16:51 PDT
What's the status of the underlying driver bug? Will we be exposing users of other applications?
Comment 45 Benoit Jacob [:bjacob] (mostly away) 2012-05-18 12:47:39 PDT
See my reply in bug 684882 comment 92 as this is very similar.

Yes, other applications are affected, all the more since this bug is about 2D textures which are used by all the applications that use OpenGL to accelerate any kind of graphics. However, this has been known publicly for a while as a google search suggests:

https://www.google.ca/search?q=mac+texture+corruption

The discussions returned by this search don't have as specific information as we discussed in this bug though, but I don't think that our conclusion here, "2D texture uploads are garbage above a certain critical size that is roughly 5000", is particularly hard to arrive to.
Comment 46 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-07-11 10:45:26 PDT
Tracy, can you please take a moment to verify this fixed with Firefox 14 and the latest ESR nightly? Thanks.

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