Crash in PR_Write | mozilla::layout::PRFileDescStream::write with "print selection"

RESOLVED FIXED in Firefox 58

Status

()

defect
P1
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jseward, Assigned: Alex_Gaynor)

Tracking

(4 keywords)

unspecified
mozilla58
x86
Windows 10
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 unaffected, firefox57 unaffected, firefox58 fixed)

Details

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

Attachments

(2 attachments, 2 obsolete attachments)

This bug was filed from the Socorro interface and is 
report bp-3ef8abc2-ad4e-49c6-b1d8-36edb0171028.
=============================================================

This is topcrash #16 in Windows nightly 20171026221945.  There are
4 different installations, so it looks real.  From the crash stacks
I assume it's some printing problem.
Flags: needinfo?(bobowencode)
Flags: needinfo?(bobowencode) → needinfo?(agaynor)
It seems a regression from bug 1319423 as the first crash report started after it's landed and only happened on FF58.0a1.
Blocks: 1319423
Keywords: regression
Priority: -- → P2
I'm pretty confused by the stack here, it shows nsDeviceContextSpecProxy::BeginPage -> PRFileDescStream::write, but BeginPage doesn't call PRFileDescStream::write, it calls PRFileDescStream::OpenFD.

Not quite sure what to make of this, is it some quirk of the optimizer/stack trace generation, or is the stack right and there's really some bug with the dispatch?
OpenFD is probably getting inlined along with WriteHeader.
Ah, of course, thanks.
Flags: needinfo?(agaynor)
FWIW, the methods table on the FD is e5e5e5e5
That appears to be a jemalloc magic value for "freed", so this is a UAF...

I'm not sure I understand what control flow can lead to this, but I'm digging.
Assignee: nobody → agaynor
:bobowen says he was able to reproduce with 'print-selection'. I'm having some trouble reproducing due to running into other bugs, but will keep at it :-)
Posted file test-case.html
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #7)
> :bobowen says he was able to reproduce with 'print-selection'. I'm having
> some trouble reproducing due to running into other bugs, but will keep at it
> :-)

This comment is just a quick note of my crash.

In my environment:
 * Windows 10 (XPS 13, Core i7/16G) 
 * Nightly(2017-10-30) 64bit
 * Printer is 'Microsoft Print to PDF'

STR:
 1) Open attachment file
 2) Select all text
 3) Print with 'Print selection only'
 4) Enter PDF Name into printer driver's dialog.
Ok, spent some time with a debugger, and here's what appears to be doing on. Code here: https://searchfox.org/mozilla-central/source/layout/generic/nsSimplePageSequenceFrame.cpp#749-798

1) We get a FD from the parent when we start printing.
2) We enter this loop and do a BeginPage(), initializing the mPrintRecorder with the FD we started with
3) We continue through the loop, eventually calling EndPage(). EndPage() sends a ProcessPage() message to the parent, but we don't wait for the parent to respond.
4) We get back to the top of the loop and go again!
5) We call BeginPage() again which tries to use the FD a second time, leading to two people thinking they own the FD for freeing it.

I see the change I made before that broke it. I'm concerned it was working only by accident, but this should restore the previous behaiour.

Patch up shortly after I verify it works the way I think it does.
Well, I was half right. My diagnosis of the issue was correct, but the fix wasn't. Seems like the best direction would be to see how hard it would be to use the print timer here, so we don't try to proceed until a new FD is sent down to the child. I'll see how much trouble that looks like.
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8924263 [details]
Bug 1412643 - revert part 3 of the changes from 1319423 to fix print selection;

https://reviewboard.mozilla.org/r/195480/#review201484

I think it probably makes sense to land these as part of the relanding of the original change in Fx59.
Attachment #8924263 - Flags: review?(bobowencode)
Comment on attachment 8924264 [details]
Bug 1412643 - Part 2 - make the memory management in print IPC clearer;

https://reviewboard.mozilla.org/r/195482/#review201488
Attachment #8924264 - Flags: review?(bobowencode)
Comment on attachment 8924265 [details]
Bug 1412643 - Part 3 - fixed print selection;

https://reviewboard.mozilla.org/r/195484/#review201490
Attachment #8924265 - Flags: review?(bobowencode)
Summary: Crash in PR_Write | mozilla::layout::PRFileDescStream::write → Crash in PR_Write | mozilla::layout::PRFileDescStream::write with "print selection"
Attachment #8924264 - Attachment is obsolete: true
Attachment #8924265 - Attachment is obsolete: true
Comment on attachment 8924263 [details]
Bug 1412643 - revert part 3 of the changes from 1319423 to fix print selection;

https://reviewboard.mozilla.org/r/195480/#review201534

I see that PRFileDescStream::Seek was added in the changeset we're reverting, but I guess it doesn't matter if we leave it in.
It looks like you're buffering fixes are still in place for the use of nspr.
Attachment #8924263 - Flags: review?(bobowencode) → review+
gah
s/you're/your/
Yes, when I went to backout that patch Seek conflicted, and I decided to just leave it in, even though it was unused, since that would ensure I didn't regress it by accident when I migrate the patch to another branch.
Keywords: checkin-needed
UAFs show up in the crashes.  Recent regression in 58, doesn't need security approval to land.
Group: core-security
This was autolanded before the bug was closed off and has now been merged to m-c.

https://hg.mozilla.org/mozilla-central/rev/ed7bb5988eb1
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Group: core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.