Closed Bug 1394265 Opened 2 years ago Closed 2 years ago

Crash in OOM | large | NS_ABORT_OOM | nsTArray_base<T>::EnsureCapacity<T> | nsTArray_base<T>::InsertSlotsAt<T> | nsTArray_Impl<T>::SetLength<T> | mozilla::WebGLContext::InitAndValidateGL

Categories

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

Unspecified
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 57+ fixed
firefox56 --- wontfix
firefox57 + fixed
firefox58 + fixed

People

(Reporter: njn, Assigned: jgilbert)

Details

(4 keywords, Whiteboard: [gfx-noted][adv-main57+][adv-esr52.5+][post-critsmash-triage])

Crash Data

Attachments

(3 files, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-3d809a0d-8c3a-42a4-a04d-70cea0170826.
=============================================================

This is an OOM|large signature so the obvious response is to make the allocation in question fallible.

*However*, all crash reports I looked at (across multiple installations) are requesting 30,856,392,488 bytes (28.74 GiB). In hex, that value is 0x72f2f2f28 which kinda looks like a pointer. So perhaps something funky is going on.

jgilbert, any ideas?
Flags: needinfo?(jgilbert)
These crashes happened from build 20170825100126 on linux platform only.
Based on the following commit changes between build 20170825100126 and build 20170824100243. I suspected Bug 1217290 might be related. Chung-Sheng, any ideas? 


https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d1c70c20e7&tochange=2306e153f
Flags: needinfo?(cfu)
Attachment #8903062 - Flags: review?(jgilbert)
It seems that somehow LOCAL_GL_MAX_COMBINED_TEXTURE_IMAGE_UNITS returns a negative value.  Previously mGLMaxTextureUnits was int32_t so it could satisfy the condition mGLMaxTextureUnits < 8 for the failure case.  But it is now uint32_t so the value becomes a very large number and the condition never fails.
Assignee: nobody → cfu
Flags: needinfo?(cfu)
Comment on attachment 8903062 [details]
Bug 1394265 - LOCAL_GL_MAX_COMBINED_TEXTURE_IMAGE_UNITS returns negative value, causing OOM crash.

https://reviewboard.mozilla.org/r/174840/#review180138

This should only fail on GL 1.x. We should try just being more explicit about requiring GL 2.0.
Attachment #8903062 - Flags: review?(jgilbert) → review-
Assignee: cfu → jgilbert
Flags: needinfo?(jgilbert)
Comment on attachment 8903286 [details]
Bug 1394265 - Don't allow GL version <200. -

https://reviewboard.mozilla.org/r/175080/#review180236

LGTM
Attachment #8903286 - Flags: review?(dmu) → review+
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a32be8f5350
Don't allow GL version <200. - r=daoshengmu
https://hg.mozilla.org/mozilla-central/rev/8a32be8f5350
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
This is still happening on today's nightly. Are we sure that OpenGL is returning a value at all?

30,856,392,488 / 8 (sizeof the WebGLRetPtr = 3857049061 = 0xe5e5e5e5

Seems to me that mGLMaxTextureUnits is not assigned a default value, and gl->GetUIntegerv(LOCAL_GL_MAX_COMBINED_TEXTURE_IMAGE_UNITS, &mGLMaxTextureUnits) fails to populate it.

https://crash-stats.mozilla.com/report/index/d42cb26f-8cf0-483b-b8ab-f13840170915
https://crash-stats.mozilla.com/report/index/00083e9c-c5ae-44ca-8a26-6266d0170915
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Priority: -- → P1
Whiteboard: [gfx-noted]
> 30,856,392,488 / 8 (sizeof the WebGLRetPtr = 3857049061 = 0xe5e5e5e5

Oh snap! 0xe5 is our poison-after-free value. So this suggests a UAF.
Group: core-security
This should really never happen, but I guess there are some terrible ancient GL drivers out there.
Attachment #8903062 - Attachment is obsolete: true
Attachment #8908923 - Flags: review?(dmu)
Comment on attachment 8908923 [details] [diff] [review]
0001-Bug-1394265-Set-MAX_COMBINED_TEXTURE_IMAGE_UNITS-to-.patch

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

LGTM
Attachment #8908923 - Flags: review?(dmu) → review+
Group: core-security → gfx-core-security
Comment on attachment 8908923 [details] [diff] [review]
0001-Bug-1394265-Set-MAX_COMBINED_TEXTURE_IMAGE_UNITS-to-.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Hard.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No, it's at least one indirection away.

Which older supported branches are affected by this flaw?
All.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Simple, low risk.

How likely is this patch to cause regressions; how much testing does it need?
Very low risk. We already have general tests to ensure continued correctness.
Attachment #8908923 - Flags: sec-approval?
(In reply to Jeff Gilbert [:jgilbert] from comment #14)
> Which older supported branches are affected by this flaw?
> All.

Why does this stack appear to be new in crash-stats, showing up first on Aug 26 or so? Clearing RyanVM's "unaffected" markings while we clear that up. Maybe we do need this on ESR?
sec-approval for trunk checkin on October 9, two weeks into the cycle. Do not check this in before that date, please.

We'll want a beta patch made and nominated for 57 and ESR52 at that time.
Whiteboard: [gfx-noted] → [gfx-noted][checkin on 10/9]
Attachment #8908923 - Flags: sec-approval? → sec-approval+
(In reply to Daniel Veditz [:dveditz] from comment #15)
> (In reply to Jeff Gilbert [:jgilbert] from comment #14)
> > Which older supported branches are affected by this flaw?
> > All.
> 
> Why does this stack appear to be new in crash-stats, showing up first on Aug
> 26 or so? Clearing RyanVM's "unaffected" markings while we clear that up.
> Maybe we do need this on ESR?

It's possible some recent change exposed this, but IIRC this is old code.
https://hg.mozilla.org/integration/mozilla-inbound/rev/34bd2d1df418b4559b7d42c4298f5904aed20c33
Whiteboard: [gfx-noted][checkin on 10/9] → [gfx-noted]
Target Milestone: mozilla57 → ---
https://hg.mozilla.org/mozilla-central/rev/34bd2d1df418

Please request Beta and ESR52 approval on this when you get a chance. Note that it grafts cleanly to Beta but will require a rebased patch for ESR52.
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Jeff or Daosheng, can you help out with a uplift requests & esr patch? We could still get this into tomorrow's beta 8 build if it lands by tomorrow morning. Thanks!
Flags: needinfo?(dmu)
I would like to let jgilbert to do this uplift. Please try to ping him on irc.
Flags: needinfo?(dmu)
Group: gfx-core-security → core-security-release
Milan, as Jeff seems to be away, could you please help?
Thanks
Flags: needinfo?(milan)
Please email me next time.
Flags: needinfo?(milan)
Comment on attachment 8908923 [details] [diff] [review]
0001-Bug-1394265-Set-MAX_COMBINED_TEXTURE_IMAGE_UNITS-to-.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: sec-high
Fix Landed on Version: 58
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: driver bugs
[User impact if declined]: sec-high
[Is this code covered by automated tests?]: not worth adding tests
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: no
[Why is the change risky/not risky?]: it's simple
[String changes made/needed]: none
Flags: needinfo?(jgilbert)
Attachment #8908923 - Flags: approval-mozilla-esr52?
Attachment #8908923 - Flags: approval-mozilla-beta?
(In reply to Ryan VanderMeulen [:RyanVM] from comment #19)
> https://hg.mozilla.org/mozilla-central/rev/34bd2d1df418
> 
> Please request Beta and ESR52 approval on this when you get a chance. Note
> that it grafts cleanly to Beta but will require a rebased patch for ESR52.

I'm guessing we need a different patch for 52?
Comment on attachment 8908923 [details] [diff] [review]
0001-Bug-1394265-Set-MAX_COMBINED_TEXTURE_IMAGE_UNITS-to-.patch

Sec-high, Beta57+, ESR52.5+
Attachment #8908923 - Flags: approval-mozilla-esr52?
Attachment #8908923 - Flags: approval-mozilla-esr52+
Attachment #8908923 - Flags: approval-mozilla-beta?
Attachment #8908923 - Flags: approval-mozilla-beta+
Whiteboard: [gfx-noted] → [gfx-noted][adv-main57+][adv-esr52.5+]
Flags: qe-verify-
Whiteboard: [gfx-noted][adv-main57+][adv-esr52.5+] → [gfx-noted][adv-main57+][adv-esr52.5+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.