Closed Bug 1298576 Opened 9 years ago Closed 9 years ago

Division exception/EXCEPTION_INT_OVERFLOW in WebGl shader compiler

Categories

(Core :: Graphics: CanvasWebGL, defect, P1)

Product:
Core
Component:
Graphics: CanvasWebGL
Version:
51 Branch
Platform:
x86_64
Windows 10
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
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)

crash.html
648 bytes, text/html
Details
windbgOutput.txt
18.32 KB, text/plain
Details
Check int value
1.67 KB, patch
jgilbert
: review+
gchang
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Add crash test (carry r+: jgilbert)
1.51 KB, patch
Details | Diff | Splinter Review
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 value — — Splinter 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: 9 years ago
status-firefox52: --- → fixed
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.

Attachment

General

Creator:
Created:
Updated:
Size: