963492 - 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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Carsten Book [:Tomcat]]

Attachment: 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

Comment 1

[Carsten Book [:Tomcat]]

hm seems to crash also aurora opt builds -> https://crash-stats.mozilla.com/report/index/a6a5f10d-f3d4-42c6-8ee0-4201b2140124

Comment 2

[Andrew McCreight [:mccr8]]

Comment 1 is just a null-deref, so maybe this isn't too bad?

Comment 3

[Andrew McCreight [:mccr8]]

Have you had a chance to look at this yet?  Is this a security problem or just a null deref?  Thanks.

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

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?

Comment 5

[Robert Kaiser]

(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.

Comment 6

[u480271]

(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.

Comment 7

[u480271]

Aren't the stacks (Windows stack and comment #1) completely different?

Comment 8

[u480271]

Attachment: 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?

Comment 9

[u480271]

Milan, who do I ask gfxFontUtil questions to (re: Comment 8)??

Comment 10

[Andrew McCreight [:mccr8]]

Probably John Daggett or Jonathan Kew, judging by the hg log for gfxFontUtils.

Comment 11

[u480271]

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?

Comment 12

[John Daggett (:jtd)]

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.

Comment 13

[Jonathan Kew [:jfkthame]]

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.

Comment 14

[Jonathan Kew [:jfkthame]]

I filed bug 978129 about removing the spurious assertion from DecodeFontName.

Comment 15

[u480271]

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.

Comment 16

[Jonathan Kew [:jfkthame]]

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.

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[u480271]

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?

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

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...

Comment 20

[Jesse Ruderman]

Attachment: jesse's fuzz testcase

Hits the same assertion, but only on my remote-desktop Win7 machine, not on my home Win7 machine??

Comment 21

[u480271]

(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)

Comment 22

[u480271]

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."

Comment 23

[u480271]

Attachment: 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.

Comment 24

[u480271]

Try run https://tbpl.mozilla.org/?tree=Try&rev=838cd584815f

Comment 25

[Matt Woodrow (:mattwoodrow)]

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.

Comment 26

[u480271]

Attachment: Bug fix for Aurora

This is the patch for ESR24 ported to Aurora.

Comment 27

[u480271]

Comment on attachment 8406628 [details] [diff] [review]
Bug fix for Aurora

Wrong bug

Comment 28

[u480271]

Attachment: patch

Address Matt's suggestions.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc4dc391e6e7

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cc4dc391e6e7