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

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: 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.

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

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

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: patch

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)

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: part 1/3: fix level parameter validation

This part does just the validation of the value of |level| (must not be bigger than log2(max_tex_size).

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: part 2/3: use correct face for given target

This part makes us not assume face 0.

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: part 3/3: actual crash fix: check that image data has been defined

this is where we check that the image info array has data for that level before dereferencing that data

Comment 7

[Joe Drew (not getting mail)]

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?

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

(Requested blocker status because this is an easy-to-hit crasher, in that just passing a bogus |level| parameter to copyTexSubImage2D typically crashes).

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/mozilla-central/rev/f79c1f63133c
http://hg.mozilla.org/mozilla-central/rev/d5de293a00d1
http://hg.mozilla.org/mozilla-central/rev/4654c0c956b2