Closed Bug 805957 Opened 7 years ago Closed 7 years ago

Possible memory corruption of nsDeviceContext

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox16 --- wontfix
firefox17 + fixed
firefox18 + fixed
firefox19 --- fixed
firefox-esr10 17+ fixed

People

(Reporter: mccr8, Assigned: mats)

Details

(Keywords: csectype-uaf, sec-critical, Whiteboard: [adv-track-main17+][adv-track-esr17+])

Attachments

(3 files)

Bug 776505 involves an intermittent orange with ASSERTION: nsDeviceContext not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread().  roc landed an assertion in nsDeviceContext's constructor that it is being created on the main thread, but it does not seem to be triggering in that case. Given that, it seems that it is likely that there is some kind of memory corruption. If the field that stores the thread ID gets messed up, then we'll fail this assertion, even though it is only ever used on the main thread.

The stacks seem to always look like:

0  XUL!nsDeviceContext::AddRef() [nsDeviceContext.h : 26 + 0x0]
1  XUL!nsRenderingContext::Init(nsDeviceContext*, gfxContext*) [nsAutoPtr.h : 845 + 0x7]
2  XUL!PresShell::RenderDocument(nsRect const&, unsigned int, unsigned int, gfxContext*) [nsPresShell.cpp : 4359 + 0xd]
3  XUL!mozilla::dom::CanvasRenderingContext2D::DrawWindow(nsIDOMWindow*, double, double, double, double, nsAString_internal const&, unsigned int, mozilla::ErrorResult&) [CanvasRenderingContext2D.cpp : 3962 + 0xb]
4  XUL!mozilla::dom::CanvasRenderingContext2DBinding::drawWindow [CanvasRenderingContext2DBinding.cpp : 1868 + 0x43]
5  XUL!mozilla::dom::CanvasRenderingContext2DBinding::genericMethod [CanvasRenderingContext2DBinding.cpp : 2570 + 0x12]

This is the strip of code in question:
4347   nsDeviceContext* devCtx = mPresContext->DeviceContext();
4348   gfxFloat scale = gfxFloat(devCtx->AppUnitsPerDevPixel())/nsPresContext::AppUnitsPerCSSPixel();
4349   aThebesContext->Scale(scale, scale);
4354   aThebesContext->NudgeCurrentMatrixToIntegers();
4356   AutoSaveRestoreRenderingState _(this);
4358   nsRefPtr<nsRenderingContext> rc = new nsRenderingContext();
4359   rc->Init(devCtx, aThebesContext);

Not knowing anything about this code, my spideysense did start tingling at that raw pointer to the devCtx. |this| strongly holds mPresContext, which strongly holds its device context, but I guess if either of those fields changes in between line 4347 and 4359 then we could have a UAF. To fix that problem, devCtx could become a strong pointer, or just be inlined, as it is only used twice. We could also add an assert that devCtx is the same as mPresContext->DeviceContext() before the Init, assuming that should be the case.

Kyle also suggested the possibility that devCtx is just corrupt from the get-go. This could be tested by landing an assertion in DeviceContext() to check valid threadedness of the nsDeviceContext.

I'm slightly nervous about poking around with exploratory asserts, as it points out possible security problems, but maybe it isn't a big deal.
> if either of those fields changes in between line 4347 and 4359

Given the code I'm pretty sure that can't happen.  Also, the pres context
is held strongly by the caller (CanvasRenderingContext2D::DrawWindow).
So I believe it must be corrupt already at 4347.

Reading CanvasRenderingContext2D::DrawWindow I see one bug:
http://hg.mozilla.org/mozilla-central/annotate/792a91b91020/content/canvas/src/CanvasRenderingContext2D.cpp#l3961
presContext->PresShell()->RenderDocument(...)
This is wrong, the caller MUST guarantee liveness for the shell while calling
RenderDocument, otherwise the shell might be deleted and 'mIsDestroying'
is garbage here:
http://hg.mozilla.org/mozilla-central/annotate/792a91b91020/layout/base/nsPresShell.cpp#l4283

It seems somewhat unlikely that we would avoid crashing to reach line
4347 with a deleted 'this', then use a garbage mPresContext ptr.
But I guess it could happen in theory; in that case it seems likely there
would be some crashes also in PresShell::RenderDocument, and there is:
https://crash-stats.mozilla.com/query/query?product=Firefox&version=ALL%3AALL&range_value=4&range_unit=weeks&query_search=signature&query_type=contains&query=PresShell%3A%3ARenderDocument&reason=&build_id=&process_type=any&hang_type=any&do_query=1
(eg bug 747602)

We should fix the RenderDocument() call site anyway, so let's start
there and see if it helps.
Attached patch fixSplinter Review
see last comment
Attachment #675726 - Flags: review?(roc)
Comment on attachment 675726 [details] [diff] [review]
fix

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

good catch(es)
Attachment #675726 - Flags: review?(roc) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7aebe2c45ef

Leave open because this is a tentative fix.  We should look at crash-stats
for PresShell::RenderDocument and our own "orange crashes" like bug 747602
in a few weeks time and see if it seems to have fixed it.
Assignee: nobody → matspal
Whiteboard: [leave open]
speculatively adding keywords based on the fix
Attached patch for aurora/betaSplinter Review
On aurora/beta there is also a RenderDocument() call in
nsCanvasRenderingContext2DAzure.cpp which needs the same fix
Attachment #675890 - Flags: review?(roc)
Attached patch for esr10Splinter Review
esr10 is slightly different
Attachment #675891 - Flags: review?
Attachment #675891 - Attachment is patch: true
Attachment #675891 - Flags: review? → review?(roc)
No Bug 776505 crashes in the 1.5 days since this landed, which is promising.
Comment on attachment 675891 [details] [diff] [review]
for esr10

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: crash
Testing completed (on m-c, etc.): on m-c since 2012-10-27
Risk to taking this patch (and alternatives if risky): very low risk
String or UUID changes made by this patch: none
Attachment #675891 - Flags: approval-mozilla-esr10?
Comment on attachment 675890 [details] [diff] [review]
for aurora/beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: crash
Testing completed (on m-c, etc.): on m-c since 2012-10-27
Risk to taking this patch (and alternatives if risky): very low risk
String or UUID changes made by this patch: none
Attachment #675890 - Flags: approval-mozilla-beta?
Attachment #675890 - Flags: approval-mozilla-aurora?
Attachment #675890 - Flags: approval-mozilla-beta?
Attachment #675890 - Flags: approval-mozilla-beta+
Attachment #675890 - Flags: approval-mozilla-aurora?
Attachment #675890 - Flags: approval-mozilla-aurora+
Attachment #675891 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Whiteboard: [leave open] → [leave open][adv-track-main17+][adv-track-esr17+]
The underlying orange has gone away, so it seems like this can be closed.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [leave open][adv-track-main17+][adv-track-esr17+] → [adv-track-main17+][adv-track-esr17+]
Target Milestone: --- → mozilla19
Group: core-security
You need to log in before you can comment on or make changes to this bug.