Closed
Bug 1225250
Opened 9 years ago
Closed 8 years ago
«failed assertion "fPixelRef->rowBytes() == fRowBytes"» with canvas, shadow, drawImage(HTMLVideoElement)
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
People
(Reporter: jruderman, Assigned: lsalzman)
References
Details
(4 keywords, Whiteboard: [gfx-noted][post-critsmash-triage][adv-main43+][adv-esr38.5+])
Crash Data
Attachments
(5 files, 1 obsolete file)
459 bytes,
text/html
|
Details | |
13.65 KB,
text/plain
|
Details | |
7.32 KB,
text/plain
|
Details | |
1.21 KB,
patch
|
jrmuizel
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
752 bytes,
patch
|
lsalzman
:
review+
ritu
:
approval-mozilla-esr38+
cbook
:
checkin+
|
Details | Diff | Splinter Review |
Debug: gfx/skia/skia/src/core/SkBitmap.cpp:1343: failed assertion "fPixelRef->rowBytes() == fRowBytes" ASan: SEGV on unknown address [@ S16_opaque_D32_filter_DX] Nightly: bp-66bed0f0-5e76-458d-a14b-e8d112151116 [@ S16_opaque_D32_filter_DX]
Reporter | ||
Comment 1•9 years ago
|
||
Reporter | ||
Comment 2•9 years ago
|
||
Reporter | ||
Updated•9 years ago
|
Attachment #8688051 -
Attachment description: testcase (crashes Firefox) → testcase (crashes Firefox) (requires path adjustment)
Reporter | ||
Updated•9 years ago
|
Attachment #8688053 -
Attachment description: stack (debug, llvm) → stack (debug, lldb)
Assignee | ||
Comment 3•9 years ago
|
||
Is the frames.webm file which is not provided in this testcase at all actually required to make the testcase work?
Flags: needinfo?(jruderman)
Updated•9 years ago
|
Whiteboard: [gfx-noted]
Reporter | ||
Comment 4•9 years ago
|
||
It seems to need a video file, but it doesn't have to be layout/reftests/webm-video/frames.webm. You can use dom/media/test/320x240.ogv instead if you prefer. I tried with an image instead and it didn't crash.
Flags: needinfo?(jruderman)
Assignee | ||
Comment 5•9 years ago
|
||
Row bytes is supposed to be just the row stride. For some reason the code here was supplying the size of the entire image. Just using the default row bytes calculation here fixes it.
Assignee | ||
Comment 6•9 years ago
|
||
Initialize format and stride, as they were seemingly not being initialized at all and should have been.
Attachment #8688152 -
Attachment is obsolete: true
Attachment #8688152 -
Flags: review?(jmuizelaar)
Attachment #8688489 -
Flags: review?(jmuizelaar)
Updated•8 years ago
|
Attachment #8688489 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 7•8 years ago
|
||
Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=14a215606be3
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8688489 [details] [diff] [review] fix stride on SourceSurfaceSkia when initialized from GPU texture [Security approval request comment] > How easily could an exploit be constructed based on the patch? The testcase is at least non-obvious and requires interaction with a video to work, which is not specifically highlighted in the patch. Further, this assert is only ever triggered in debug builds, so probably doesn't affect most users. > 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? The regression was introduced to central on Nov 11, 2014. > If not all supported branches, which bug introduced the flaw? https://hg.mozilla.org/mozilla-central/rev/2f830555f138 > Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Patch should theoretically apply against work in 39+. > How likely is this patch to cause regressions; how much testing does it need? Very unlikely. It should only increase correctness vs. allowing the assertion to trigger.
Attachment #8688489 -
Flags: sec-approval?
Comment 9•8 years ago
|
||
sec-approval+ for trunk. We should take patches on Aurora, Beta, and ESR38 for this issue. Can you nominate the existing patch for Beta and Aurora and do an ESR38 patch?
status-firefox42:
--- → wontfix
status-firefox43:
--- → affected
status-firefox44:
--- → affected
status-firefox-esr38:
--- → affected
tracking-firefox43:
--- → +
tracking-firefox44:
--- → +
tracking-firefox45:
--- → +
tracking-firefox-esr38:
--- → 43+
Updated•8 years ago
|
Attachment #8688489 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 10•8 years ago
|
||
Comment on attachment 8688489 [details] [diff] [review] fix stride on SourceSurfaceSkia when initialized from GPU texture Approval Request Comment [Feature/regressing bug #]: Bug 880114 [User impact if declined]: Assertions triggered in debug builds [Describe test coverage new/current, TreeHerder]: Mochitest, reftest [Risks and why]: Should be innocuous, as it only fixes an assertion trigger but otherwise doesn't change SourceSurface behavior. [String/UUID change made/needed]: None
Attachment #8688489 -
Flags: approval-mozilla-beta?
Attachment #8688489 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 11•8 years ago
|
||
Comment on attachment 8688489 [details] [diff] [review] fix stride on SourceSurfaceSkia when initialized from GPU texture Fix for debug build issue, ok for uplift to beta and aurora
Attachment #8688489 -
Flags: approval-mozilla-beta?
Attachment #8688489 -
Flags: approval-mozilla-beta+
Attachment #8688489 -
Flags: approval-mozilla-aurora?
Attachment #8688489 -
Flags: approval-mozilla-aurora+
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d2541ca9d8b
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8d2541ca9d8b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment 16•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/79c9f80eeeb8
status-b2g-v2.5:
--- → fixed
Comment 17•8 years ago
|
||
This should be fixed on ESR38 unless there is a reason not to as I mention in comment 9. We need this soon as well.
Flags: needinfo?(lsalzman)
Updated•8 years ago
|
Whiteboard: [gfx-noted] → [gfx-noted][post-critsmash-triage]
Assignee | ||
Comment 18•8 years ago
|
||
[Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: User impact if declined: Assertions triggered in debug builds Fix Landed on Version: 43+ Risk to taking this patch (and alternatives if risky): Should be innocuous, as it only fixes an assertion trigger but otherwise doesn't change SourceSurface behavior. String or UUID changes made by this patch: None See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Flags: needinfo?(lsalzman)
Attachment #8695425 -
Flags: review+
Attachment #8695425 -
Flags: approval-mozilla-esr38?
Updated•8 years ago
|
Group: gfx-core-security → core-security-release
Comment 19•8 years ago
|
||
ESR38 needs approval to land from release management.
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Comment 20•8 years ago
|
||
Sylvestre will be handling esr38 on Monday, I think.
Flags: needinfo?(lhenry)
Comment 21•8 years ago
|
||
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #20) > Sylvestre will be handling esr38 on Monday, I think. Wouldn't it be better to approve things this week?
(In reply to Al Billings [:abillings] from comment #21) > (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #20) > > Sylvestre will be handling esr38 on Monday, I think. > > Wouldn't it be better to approve things this week? Given that this patch has been in all other branches for a while without any known negative impact, I will take it on behalf of Sylvestre today as Al is right about next week being a bit chaotic and busy.
Flags: needinfo?(rkothari)
Comment on attachment 8695425 [details] [diff] [review] fix stride on SourceSurfaceSkia when initialized from GPU texture (esr38) This patch looks exactly similar to the m-c, m-b and m-a patch. Given that this fix has been on those channels for almost 3 weeks now and it fixes a sec-high, approving for uplift to esr38 on Sylvestre's behalf.
Attachment #8695425 -
Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Reporter | ||
Comment 24•8 years ago
|
||
I can still reproduce the assertion on mozilla-central. The ASan "SEGV on unknown address" seems to be gone, though. Did they turn out to be two separate bugs?
Comment 25•8 years ago
|
||
This is ESR38 approved but not checked in yet. Can you check this in soon?
Flags: needinfo?(lsalzman)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(lsalzman)
Attachment #8695425 -
Flags: checkin?
Updated•8 years ago
|
Keywords: checkin-needed
Comment 26•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-esr38/rev/008197b642e5
status-thunderbird_esr38:
--- → fixed
Keywords: checkin-needed
Updated•8 years ago
|
Whiteboard: [gfx-noted][post-critsmash-triage] → [gfx-noted][post-critsmash-triage][adv-main43+][adv-esr38.5+]
Updated•8 years ago
|
Attachment #8695425 -
Flags: checkin? → checkin+
Reporter | ||
Comment 27•8 years ago
|
||
(In reply to Jesse Ruderman from comment #24) > I can still reproduce the assertion on mozilla-central. The ASan "SEGV on > unknown address" seems to be gone, though. Did they turn out to be two > separate bugs? Now the assertion is gone, but I get another ASan crash! Filed bug 1235469.
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•