e10s Crash in nsPrintEngine::PrePrintPage

RESOLVED FIXED in Firefox 52

Status

()

Core
Printing: Output
--
critical
RESOLVED FIXED
a year ago
24 days ago

People

(Reporter: philipp, Assigned: bobowen)

Tracking

(4 keywords)

51 Branch
mozilla53
All
Windows
crash, csectype-uaf, regression, sec-moderate
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 wontfix, firefox50 wontfix, firefox51 wontfix, firefox52 fixed, firefox53 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main52+], crash signature)

Attachments

(2 attachments)

(Reporter)

Description

a year ago
This bug was filed from the Socorro interface and is 
report bp-34142b46-3cc4-4540-b0ac-aeeaf2161201.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	nsPrintEngine::PrePrintPage() 	layout/printing/nsPrintEngine.cpp:2647
1 	xul.dll 	nsPagePrintTimer::Notify(nsITimer*) 	layout/printing/nsPagePrintTimer.cpp:152
2 	xul.dll 	nsTimerImpl::Fire() 	xpcom/threads/nsTimerImpl.cpp:524
3 	xul.dll 	nsTimerEvent::Run() 	xpcom/threads/TimerThread.cpp:286
4 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1067
5 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/glue/nsThreadUtils.cpp:290
6 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:124
7 	xul.dll 	mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:301
8 	xul.dll 	_SEH_epilog4

hi, this is another printing related signature that is spiking up in 51.0b5. it's happening across all versions of windows and always in the content process. 
in early data for beta 5 this is accounting for 28% of all content crashes.

pushlog from beta 4 to beta 5: 51.0b4 & beta 5:
https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=FIREFOX_51_0b4_RELEASE&tochange=FIREFOX_51_0b5_RELEASE

might this be related to bug 1279699 as well?

(marking the report as security sensitive due to the crashing address)
Flags: needinfo?(bobowencode)
(Assignee)

Updated

a year ago
Duplicate of this bug: 1321567
Component: General → Printing: Output
Clearly a UAF, but print preview requires enough user interaction we can call this sec-moderate rather than sec-high.
Blocks: 1279699
Keywords: csectype-uaf, sec-moderate
(Assignee)

Comment 3

a year ago
I'm going to leave this open even though the patches for bug 1279699, have been backed out.

It has been happening at a lower level for some time.
I suspect it happens when the print is aborted for some reason and the last patches for bug 1279699 were causing many more failures.

We should still get this fixed though.
Flags: needinfo?(bobowencode)
(Assignee)

Updated

a year ago
status-firefox-esr45: --- → affected
(Assignee)

Comment 4

11 months ago
Created attachment 8819284 [details] [diff] [review]
Use UniquePtr to hold nsPrintData in nsPrintEngine

I think you're probably the best person to review this.

The code this is replacing, appears to be doing the right thing with these pointers, so I'm not sure how this is happening.
Using UniquePtr can't hurt though.

I don't think it can be a threading problem as I believe this is all called on the main thread.
I'm not sure UniquePtr will help if it is a threading issue.

As an aside, I'm not at all convinced we need all three of these nsPrintData, I think we could probably get away with one, but I don't want to make more changes until I've got some tests working.
Attachment #8819284 - Flags: review?(jwatt)
(Assignee)

Updated

11 months ago
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
Attachment #8819284 - Flags: review?(jwatt) → review+

Updated

11 months ago
Group: core-security → layout-core-security

Comment 5

