1394265 - 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

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect, P1)

Description

[Nicholas Nethercote [inactive]]

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?

Comment 1

[Peter Chang[:pchang]]

These crashes happened from build 20170825100126 on linux platform only.

Comment 2

[Peter Chang[:pchang]]

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

Comment 3

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 1394265 - LOCAL_GL_MAX_COMBINED_TEXTURE_IMAGE_UNITS returns negative value, causing OOM crash.

Review commit: https://reviewboard.mozilla.org/r/174840/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174840/

Comment 4

[Chung-Sheng Fu [:cfu]]

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.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1394265 - Don't allow GL version <200. -

Review commit: https://reviewboard.mozilla.org/r/175080/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/175080/

Comment 6

[Kelsey Gilbert [:jgilbert]]

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.

Comment 7

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8903286 [details]
Bug 1394265 - Don't allow GL version <200. -

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

LGTM

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a32be8f5350
Don't allow GL version <200. - r=daoshengmu

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8a32be8f5350

Comment 10

[Andrew Osmond [:aosmond] (he/him)]

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

Comment 11

[Nicholas Nethercote [inactive]]

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

Oh snap! 0xe5 is our poison-after-free value. So this suggests a UAF.

Comment 12

[Kelsey Gilbert [:jgilbert]]

Attachment: 0001-Bug-1394265-Set-MAX_COMBINED_TEXTURE_IMAGE_UNITS-to-.patch

This should really never happen, but I guess there are some terrible ancient GL drivers out there.

Comment 13

[Daosheng Mu[:daoshengmu]]

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

Comment 14

[Kelsey Gilbert [:jgilbert]]

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.

Comment 15

[Daniel Veditz [:dveditz]]

(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?

Comment 16

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 17

[Kelsey Gilbert [:jgilbert]]

(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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/34bd2d1df418b4559b7d42c4298f5904aed20c33

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

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!

Comment 21

[Daosheng Mu[:daoshengmu]]

I would like to let jgilbert to do this uplift. Please try to ping him on irc.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Milan, as Jeff seems to be away, could you please help?
Thanks

Comment 23

[Kelsey Gilbert [:jgilbert]]

Please email me next time.

Comment 24

[Kelsey Gilbert [:jgilbert]]

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

Comment 25

[Milan Sreckovic [:milan] (needinfo for best results)]

(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 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8908923 [details] [diff] [review]
0001-Bug-1394265-Set-MAX_COMBINED_TEXTURE_IMAGE_UNITS-to-.patch

Sec-high, Beta57+, ESR52.5+

Comment 27

[Kelsey Gilbert [:jgilbert]]

Attachment: (esr52) 0001-Bug-1394265-Set-MAX_COMBINED_TEXTURE_IMAGE_UNITS-to-.patch

Comment 28

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0cc2943b6df8540cddc1422d56251fbaebc19ae2
https://hg.mozilla.org/releases/mozilla-esr52/rev/2b30335d0b95fb27ee28fa028ed4e1b493262c65