Assertion failure: GetTransform() == Matrix(mat.xx, mat.yx, mat.xy, mat.yy, mat.x0, mat.y0) (Transforms are out of sync), at gfx\2d\DrawTargetCairo.cpp:1009

RESOLVED FIXED in Firefox 31

Status

()

RESOLVED FIXED
5 years ago
3 months ago

People

(Reporter: cbook, Assigned: kamidphish)

Tracking

(Blocks: 1 bug, {assertion})

unspecified
mozilla31
x86
Windows 7
assertion
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 wontfix, firefox30 wontfix, firefox31 fixed)

Details

(URL)

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 8364965 [details]
windows stack

found via bughunter (feel to reopen this bug if its not a security issue) while loading https://app.box.com/s/yoghzx5gql9mxoy1betq

Loading this page (or maybe reload) results in windows 7 trunk debug builds in

Assertion failure: GetTransform() == Matrix(mat.xx, mat.yx, mat.xy, mat.yy, mat.x0, mat.y
0) (Transforms are out of sync), at c:\Users\mozilla\debug-builds\mozilla-central\gfx\2d\DrawTargetCairo.cpp:1009


will try to see if i can get a testcase on this
(Reporter)

Updated

5 years ago
Group: core-security
Assignee: nobody → dglastonbury
Comment 1 is just a null-deref, so maybe this isn't too bad?
Have you had a chance to look at this yet?  Is this a security problem or just a null deref?  Thanks.
Flags: needinfo?(dglastonbury)
This is still crashing in Nightly and Aurora in the last couple of days. KaiRo do you want to add any info here from crash-stats that might be helpful?
Flags: needinfo?(kairo)
status-firefox29: --- → affected
status-firefox30: --- → affected

Comment 5

