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

Status: RESOLVED FIXED

(Core :: Printing: Output, defect, P1)

Description

[Julian Seward [:jseward]]

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.

Comment 1

[Astley Chen (inactive)]

It seems a regression from bug 1319423 as the first crash report started after it's landed and only happened on FF58.0a1.

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

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?

Comment 3

[Jeff Muizelaar [:jrmuizel]]

OpenFD is probably getting inlined along with WriteHeader.

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

Ah, of course, thanks.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

FWIW, the methods table on the FD is e5e5e5e5

Comment 6

[Alex Gaynor [:Alex_Gaynor]]

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.

Comment 7

[Alex Gaynor [:Alex_Gaynor]]

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

Comment 8

[Mantaroh Yoshinaga[:mantaroh]]

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

Comment 9

[Alex Gaynor [:Alex_Gaynor]]

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.

Comment 10

[Alex Gaynor [:Alex_Gaynor]]

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.

Comment 11

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1412643 - revert part 3 of the changes from 1319423 to fix print selection;

Review commit: https://reviewboard.mozilla.org/r/195480/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195480/

Comment 12

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1412643 - Part 2 - make the memory management in print IPC clearer;

This should result in cleaner crashes if methods are called out of order

Review commit: https://reviewboard.mozilla.org/r/195482/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195482/

Comment 13

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1412643 - Part 3 - fixed print selection;

When printing a selection, we treat all pages in a selection as a single 'page'
internally. Before this patch we handled this in a single iteration, thereby
bypassing the control flow normally applied to the IPC. With this patch, we
don't attempt to print additional pages until IPC related to the former page has
completed.

Review commit: https://reviewboard.mozilla.org/r/195484/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195484/

Comment 14

[Bob Owen (:bobowen)]

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.

Comment 15

[Bob Owen (:bobowen)]

Comment on attachment 8924264 [details]
Bug 1412643 - Part 2 - make the memory management in print IPC clearer;

https://reviewboard.mozilla.org/r/195482/#review201488

Comment 16

[Bob Owen (:bobowen)]

Comment on attachment 8924265 [details]
Bug 1412643 - Part 3 - fixed print selection;

https://reviewboard.mozilla.org/r/195484/#review201490

Comment 17

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8924263 [details]
Bug 1412643 - revert part 3 of the changes from 1319423 to fix print selection;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195480/diff/1-2/

Comment 18

[Bob Owen (:bobowen)]

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.

Comment 19

[Bob Owen (:bobowen)]

gah
s/you're/your/

Comment 20

[Alex Gaynor [:Alex_Gaynor]]

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.

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

UAFs show up in the crashes.  Recent regression in 58, doesn't need security approval to land.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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