11 months ago
Isn't the crash more likely that nsPrintEngine is gone when the timer fires
(but using UniquePtr doesn't hurt).

http://searchfox.org/mozilla-central/rev/f680e72cc6579f90b992b63ca14d923d2afea612/layout/printing/nsPagePrintTimer.h#61 looks very suspicious.

Comment 6

11 months ago
Need to be a bit careful with cycles. So perhaps nsPagePrintTimer could have nsWeakPtr pointing to nsPrintEngine or so.

Comment 7

11 months ago
Created attachment 8819837 [details] [diff] [review]
wip

I ended up starting to write a patch, but since print-to-file seems to be broken on linux, I couldn't test this.

Comment 8

11 months ago
Bob, were you going to take a look at this?
I forgot what we discussed on IRC.
Flags: needinfo?(bobowencode)
(Assignee)

Comment 9

11 months ago
(In reply to Olli Pettay [:smaug] from comment #8)
> Bob, were you going to take a look at this?
> I forgot what we discussed on IRC.

We decided we were going to go with your patch and I asked a few days ago (on IRC) if you wanted to upload another version or whether I could just put a header on the existing one and land them both.

I think it all got lost as it was just before Christmas.
Flags: needinfo?(bobowencode) → needinfo?(bugs)

Comment 10

11 months ago
ok, let me test the patch a bit. I think it is still doable somehow if I disable all sandboxing
(Assignee)

Comment 11

11 months ago
(In reply to Olli Pettay [:smaug] from comment #10)
> ok, let me test the patch a bit. I think it is still doable somehow if I
> disable all sandboxing

OK thanks.
I have also done some testing with your patch on Windows.

Comment 12

11 months ago
Can't test this right now since printing crashes in debug builds even without this patch :/

Updated

11 months ago
Depends on: 1326418

Comment 13

11 months ago
Comment on attachment 8819837 [details] [diff] [review]
wip

Whoever gets a chance to review this first.
The changes should be minimal and patch applies for example to beta too.
Flags: needinfo?(bugs)
Attachment #8819837 - Flags: review?(jwatt)
Attachment #8819837 - Flags: review?(bobowencode)
(Assignee)

Comment 14

11 months ago
Comment on attachment 8819837 [details] [diff] [review]
wip

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

(In reply to Olli Pettay [:smaug] from comment #13)
> Comment on attachment 8819837 [details] [diff] [review]
> wip
> 
> Whoever gets a chance to review this first.
> The changes should be minimal and patch applies for example to beta too.

Thanks Olli, just gone over this again to remind myself, although I'd looked at it before Christmas.

I'll get these two landed, although as you said, your patch is far more likely to fix this problem.

Over uplift, given that this crash is not all that common and the fragility of the printing code, I think I'd say uplift to aurora but not beta, so it gets a full beta cycle just in case it causes problems.
Attachment #8819837 - Flags: review?(jwatt)
Attachment #8819837 - Flags: review?(bobowencode)
Attachment #8819837 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/23646223ff69
https://hg.mozilla.org/mozilla-central/rev/e8a7beb2bf1a
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
status-firefox50: affected → wontfix
Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(bobowencode)
(Assignee)

Comment 17

11 months ago
Comment on attachment 8819284 [details] [diff] [review]
Use UniquePtr to hold nsPrintData in nsPrintEngine

smaug, dveditz - I'm only requesting uplift to Aurora as this is a fairly low level crash and the printing code is pretty fragile. Please say if you think we should request for Beta as well.

Approval Request Comment
[Feature/Bug causing the regression]:
Long-term crash.

[User impact if declined]:
Small number of users will still experience this crash.

[Is this code covered by automated tests?]:
No

[Has the fix been verified in Nightly?]:
We do not have STR.

[Needs manual test from QE? If yes, steps to reproduce]: 
No STR.

[List of other uplifts needed for the feature/fix]:
Other patch in this bug.

[Is the change risky?]:
Slightly.

[Why is the change risky/not risky?]:
The changes are relatively straight forward, but the printing code is quite fragile and does not have automated testing.

[String changes made/needed]:
No
Flags: needinfo?(dveditz)
Flags: needinfo?(bugs)
Flags: needinfo?(bobowencode)
Attachment #8819284 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 18

11 months ago
Comment on attachment 8819837 [details] [diff] [review]
wip

See comment 17.
Attachment #8819837 - Flags: approval-mozilla-aurora?

Comment 19

11 months ago
The initial comment says "hi, this is another printing related signature that is spiking up in 51.0b5.".
I consider the patch relatively safe since it is effectively adding null checks.
So, perhaps beta wouldn't be too risky, at least if we can get at least some manual testing (since the next merge day is soon).
Flags: needinfo?(bugs)
(Assignee)

Comment 20

11 months ago
(In reply to Olli Pettay [:smaug] from comment #19)
> The initial comment says "hi, this is another printing related signature
> that is spiking up in 51.0b5.".

That was because of a problem I introduced that I've since rectified.
It did however highlight this and a couple of other low-level crashes that I thought were worth fixing anyway.
Comment on attachment 8819284 [details] [diff] [review]
Use UniquePtr to hold nsPrintData in nsPrintEngine

printing crash fix, aurora52+
Attachment #8819284 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8819837 [details] [diff] [review]
wip

printing crash fix, aurora52+
Attachment #8819837 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Bob Owen (:bobowen) from comment #17)
> smaug, dveditz - I'm only requesting uplift to Aurora as this is a fairly
> low level crash and the printing code is pretty fragile. Please say if you
> think we should request for Beta as well.

Fair enough.
Flags: needinfo?(dveditz)
status-firefox51: affected → wontfix
status-firefox-esr45: affected → wontfix

Comment 24

10 months ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/9d7e7bc831d1
https://hg.mozilla.org/releases/mozilla-aurora/rev/873a99553516
status-firefox52: affected → fixed

Updated

10 months ago
Group: layout-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.