Closed
Bug 1324000
Opened 7 years ago
Closed 7 years ago
Crash in nsPrintEngine::DonePrintingPages
Categories
(Core :: Printing: Output, defect)
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: bobowen, Assigned: bobowen)
Details
(Keywords: crash, csectype-uaf, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main52+])
Crash Data
Attachments
(1 file)
8.37 KB,
patch
|
smaug
:
review+
jcristau
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Spotted this while working on another printing bug. Looks like there is already a bug for this (bug 413942), although it was originally opened for the same as bug 1321566, I think. Example crash: https://crash-stats.mozilla.com/report/index/acc8befd-d765-4e55-91bd-f4c6f2161215 Anyway as that's a really old bug, thought it was best to open a new sec bug for this, with no links back from the original. mPageSeqFrame originally comes from [1], but never gets nulled out. Looks like we don't reference count frames for performance reasons. When I saw it, it occurred when the nsPrintEngine is reused and we fail before we've got the new mPageSeqFrame, but DonePrintingPages still gets called. But it generally (according to crash-stats) seems to happen during printing, when called from [2]. Not sure how it's been deleted at this point. My first thought was to do away with mPageSeqFrame and get it from the nsPrintObject each time, but as far as I can tell the object pointed to by mPageSeqFrame only gets deleted when the nsPrintObject dies, so maybe that's not valid at this point either. [1] https://hg.mozilla.org/releases/mozilla-release/file/cc272f7d48d3/layout/printing/nsPrintEngine.cpp#l2415 [2] https://hg.mozilla.org/releases/mozilla-release/file/cc272f7d48d3/layout/printing/nsPagePrintTimer.cpp#l92
Updated•7 years ago
|
Group: core-security → layout-core-security
Comment hidden (typo) |
Comment 2•7 years ago
|
||
nsWeakFrame could be used here.
Assignee | ||
Comment 3•7 years ago
|
||
Not sure if I'm using nsWeakFrame correctly here.
Attachment #8824715 -
Flags: review?(bugs)
Updated•7 years ago
|
Attachment #8824715 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 4•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a610591e1fcbdf489d02ceaf19e0c2f91ce37fe
https://hg.mozilla.org/mozilla-central/rev/3a610591e1fc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee | ||
Comment 6•7 years ago
|
||
Comment on attachment 8824715 [details] [diff] [review] Use nsWeakFrame to hold nsIPageSequenceFrame member in nsPrintEngine 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, but tested printing manually. [Needs manual test from QE? If yes, steps to reproduce]: No STR. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: Slightly. [Why is the change risky/not risky?]: The changes are straight forward, but the printing code is quite fragile and does not have automated testing. So only requesting uplift to Aurora. [String changes made/needed]: No
Attachment #8824715 -
Flags: approval-mozilla-aurora?
Comment 7•7 years ago
|
||
Comment on attachment 8824715 [details] [diff] [review] Use nsWeakFrame to hold nsIPageSequenceFrame member in nsPrintEngine printing-related uaf fix for aurora52
Attachment #8824715 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 8•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/c0ed1d46370d
status-firefox52:
--- → fixed
Updated•7 years ago
|
Group: layout-core-security → core-security-release
Updated•7 years ago
|
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Updated•7 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52+]
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•