Last Comment Bug 705663 - Fix braindead WebGLTexture::HasImageInfoAt
: Fix braindead WebGLTexture::HasImageInfoAt
: regression
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: Benoit Jacob [:bjacob] (mostly away)
Depends on:
Blocks: 635666
  Show dependency treegraph
Reported: 2011-11-28 07:10 PST by Benoit Jacob [:bjacob] (mostly away)
Modified: 2012-01-04 12:59 PST (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

fix HasImageInfoAt (1.14 KB, patch)
2011-11-28 07:10 PST, Benoit Jacob [:bjacob] (mostly away)
jmuizelaar: review+
christian: approval‑mozilla‑aurora+
christian: approval‑mozilla‑beta+
Details | Diff | Review

Description Benoit Jacob [:bjacob] (mostly away) 2011-11-28 07:10:25 PST
Created attachment 577251 [details] [diff] [review]
fix HasImageInfoAt

I have no idea how I've been able to create 3 separate bugs in 3 lines of code. This was done in a rush for bug 635666.

The first bug is that (level <= mMaxLevelWithCustomImages) didn't account for the case where no image at all has been defined, so the array length is zero.

The second bug is that ImageInfoAt(level, 0) should have been ImageInfoAt(level, face).

The third bug is only theoretical: we weren't really checking for integer overflow. This couldn't really be exploited in practice though, as doing so would have required creating arrays larger than addressable space. But since this function is security-critical (see bug 635666) let's be future-proof.
Comment 1 Benoit Jacob [:bjacob] (mostly away) 2011-11-28 11:38:18 PST
Comment 3 Daniel Veditz [:dveditz] 2011-12-18 22:32:00 PST
I don't think we need to hide this as a security bug, there's no immediate vulnerability here.
Comment 4 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-28 13:38:59 PST
Is this something QA can verify?
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2011-12-30 16:06:52 PST
The only useful thing to do here would be to add a unit test; ping me after holidays if you think this is worth it (the likelihood of a regression around here seems low to me). This function is used by webgl.texImage2D so a unit test would do corresponding texImage2D calls hitting the cases that were improperly handled, as described in comment 0.
Comment 6 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-01-04 12:59:57 PST
I defer to your judgement to whether or not a unit test is useful here. Without it, this fix will likely not be verifiable.

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