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)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox42 --- wontfix
firefox43 + fixed
firefox44 + fixed
firefox45 + fixed
firefox-esr38 43+ fixed
b2g-v2.5 --- fixed
thunderbird_esr38 --- fixed

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)

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]
Attached file stack (debug, lldb)
Attachment #8688051 - Attachment description: testcase (crashes Firefox) → testcase (crashes Firefox) (requires path adjustment)
Attachment #8688053 - Attachment description: stack (debug, llvm) → stack (debug, lldb)
Is the frames.webm file which is not provided in this testcase at all actually required to make the testcase work?
Flags: needinfo?(jruderman)
Whiteboard: [gfx-noted]
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)
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: nobody → lsalzman
Status: NEW → ASSIGNED
Attachment #8688152 - Flags: review?(jmuizelaar)
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)
Attachment #8688489 - Flags: review?(jmuizelaar) → review+
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?
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?
Attachment #8688489 - Flags: sec-approval? → sec-approval+
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?
Keywords: checkin-needed
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+
https://hg.mozilla.org/mozilla-central/rev/8d2541ca9d8b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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)
Whiteboard: [gfx-noted] → [gfx-noted][post-critsmash-triage]
[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?
Group: gfx-core-security → core-security-release
ESR38 needs approval to land from release management.
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Sylvestre will be handling esr38 on Monday, I think.
Flags: needinfo?(lhenry)
(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+
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?
This is ESR38 approved but not checked in yet. Can you check this in soon?
Flags: needinfo?(lsalzman)
Flags: needinfo?(lsalzman)
Attachment #8695425 - Flags: checkin?
Whiteboard: [gfx-noted][post-critsmash-triage] → [gfx-noted][post-critsmash-triage][adv-main43+][adv-esr38.5+]
Attachment #8695425 - Flags: checkin? → checkin+
(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.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.