Closed
Bug 807096
Opened 12 years ago
Closed 11 years ago
Texture memory exposure and abort() with sketch3d.com
Categories
(Core :: Graphics: CanvasWebGL, defect)
Tracking
()
People
(Reporter: jrmuizel, Assigned: milan)
Details
(Keywords: sec-high)
Attachments
(5 files, 5 obsolete files)
2.62 KB,
patch
|
milan
:
review+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
3.29 KB,
patch
|
milan
:
review+
|
Details | Diff | Splinter Review |
3.24 KB,
patch
|
milan
:
review+
|
Details | Diff | Splinter Review |
3.19 KB,
patch
|
milan
:
review+
milan
:
approval-mozilla-b2g18-
|
Details | Diff | Splinter Review |
189 bytes,
text/plain
|
abillings
:
approval-mozilla-aurora+
abillings
:
approval-mozilla-beta+
|
Details |
If I force Intel GPU on OS X 10.7 and go to the following url: https://sketchfab.com/show/hZimZzGxleN5z9GgguH5DgvbQDl I get a crash in glGenerateMipMap with a 4096x4096 texture. This also causes Chrome's gpu process to crash.
Comment 1•12 years ago
|
||
Critsmash Triag: Can we get more information or a dump of the crash? Can you propose a security rating?
Comment 2•12 years ago
|
||
Could this be related to bug 807741? Marking sec-high because driver crashes sound bad.
Keywords: sec-high
Comment 3•11 years ago
|
||
Jeff should we get someone on this?
Assignee | ||
Comment 4•11 years ago
|
||
For what it's worth, this doesn't crash for me on 10.8 (forcing Intel HD 4000) on 17.0.1 or nightly.
Assignee | ||
Comment 5•11 years ago
|
||
Can somebody CC me on (In reply to Andrew McCreight [:mccr8] from comment #2) > Could this be related to bug 807741? > > Marking sec-high because driver crashes sound bad. Can somebody with access CC me on 807741 that is possibly related to this?
Comment 6•11 years ago
|
||
(In reply to Milan Sreckovic [:milan] from comment #5) > > Can somebody with access CC me on 807741 that is possibly related to this? Done.
Assignee | ||
Comment 7•11 years ago
|
||
Jeff, when you get to it, could you try to reproduce to see if it still happens, or if it was taken care of by the blacklisting? If you still have 10.7.
Flags: needinfo?(jmuizelaar)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → jmuizelaar
Comment 8•11 years ago
|
||
In bug 737182 we limited the max texture size to 4k on Mac/Intel. Should we just further limit it to 2k?
Assignee | ||
Updated•11 years ago
|
Assignee: jmuizelaar → bjacob
Flags: needinfo?(jmuizelaar)
Assignee | ||
Comment 9•11 years ago
|
||
(In reply to Benoit Jacob [:bjacob] from comment #8) > In bug 737182 we limited the max texture size to 4k on Mac/Intel. > > Should we just further limit it to 2k? Are we sure this is because of the size? If so, I agree, let's lower the limit.
Comment 10•11 years ago
|
||
I don't really know; but since Jeff can reproduce, we could verify ourselves.
Assignee | ||
Comment 13•11 years ago
|
||
To summarize. This crash does not happen on 10.8. It does happen on 10.7. It does not crash on 10.7 if the Intel/Mac limit is lowered from 4096 to 2048, but the texturing is gone from the resulting 3d surface - the shape is there, but it's all black. On 10.8, it looks correct even with the limit at 2k.
Comment 14•11 years ago
|
||
Is this an Angle bug?
Comment 15•11 years ago
|
||
No, an ANGLE bug couldn't do that on Mac. (It could only on Windows, where ANGLE provides the entire GL implementation; on Mac it only provides the shader compiler).
Comment 16•11 years ago
|
||
(In reply to Milan Sreckovic [:milan] from comment #13) > To summarize. This crash does not happen on 10.8. It does happen on 10.7. > It does not crash on 10.7 if the Intel/Mac limit is lowered from 4096 to > 2048, but the texturing is gone from the resulting 3d surface - the shape is > there, but it's all black. On 10.8, it looks correct even with the limit at > 2k. OK, so let's limit the max texture size on Mac+Intel to 2K. Patch coming.
Comment 17•11 years ago
|
||
Attachment #739734 -
Flags: review?(jmuizelaar)
Reporter | ||
Updated•11 years ago
|
Attachment #739734 -
Flags: review?(jmuizelaar) → review+
Comment 18•11 years ago
|
||
Can we get this reviewed and checked in, if good?
Flags: needinfo?(jmuizelaar)
Comment 19•11 years ago
|
||
My bad, this *is* reviewed. Can we get this checked in?
Flags: needinfo?(jmuizelaar)
Comment 20•11 years ago
|
||
The patch has been reviewed - can it be checked in?
Flags: needinfo?(bjacob)
Assignee | ||
Comment 21•11 years ago
|
||
This got out of date, rebased with the latest, carrying over r=jmuizelaar. When landing, this is bjacob's patch.
Attachment #739734 -
Attachment is obsolete: true
Attachment #767363 -
Flags: review+
Assignee | ||
Comment 22•11 years ago
|
||
Modification to the original patch, to only restrict the texture size on Mac+Intel on < 10.8. On my 10.8 system, I do not run into this problem with Intel (or discrete) graphics. This way we'd have fewer restrictions.
Attachment #767369 -
Flags: review?(bjacob)
Updated•11 years ago
|
Flags: needinfo?(bjacob)
Comment 23•11 years ago
|
||
Comment on attachment 767369 [details] [diff] [review] Limit maximum texture size to 2048 on Mac+Intel < 10.8 Review of attachment 767369 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLContext.cpp @@ +603,5 @@ > + OSErr err2 = ::Gestalt(gestaltSystemVersionMinor, &minor); > + > + // For 2D textures, see bug 737182 for the original restriction to 4K and > + // see bug 807096 for why we further restricted it to 2K on < 10.8 > + // for good measure, we align renderbuffers on what we do for 2D textures Nitpicky nit: in multi-line, multi-sentence comments, punctuation/case becomes important. "for good measure..." is now the start of a new sentence.
Attachment #767369 -
Flags: review?(bjacob) → review+
Assignee | ||
Comment 24•11 years ago
|
||
Carrying over r=bjacob addressing a nit. Try run here https://tbpl.mozilla.org/?tree=Try&rev=8e661b60a4af
Attachment #767363 -
Attachment is obsolete: true
Attachment #767369 -
Attachment is obsolete: true
Attachment #767876 -
Flags: review+
Assignee | ||
Comment 25•11 years ago
|
||
Comment on attachment 767876 [details] [diff] [review] Limit maximum texture size to 2048 on Mac+Intel < 10.8 [Security approval request comment] How easily could an exploit be constructed based on the patch? One could trigger the problem easily based on the fix, with the large texture. However, there is no help as to how to exploit it. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. Which older supported branches are affected by this flaw? All. If not all supported branches, which bug introduced the flaw? n/a Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Backports would need to be created, but they are trivial, just accounting for the recent changes. How likely is this patch to cause regressions; how much testing does it need? There may be performance regressions because some of the larger textures are now handled in software.
Attachment #767876 -
Flags: sec-approval?
Comment 26•11 years ago
|
||
Comment on attachment 767876 [details] [diff] [review] Limit maximum texture size to 2048 on Mac+Intel < 10.8 sec-approval+ for trunk. Please also prepare and nominate patches for aurora and beta once this has landed on trunk.
Attachment #767876 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 27•11 years ago
|
||
Checkin-needed - for trunk, with the attached patch. I will prepare the aurora and beta versions.
Keywords: checkin-needed
Assignee | ||
Comment 28•11 years ago
|
||
Aurora version of the patch. Only picking up the Intel check.
Attachment #768511 -
Flags: review?(bjacob)
Assignee | ||
Comment 29•11 years ago
|
||
Beta matching patch. Only picking up the Intel check.
Attachment #768513 -
Flags: review?(bjacob)
Updated•11 years ago
|
Attachment #768511 -
Flags: review?(bjacob) → review+
Updated•11 years ago
|
Attachment #768513 -
Flags: review?(bjacob) → review+
Assignee | ||
Updated•11 years ago
|
Attachment #768511 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #768513 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #768511 -
Attachment is obsolete: false
Assignee | ||
Updated•11 years ago
|
Attachment #768513 -
Attachment is obsolete: false
Assignee | ||
Comment 30•11 years ago
|
||
Proper Aurora matching patch, carrying over r+ from bjacob.
Attachment #768511 -
Attachment is obsolete: true
Attachment #768566 -
Flags: review+
Assignee | ||
Comment 31•11 years ago
|
||
Proper Beta patch, carrying over r+ from bjacob.
Attachment #768513 -
Attachment is obsolete: true
Attachment #768567 -
Flags: review+
Assignee | ||
Comment 32•11 years ago
|
||
Comment on attachment 768566 [details] [diff] [review] Aurora matching patch [Security approval request comment] See the m-c comment. The same applies here.
Attachment #768566 -
Flags: sec-approval?
Assignee | ||
Comment 33•11 years ago
|
||
Comment on attachment 768567 [details] [diff] [review] Beta matching patch [Security approval request comment] See the m-c comment. The same applies here.
Attachment #768567 -
Flags: sec-approval?
Comment 34•11 years ago
|
||
Comment on attachment 768566 [details] [diff] [review] Aurora matching patch sec-approval? is only for trunk. You need to ask for approval-mozilla-aurora and beta and fill out the form for those.
Attachment #768566 -
Flags: sec-approval?
Updated•11 years ago
|
Attachment #768567 -
Flags: sec-approval?
Assignee | ||
Comment 35•11 years ago
|
||
Comment on attachment 768566 [details] [diff] [review] Aurora matching patch [Approval Request Comment] Bug caused by (feature/regressing bug #): n/a User impact if declined: sec-high in the wild Testing completed (on m-c, etc.): yes Risk to taking this patch (and alternatives if risky): low String or IDL/UUID changes made by this patch: n/a
Attachment #768566 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 36•11 years ago
|
||
Comment on attachment 768567 [details] [diff] [review] Beta matching patch [Approval Request Comment] Bug caused by (feature/regressing bug #): n/a User impact if declined: sec-high in the wild Testing completed (on m-c, etc.): yes Risk to taking this patch (and alternatives if risky): low String or IDL/UUID changes made by this patch: n/a
Attachment #768567 -
Flags: approval-mozilla-beta?
Comment 37•11 years ago
|
||
Comment on attachment 768566 [details] [diff] [review] Aurora matching patch Low risk sec-high, approving for branches
Attachment #768566 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
Attachment #768567 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•11 years ago
|
status-firefox22:
--- → wontfix
status-firefox23:
--- → affected
status-firefox24:
--- → affected
status-firefox25:
--- → affected
Comment 38•11 years ago
|
||
Do we need this on ESR17 or B2G18?
Updated•11 years ago
|
tracking-b2g18:
--- → ?
tracking-firefox-esr17:
--- → ?
Assignee | ||
Comment 39•11 years ago
|
||
(In reply to Alex Keybl [:akeybl] from comment #38) > Do we need this on ESR17 or B2G18? This is OS X only code change, so we don't need it for B2G18. I'll attach the patch for ESR17 - do we uplift sec-high as a matter of course? I'll ask for approval, so that we can decide. Checkin-needed for central, aurora and beta for now.
Assignee | ||
Comment 40•11 years ago
|
||
[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: 23 (while in beta) Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: n/a See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #769414 -
Flags: review+
Attachment #769414 -
Flags: approval-mozilla-esr17?
Attachment #769414 -
Flags: approval-mozilla-b2g18-
Comment 41•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/821104668b01 We'll get the branch uplifts once this hits m-c.
Keywords: checkin-needed
Comment 42•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/821104668b01
Assignee: bjacob → milan
Status: NEW → RESOLVED
Closed: 11 years ago
status-b2g18:
--- → wontfix
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → wontfix
status-b2g-v1.1hd:
--- → wontfix
status-firefox-esr17:
--- → affected
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Comment 43•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/c921b322bd98 https://hg.mozilla.org/releases/mozilla-beta/rev/7dcf4ff8fb59
Comment 44•11 years ago
|
||
Just talked to Milan and Benoit about this. The patch causes my nightly to render completely white if the window is larger than 2048px, which it typically is.
Assignee | ||
Comment 45•11 years ago
|
||
Opened bug 891568 for this.
Comment 46•11 years ago
|
||
Matt, can you please test to verify this is fixed?
Keywords: verifyme
QA Contact: mwobensmith
Reporter | ||
Comment 47•11 years ago
|
||
I can no longer reproduce this problem with the limit set back to 4096. I suggest we back it all out to avoid causing bug 891568.
Comment 48•11 years ago
|
||
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/e35388d6d558 We should back this out of mozilla-aurora and mozilla-beta too. Milan, I'll leave that to you.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 49•11 years ago
|
||
Backing out the change.
Attachment #774261 -
Flags: sec-approval?
Attachment #774261 -
Flags: approval-mozilla-beta?
Attachment #774261 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•11 years ago
|
Attachment #769414 -
Flags: approval-mozilla-esr17?
Assignee | ||
Comment 50•11 years ago
|
||
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/e35388d6d558
Assignee | ||
Comment 51•11 years ago
|
||
Comment on attachment 774261 [details] Request to back out the changes on uplift branches. >The original problem is no longer reproducible even without the fix, and the fix caused bug 891568. It has been backed out on inbound, would like this backed out of beta and aurora as well.
Comment 52•11 years ago
|
||
Comment on attachment 774261 [details]
Request to back out the changes on uplift branches.
Well, you already backed it out so you don't need sec-approval now...
Really, though, the main purpose of sec-approval is to avoid accidental 0 days and exposure. Since this is already in the trees, I'm less concerned (yes, I recognize that any change can draw attention).
I expect that release management will approve the backout unless it is dangerous somehow. I assume it is not?
Attachment #774261 -
Flags: sec-approval?
Assignee | ||
Comment 53•11 years ago
|
||
The backout on alpha/beta gets us back to where we were a few days ago, and we can't reproduce the original issue, so, yes, I'd say it's safe.
Comment 55•11 years ago
|
||
Comment on attachment 774261 [details]
Request to back out the changes on uplift branches.
Giving a + for branches.
Attachment #774261 -
Flags: approval-mozilla-beta?
Attachment #774261 -
Flags: approval-mozilla-beta+
Attachment #774261 -
Flags: approval-mozilla-aurora?
Attachment #774261 -
Flags: approval-mozilla-aurora+
Comment 56•11 years ago
|
||
This never made it to b2g, did it? If not, we don't need to track it there.
Assignee | ||
Comment 57•11 years ago
|
||
(In reply to Al Billings [:abillings] from comment #56) > This never made it to b2g, did it? If not, we don't need to track it there. Right - it never made it to B2G.
Assignee | ||
Comment 58•11 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #43) > https://hg.mozilla.org/releases/mozilla-aurora/rev/c921b322bd98 > https://hg.mozilla.org/releases/mozilla-beta/rev/7dcf4ff8fb59 With the approval given in comment 55, tagging checkin-needed for the backout on these two branches.
Keywords: checkin-needed
Updated•11 years ago
|
tracking-b2g18:
? → ---
Comment 59•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/2c68df0e3a54 https://hg.mozilla.org/releases/mozilla-beta/rev/44bb8aa083d1
Keywords: checkin-needed
Target Milestone: mozilla25 → ---
Assignee | ||
Comment 60•11 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #47) > I can no longer reproduce this problem with the limit set back to 4096. I > suggest we back it all out to avoid causing bug 891568. Now that we've done the backout, marking "works for me"
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → WORKSFORME
Comment 61•11 years ago
|
||
Comment on attachment 768566 [details] [diff] [review] Aurora matching patch Per comment 49, this isn't landing on Aurora at this point.
Attachment #768566 -
Flags: approval-mozilla-aurora+
Comment 62•11 years ago
|
||
Comment on attachment 768567 [details] [diff] [review] Beta matching patch Per comment 49, this isn't landing on Beta at this point.
Attachment #768567 -
Flags: approval-mozilla-beta+
Updated•11 years ago
|
Updated•11 years ago
|
status-firefox-esr24:
--- → wontfix
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•