Closed Bug 966630 Opened 6 years ago Closed 6 years ago

WebGL crash [@mozilla::gl::GLContext::fCompressedTexImage2D / gleEvaluateTextureImageChange]

Categories

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

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla31
Tracking Status
firefox27 --- wontfix
firefox28 --- wontfix
firefox29 + verified
firefox30 + verified
firefox31 --- verified
firefox-esr24 29+ verified
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: posidron, Assigned: kamidphish)

References

(Blocks 1 open bug)

Details

(Keywords: crash, sec-high, testcase, Whiteboard: [adv-main29+][adv-esr24.5+])

Crash Data

Attachments

(4 files)

Attached file callstack
Doesn't seem to be the driver (crashes on Windows and Mac).
Keywords: sec-high
Dan, does this fall under the "refactoring so that we don't have to do them one by one"?
Assignee: nobody → dglastonbury
Milan, yes. This is the one that I started on.
I've fixed this in the work I've done for 966624, so adding a dependency.

Instead of crashing, now detects invalid level. From console:
  "Error: WebGL: compressedTexImage2D: level > maximum texture level"
Depends on: 966624
Fixed by Bug 966624.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Did this only affect trunk (Firefox 30)?
Flags: needinfo?(dglastonbury)
I tested again the following and all are affected:
 * aurora
 * ff-27
 * ff-24esr
Flags: needinfo?(dglastonbury)
We should get this fixed on Aurora, Beta, and ESR24.
Can we get a nom for uplift in Bug 966624 in order to for the fix to make into into ESR24?
Dan - Can you handle the nom and uplift for ESR24?
Flags: needinfo?(dglastonbury)
I'm not comfortable uplifting the patches from Bug 966624 to ESR24. I'll look at creating a fix for just this specific issue.
Flags: needinfo?(dglastonbury)
Patch to fix the issue in ESR 24 instead of uplifting the whole refactoring of texture image parameters.
Attachment #8405996 - Flags: review?(bjacob)
Comment on attachment 8405996 [details] [diff] [review]
Patch for FF ESR 24

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Ability to crash browser with bad WebGL parameters
Fix Landed on Version: FF30
Risk to taking this patch (and alternatives if risky): This patch is very small and easy to code review. The risk is negligible.
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8405996 - Flags: approval-mozilla-esr24?
Attachment #8405996 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
Approved for ESR assuming it passed review.
What about beta29?
Flags: needinfo?(dglastonbury)
Comment on attachment 8405996 [details] [diff] [review]
Patch for FF ESR 24

Review of attachment 8405996 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/WebGLContextValidate.cpp
@@ +512,5 @@
> +     * expecting.
> +     */
> +
> +    if (level > 31)
> +        level = 31;

Good, and here is another justification for this good change: GLsizei is int32_t, so it can't exceed 2^31 - 1, so, even if we allowed NPOT cubemaps, there can't be a texture level > 30  (or else, texture mipmap completeness rules are violated, and we fake the sampling as a black texture).
Attachment #8405996 - Flags: review?(bjacob) → review+
Attachment #8405996 - Flags: review?(jgilbert)
Flags: needinfo?(dglastonbury)
Attachment #8405996 - Flags: review?(jgilbert) → review+
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #16)
> What about beta29?

This patch can be applied to Beta 29.
Comment on attachment 8405996 [details] [diff] [review]
Patch for FF ESR 24

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Ability to crash browser with bad WebGL parameters
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): This patch is very small and easy to code review. The risk is negligible.
String or IDL/UUID changes made by this patch:
Attachment #8405996 - Flags: approval-mozilla-beta?
Attachment #8405996 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attached patch Aurora patchSplinter Review
Ported patch for ESR24 to Aurora.
Attachment #8406629 - Flags: review?(jgilbert)
Comment on attachment 8406629 [details] [diff] [review]
Aurora patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Ability to crash firefox with bad WebGL parameters.
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): Very low risk. Patch is easy to code review.
String or IDL/UUID changes made by this patch:
Attachment #8406629 - Flags: approval-mozilla-aurora?
I'm confused, I thought this was fixed on Aurora by bug 966624? Does that mean that trunk (31) needs this fix as well?
Attachment #8406629 - Flags: review?(jgilbert) → review+
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #22)
> I'm confused, I thought this was fixed on Aurora by bug 966624? Does that
> mean that trunk (31) needs this fix as well?

Between comment 5 and my testing yesterday the bug regressed in m-c trunk.
Bear with me because I'm new to this, but who does the check-in of patch to ESR24, Aurora and Beta? Am I supposed to? And why in my name against the a? for Aurora?
(In reply to Dan Glastonbury :djg :kamidphish from comment #25)
> Bear with me because I'm new to this, but who does the check-in of patch to
> ESR24, Aurora and Beta? Am I supposed to? And why in my name against the a?
> for Aurora?

Your name is against the a? because you nominated it for Aurora approval?

Either you check it in or you get a sheriff like Ryan to do so.
Attachment #8406629 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [adv-main29+][adv-esr24.5+]
Confirmed crash on ASan Fx29, 2013-12-19.
Verified fix on ASan Fx29, Fx30, Fx31, 2014-04-17.
Verified fix on ASan Fx24esr, 2014-04-19.
Status: RESOLVED → VERIFIED
Group: core-security
You need to log in before you can comment on or make changes to this bug.