Closed Bug 1298576 Opened 3 years ago Closed 3 years ago

Division exception/EXCEPTION_INT_OVERFLOW in WebGl shader compiler

Categories

(Core :: Canvas: WebGL, defect, P1)

51 Branch
x86_64
Windows 10
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: aral.yaman, Assigned: ethlin)

Details

(Keywords: crash, testcase, Whiteboard: [sg:dos][gfx-noted])

Crash Data

Attachments

(4 files, 4 obsolete files)

Attached file crash.html
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20160826030226

Steps to reproduce:

Open crash.html




Actual results:

Firefox crashes if the modulo value is too high in a shader:

void main() {-2147483648 % 0xffffffff; gl_FragColor = vec4(0.5, 0.5, 1.0, 1.0);}

The crash is in gfx\angle\src\compiler\translator\intermnode.cpp (Line 1038, See windbgOutput.txt)

resultArray[i].setIConst(leftArray[i].getIConst() % rightArray[i].getIConst());

This is maybe a security relevant issue ... ?


Expected results:

No crash -> Maybe compile error
Attached file windbgOutput.txt
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64
Group: firefox-core-security → core-security
Component: Untriaged → Canvas: WebGL
Product: Firefox → Core
Whiteboard: [@ TIntermConstantUnion::foldBinary ]
Looks to be in ANGLE GLSL parsing code.

With the given fragment shader, the parser tries to optimise out the |-2147483648 % 0xffffffff| (= |INT_MIN % -1|) part by evaluating it in advance. This is undefined, since its definition would be in terms of |INT_MIN / -1|, which overflows.

I do wonder why Chromium is unaffected: maybe they compile with different flags which ignore integer overflow?
With Chrome I get two errors after compileShader:

GL_INVALID_OPERATION : glGenSyncTokenCHROMIUM: fence sync must be flushed before generating sync token
WebGL: CONTEXT_LOST_WEBGL: loseContext: context lost

Maybe this is the reason why Chrome is not chrashing as well...
Group: core-security → gfx-core-security
Keywords: crash, testcase
same thing on mac, ESR-45.3: bp-3537cff0-35e6-4109-bad8-a427a2160903

It's not really what people mean by "integer overflow" security problems, which are not caught by the CPU. On Mac it's described as "EXC_ARITHMETIC / EXC_I386_DIV" which is cryptic but closer. This crash does not appear exploitable.
Group: gfx-core-security
Status: UNCONFIRMED → NEW
Crash Signature: [@ TIntermConstantUnion::foldBinary ]
Ever confirmed: true
Summary: Integer overflow in WebGl shader compiler → Division exception/EXCEPTION_INT_OVERFLOW in WebGl shader compiler
Whiteboard: [@ TIntermConstantUnion::foldBinary ] → [sg:dos]
Whiteboard: [sg:dos] → [sg:dos][gfx-noted]
I can reproduce. I'll see how to fix.
Assignee: nobody → ethlin
It looks like we got EXC_ARITHMETIC error when doing -2147483648 / -1 or -2147483648 % -1. The testcase crashed my firefox immediately, so set priority to P1.
Priority: -- → P1
Attached patch Check int valueSplinter Review
It's like bug 504516. We should check if it's (-2147483648 / -1) or (-2147483648 % -1).
Attachment #8798729 - Flags: review?(jgilbert)
Attached patch Add crash test (obsolete) — Splinter Review
Add the test html to crash test.
Attachment #8798740 - Flags: review?(jgilbert)
Attachment #8798740 - Flags: review?(jgilbert)
Attached patch Add crash test (obsolete) — Splinter Review
Add the test case to crashtest.
Attachment #8798740 - Attachment is obsolete: true
Attachment #8798743 - Flags: review?(jgilbert)
Attachment #8798729 - Flags: review?(jgilbert) → review+
Attachment #8798743 - Flags: review?(jgilbert) → review+
Comment on attachment 8798743 [details] [diff] [review]
Add crash test

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

::: dom/canvas/crashtests/1298576-1.html
@@ +1,5 @@
> +<!DOCTYPE html>
> +<html>
> +<head>
> +<script id="fshader" type="x-shader/x-fragment">
> +void main() {-2147483648 % 0xffffffff; gl_FragColor = vec4(0.5, 0.5, 1.0, 1.0);}

Don't stuff this on one line if you can't need to. I don't think it's needed here.

