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)
: Milan Sreckovic [:milan]
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 | Splinter Review

Description User image 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 User image Benoit Jacob [:bjacob] (mostly away) 2011-11-28 11:38:18 PST
Comment 3 User image 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 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-28 13:38:59 PST
Is this something QA can verify?
Comment 5 User image 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 User image 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.