Closed Bug 635666 Opened 9 years ago Closed 9 years ago

WebGL crash [@mozilla::WebGLContext::CopyTexSubImage2D]

Categories

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

x86_64
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: posidron, Assigned: bjacob)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase, Whiteboard: [softblocker])

Crash Data

Attachments

(5 files, 1 obsolete file)

Attached file testcase
Build-Identifikator: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b11) Gecko/20100101 Firefox/4.0b11

Reload the testcase a few times if it doesn't crash on the first try.
Attached file callstack
I can reproduce on linux, but the crash report doesn't have a useful stack:
https://crash-stats.mozilla.com/report/index/bp-f7da2869-75ad-40c4-9cf5-b47ce2110222
OS: Mac OS X → All
Attached patch patch (obsolete) — Splinter Review
The crash was that we weren't validating |level| in this function, so we were accessing out of bounds ImageInfo on this texture.

Actually, there were several issues:
 * we were not checking for (2^level > maxTextureSize).
 * we were not checking that an image had already been defined at this level
 * in other functions checking for (2^level > maxTextureSize), the check was basically
      (1 << level) > maxTextureSize
which was very fragile as shift overflow could had given 1<<level == 0. So I replaced this by the condition
      !(maxTextureSize >> level)
Attachment #514248 - Flags: review?(vladimir)
Attachment #514248 - Flags: review?(vladimir) → review?(jmuizelaar)
This part does just the validation of the value of |level| (must not be bigger than log2(max_tex_size).
Attachment #514248 - Attachment is obsolete: true
Attachment #514248 - Flags: review?(jmuizelaar)
Attachment #514602 - Flags: review?(jmuizelaar)
This part makes us not assume face 0.
Attachment #514603 - Flags: review?(jmuizelaar)
this is where we check that the image info array has data for that level before dereferencing that data
Attachment #514604 - Flags: review?(jmuizelaar)
Attachment #514602 - Flags: review?(jmuizelaar) → review+
Attachment #514603 - Flags: review?(jmuizelaar) → review+
Attachment #514604 - Flags: review?(jmuizelaar) → review+
Attachment #514602 - Flags: approval2.0+
Attachment #514603 - Flags: approval2.0+
Comment on attachment 514604 [details] [diff] [review]
part 3/3: actual crash fix: check that image data has been defined

Can we add this testcase as a crashtest?
Attachment #514604 - Flags: approval2.0+
blocking2.0: --- → final+
Whiteboard: [softblocker]
(Requested blocker status because this is an easy-to-hit crasher, in that just passing a bogus |level| parameter to copyTexSubImage2D typically crashes).
Blocks: 658170
Assignee: nobody → bjacob
Crash Signature: [@mozilla::WebGLContext::CopyTexSubImage2D]
You need to log in before you can comment on or make changes to this bug.