@@ +4,5 @@
> +<script id="fshader" type="x-shader/x-fragment">
> +void main() {-2147483648 % 0xffffffff; gl_FragColor = vec4(0.5, 0.5, 1.0, 1.0);}
> +</script>
> +<script>
> +function start() {

If you put this script tag in <body>, you shouldn't even need to use a function.

@@ +5,5 @@
> +void main() {-2147483648 % 0xffffffff; gl_FragColor = vec4(0.5, 0.5, 1.0, 1.0);}
> +</script>
> +<script>
> +function start() {
> +var gl = document.getElementById("canvas").getContext("experimental-webgl");

var gl = document.createElement('canvas').getContext('webgl');

@@ +12,5 @@
> +var fshaderSource = document.getElementById("fshader").text;
> +gl.shaderSource(fshader, fshaderSource);
> +gl.compileShader(fshader);
> +}
> +</script></head><body onload="start()"><h1>try crash</h1><canvas id="canvas" width="400" height="240" style="border: 1px dotted grey"></canvas></body></html>

Spread these across lines normally. (and remove the canvas element)
Attachment #8798743 - Flags: review+ → review-
(In reply to Jeff Gilbert [:jgilbert] from comment #11)
> Comment on attachment 8798743 [details] [diff] [review]
> Add crash test
> 
> Review of attachment 8798743 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/crashtests/1298576-1.html
> @@ +1,5 @@
> > +<!DOCTYPE html>
> > +<html>
> > +<head>
> > +<script id="fshader" type="x-shader/x-fragment">
> > +void main() {-2147483648 % 0xffffffff; gl_FragColor = vec4(0.5, 0.5, 1.0, 1.0);}
> 
> Don't stuff this on one line if you can't need to. I don't think it's needed
> here.
> 
> @@ +4,5 @@
> > +<script id="fshader" type="x-shader/x-fragment">
> > +void main() {-2147483648 % 0xffffffff; gl_FragColor = vec4(0.5, 0.5, 1.0, 1.0);}
> > +</script>
> > +<script>
> > +function start() {
> 
> If you put this script tag in <body>, you shouldn't even need to use a
> function.
> 
> @@ +5,5 @@
> > +void main() {-2147483648 % 0xffffffff; gl_FragColor = vec4(0.5, 0.5, 1.0, 1.0);}
> > +</script>
> > +<script>
> > +function start() {
> > +var gl = document.getElementById("canvas").getContext("experimental-webgl");
> 
> var gl = document.createElement('canvas').getContext('webgl');
> 
> @@ +12,5 @@
> > +var fshaderSource = document.getElementById("fshader").text;
> > +gl.shaderSource(fshader, fshaderSource);
> > +gl.compileShader(fshader);
> > +}
> > +</script></head><body onload="start()"><h1>try crash</h1><canvas id="canvas" width="400" height="240" style="border: 1px dotted grey"></canvas></body></html>
> 
> Spread these across lines normally. (and remove the canvas element)

Okay, I just put the original testcase to the crash test. I'll fix these format issues.
Attached patch Add crash test (obsolete) — Splinter Review
I addressed the issues in comment 12.
Attachment #8798743 - Attachment is obsolete: true
Attachment #8800498 - Flags: review?(jgilbert)
Comment on attachment 8800498 [details] [diff] [review]
Add crash test

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

::: dom/canvas/crashtests/1298576-1.html
@@ +16,5 @@
> +gl.shaderSource(fshader, fshaderSource);
> +gl.compileShader(fshader);
> +</script>
> +</body>
> +</head>

head should not contain body.
Attachment #8800498 - Flags: review?(jgilbert) → review+
(In reply to Jeff Gilbert [:jgilbert] from comment #14)
> Comment on attachment 8800498 [details] [diff] [review]
> Add crash test
> 
> Review of attachment 8800498 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/crashtests/1298576-1.html
> @@ +16,5 @@
> > +gl.shaderSource(fshader, fshaderSource);
> > +gl.compileShader(fshader);
> > +</script>
> > +</body>
> > +</head>
> 
> head should not contain body.

Okay, I'll fix it.
Address jgilbert's comment.
Attachment #8800498 - Attachment is obsolete: true
Upload the correct patch.
Attachment #8802810 - Attachment is obsolete: true
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24b1342fe710
Check int value to prevent EXC_ARITHMETIC error. r=jgilbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/d85c7e9afe3f
Add crash test for WebGL shader compiler. r=jgilbert
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/24b1342fe710
https://hg.mozilla.org/mozilla-central/rev/d85c7e9afe3f
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment on attachment 8798729 [details] [diff] [review]
Check int value

Approval Request Comment
[Feature/regressing bug #]: webgl2
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8798729 - Flags: approval-mozilla-aurora?
Comment on attachment 8798729 [details] [diff] [review]
Check int value

Fix a crash. Aurora51+.
Attachment #8798729 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8798729 [details] [diff] [review]
Check int value

Hold the approval for the moment until I get more information.
Attachment #8798729 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora?
Comment on attachment 8798729 [details] [diff] [review]
Check int value

WebGL 2 is the feature we want to ship in 51. Aurora51+.

If you can fill the whole uplift request form in the future, that would be great.
Attachment #8798729 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.