«failed assertion "fPixelRef->rowBytes() == fRowBytes"» with canvas, shadow, drawImage(HTMLVideoElement)

RESOLVED FIXED in Firefox 43

Status

()

defect
--
critical
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jruderman, Assigned: lsalzman)

Tracking

(Blocks 2 bugs, 4 keywords)

Trunk
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 wontfix, firefox43+ fixed, firefox44+ fixed, firefox45+ fixed, firefox-esr3843+ fixed, b2g-v2.5 fixed, thunderbird_esr38 fixed)

Details

(Whiteboard: [gfx-noted][post-critsmash-triage][adv-main43+][adv-esr38.5+], crash signature)

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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

4 years ago
Posted file stack (debug, lldb)
(Reporter)

Comment 2

4 years ago
(Reporter)

Updated

4 years ago
Attachment #8688051 - Attachment description: testcase (crashes Firefox) → testcase (crashes Firefox) (requires path adjustment)
(Reporter)

Updated

4 years ago
Attachment #8688053 - Attachment description: stack (debug, llvm) → stack (debug, lldb)
(Assignee)

Comment 3

4 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)
Whiteboard: [gfx-noted]
(Reporter)

Comment 4

4 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

4 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: nobody → lsalzman
Status: NEW → ASSIGNED
Attachment #8688152 - Flags: review?(jmuizelaar)
(Assignee)

Comment 6

4 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)
Keywords: sec-high
Attachment #8688489 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 8

4 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?
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+
(Assignee)

Comment 10

4 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

4 years ago
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
Last Resolved: 4 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]
(Assignee)

Comment 18

3 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?
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+
(Reporter)

Comment 24

3 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?
This is ESR38 approved but not checked in yet. Can you check this in soon?
Flags: needinfo?(lsalzman)
(Assignee)

Updated

3 years ago
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+
(Reporter)

Comment 27

3 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.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.