5 years ago
(In reply to Liz Henry :lizzard from comment #4)
> This is still crashing in Nightly and Aurora in the last couple of days.
> KaiRo do you want to add any info here from crash-stats that might be
> helpful?

I don't know which crashes correspond to this assertion (I can't tell if all the crashes with the signature of comment #1 do, and if they would, that's bug 798274) so I can't really add any info here.
Flags: needinfo?(kairo)
(In reply to Andrew McCreight [:mccr8] from comment #3)
> Have you had a chance to look at this yet?  Is this a security problem or
> just a null deref?  Thanks.

Sorry, I haven't had a change to look at it yet.
Flags: needinfo?(dglastonbury)
Aren't the stacks (Windows stack and comment #1) completely different?
Created attachment 8382691 [details]
Bug-963492-Windows-stack.txt

Trying to repo this bug, I visited the URL in the report and hit this assertion. I don't understand the code very well to say what the effect is, but could it go on to scribble over memory?
Milan, who do I ask gfxFontUtil questions to (re: Comment 8)??
Flags: needinfo?(milan)
Probably John Daggett or Jonathan Kew, judging by the hg log for gfxFontUtils.
John or Jonathan,

Do from the assert I recorded in Comment 8, do you know if the code kept running would it result in a memory scribble?
Flags: needinfo?(jfkthame)
Flags: needinfo?(jdaggett)

Comment 12

5 years ago
I don't think we should be asserting there, since the length comes from font data not from program logic.  Should be harmless though, just lots of time wasted calling functions with zero-length strings. But I think Jonathan will have better insight into that question.
Flags: needinfo?(jdaggett)
Flags: needinfo?(milan)
I agree - I don't see a need for that assertion. We might want to issue an NS_WARNING if the name data is empty, as that shouldn't normally be the case, and might mean the font won't actually be usable, but asserting seems excessive.

Anyway, I don't think this is connected with the original assertion from the bug summary here, or the crash stack from comment 1.
Flags: needinfo?(jfkthame)
I filed bug 978129 about removing the spurious assertion from DecodeFontName.
After removing the suprious assert and replacing it with return false if aByteLen is <= 0, I can't cant the page to crash. So it's WFM at this stage.
Flags: needinfo?(cbook)
Note that in bug 978129, the assert has been replaced with return true (not false), after ensuring the output string is empty. ISTM that in this case, the (empty) input string can be said to have been successfully decoded, so returning false is unjustified.

So it'd be better to re-test with that patch, if possible. I don't expect it to affect anything (I think the original issue here was unconnected to this), but if returning true here -does- lead to a reproducible crash then we should analyze that and figure out why.
(Reporter)

Comment 17

5 years ago
after trying several reloads with this page (also possible shift-reload to avoid any cache issue) i was able to bring down a win7 trunk build

Built from http://hg.mozilla.org/mozilla-central/rev/e5b09585215f

with:

[1936] WARNING: blocked access to response header: file c:\Users\mozilla\debug-builds\mozilla-central\content\base\src\nsXMLHttpRequest.cpp, line 1203

??????????????????????????: T?????????????????????????????: TAssertion failure: GetTransform() == Matrix(mat.xx, mat.yx, mat.xy, mat.yy, mat.x0, mat.y
0) (Transforms are out of sync), at c:\Users\mozilla\debug-builds\mozilla-central\gfx\2d\DrawTargetCairo.cpp:1011
Flags: needinfo?(cbook)
Milan, I've been unable to repo this on my win7 nightly build. I've tried the STR from Comment 17 and I forced my config to use cairo as the Azure backend.

I spoke to Matt Woodrow, as the area of code isn't really my strong point, and he doesn't see mismatched transforms as being an exploitable security issue. Is there somebody more suitable that this bug should go to?
Flags: needinfo?(milan)
Maybe you can produce a debug build with more information and pass it to people that can reproduce the problem?  When the assertion hits, see what the values are, if we're just "that close" to 1e-6 tolerance, or if we're hitting nan/inf or some such...
Flags: needinfo?(milan)
Group: core-security
Blocks: 939040

Updated

5 years ago
Summary: Assertion failure: GetTransform() == Matrix(mat.xx, mat.yx, mat.xy, mat.yy, mat.x0, mat.y 0) (Transforms are out of sync), at c:\Users\mozilla\debug-builds\mozilla-central\gfx\2d\DrawTargetCairo.cpp:1009 → Assertion failure: GetTransform() == Matrix(mat.xx, mat.yx, mat.xy, mat.yy, mat.x0, mat.y0) (Transforms are out of sync), at gfx\2d\DrawTargetCairo.cpp:1009

Comment 20

5 years ago
Created attachment 8404473 [details]
jesse's fuzz testcase

Hits the same assertion, but only on my remote-desktop Win7 machine, not on my home Win7 machine??
(In reply to Jesse Ruderman from comment #20)
> Hits the same assertion, but only on my remote-desktop Win7 machine, not on
> my home Win7 machine??

I repro'd this but setting:
layers.acceleration.disabled = true
gfx.direct2d.disabled = true

(Thanks to :mattwoodrow)
So Matt Woodrow filled me in on what is happening. The assert is easy to "fix" but the class of bugs is not. Cairo is entering an error state from bad state being passed to cairo. In PopClip(), the matrix setting takes no effect and the assertion fires. I believe the fix is to change the assert to look like http://dxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetCairo.cpp#55.

Info from Matt on IRC:
"It's easy to fix, but it might be part of a larger class of bugs that isn't.
Canvas is backed by skia, content is backed by cairo (accelerated disabled, which is why Jesse only reproduced it on his VM). canvas size is 58k wide. We try create a cairo object wrapping skia’s pixels when we composite and fail, here - http://mxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetCairo.cpp#238 then we call into cairo with a nullptr here - http://mxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetCairo.cpp#523. Cairo doesn’t crash but returns an ‘error pattern' then we try draw with said error pattern, and puts the context into error state at which point it just silently closes up shop and refuses to do anything. We hit that assertion in PopClip() because we try set the transform and get it again and cairo just refuses."
Created attachment 8406602 [details] [diff] [review]
Fix PopClip() assertion.

Modified assert in PopClip() to check for the cairo_status also. (There is prior art for this).

Also added error handling for the case exposed by Jesse's testcase.
Attachment #8406602 - Flags: review?(matt.woodrow)
Comment on attachment 8406602 [details] [diff] [review]
Fix PopClip() assertion.

Review of attachment 8406602 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/DrawTargetCairo.cpp
@@ +332,3 @@
>  
> +        cairo_surface_destroy(surf);
> +      }

We can dereference pat later on it this function, so we shouldn't do this.  Maybe return nullptr immediately, and fix all the callers of this function.
Created attachment 8406628 [details] [diff] [review]
Bug fix for Aurora

This is the patch for ESR24 ported to Aurora.
Attachment #8406628 - Flags: review?(jgilbert)
Comment on attachment 8406628 [details] [diff] [review]
Bug fix for Aurora

Wrong bug
Attachment #8406628 - Attachment is obsolete: true
Attachment #8406628 - Flags: review?(jgilbert)
Created attachment 8406633 [details] [diff] [review]
patch

Address Matt's suggestions.
Attachment #8406633 - Flags: review?(matt.woodrow)
Attachment #8406602 - Attachment is obsolete: true
Attachment #8406602 - Flags: review?(matt.woodrow)
Attachment #8406633 - Flags: review?(matt.woodrow) → review+
Keywords: checkin-needed
(Reporter)

Comment 30

5 years ago
https://hg.mozilla.org/mozilla-central/rev/cc4dc391e6e7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
status-firefox29: affected → wontfix
status-firefox30: affected → wontfix
status-firefox31: --- → fixed
Flags: in-testsuite?
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.