Closed Bug 945585 Opened 6 years ago Closed 6 years ago

Crash involving nsNodeUtils::LastRelease(), possibly due to running it twice

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29
blocking-b2g koi+
Tracking Status
firefox25 --- wontfix
firefox26 --- wontfix
firefox27 + verified
firefox28 + verified
firefox29 + verified
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: njn, Assigned: smaug)

Details

(Keywords: csectype-uaf, sec-critical, Whiteboard: [adv-main27+])

Attachments

(3 files)

A user reported frequent crashes.  He provided a test case which he requested not be made public, but I can email anyone the details.  I've given them to bz.

Stack at crash time:

#0  0x000000000040fdee in jemalloc_crash ()
    at ../../../memory/mozjemalloc/jemalloc.c:1592
#1  0x000000000040e916 in arena_dalloc_small (chunk=<optimised out>, 
    arena=<optimised out>, ptr=<optimised out>, mapelm=<optimised out>)
    at ../../../memory/mozjemalloc/jemalloc.c:4610
#2  arena_dalloc (ptr=<optimised out>, offset=<optimised out>)
    at ../../../memory/mozjemalloc/jemalloc.c:4678
#3  0x00007ffff2f99b19 in nsNodeUtils::LastRelease (aNode=0x7fffbe407000)
    at ../../../../content/base/src/nsNodeUtils.cpp:193
#4  0x00007ffff2f55cc4 in nsDocument::Release (this=0x7fffbe407000)
    at ../../../../content/base/src/nsDocument.cpp:1621
#5  0x00007ffff317a024 in ~nsCOMPtr_base (this=<optimised out>)
    at ../../../../dist/include/nsCOMPtr.h:430
#6  ~nsCOMPtr (this=<optimised out>)
    at ../../../../dist/include/nsCOMPtr.h:469
#7  ~nsRefPtr (this=<optimised out>, this=<optimised out>, 
    this=<optimised out>, this=<optimised out>, this=<optimised out>)
    at ../../../../dist/include/nsCOMPtr.h:469
#8  TX_CompileStylesheet (aNode=<optimised out>, aProcessor=<optimised out>, 
    aCallerPrincipal=0x0, aStylesheet=0x7fffbe6944f0)
    at ../../../../../content/xslt/src/xslt/txMozillaStylesheetCompiler.cpp:744
#9  0x00007ffff317f2bf in txMozillaXSLTProcessor::NodeWillBeDestroyed (
    this=0x7fffbe6944c0, aNode=0x100000)
    at ../../../../../content/xslt/src/xslt/txMozillaXSLTProcessor.cpp:1199
#10 0x00007ffff2f99aee in nsNodeUtils::LastRelease (aNode=0x7fffbe407000)
    at ../../../../content/base/src/nsNodeUtils.cpp:188
#11 0x00007ffff2f55cc4 in nsDocument::Release (this=0x7fffbe407000)
    at ../../../../content/base/src/nsDocument.cpp:1621
#12 0x00007ffff3b82775 in ReleaseSliceNow (aSlice=<optimised out>, 
    aData=0x7fffbe2a4798)
    at ../../../xpcom/base/CycleCollectedJSRuntime.cpp:1064
#13 0x00007ffff3b82a06 in mozilla::IncrementalFinalizeRunnable::ReleaseNow (
    this=0x7fffbe2a4780, aLimited=true)
    at ../../../xpcom/base/CycleCollectedJSRuntime.cpp:1126
#14 0x00007ffff3b82b1f in mozilla::IncrementalFinalizeRunnable::Run (
    this=0x7fffbe2a4780)
    at ../../../xpcom/base/CycleCollectedJSRuntime.cpp:1159
#15 0x00007ffff3b7e8ec in nsThread::ProcessNextEvent (this=0x7ffff6c27f20, 
    mayWait=<optimised out>, result=0x7fffffff6dc0)
    at ../../../xpcom/threads/nsThread.cpp:622
#16 0x00007ffff3b4b0e8 in NS_ProcessNextEvent (thread=<optimised out>, 
    mayWait=true) at ../../../xpcom/glue/nsThreadUtils.cpp:238
#17 0x00007ffff2fc142b in nsXMLHttpRequest::Send (this=0x7fffc405ac00, 
    aVariant=<optimised out>, aBody=...)
    at ../../../../content/base/src/nsXMLHttpRequest.cpp:2915
#18 0x00007ffff3a855f7 in Nullable (aBody=..., this=<optimised out>, 
    this=<optimised out>, this=<optimised out>, aValue=...)
    at ../../../content/base/src/nsXMLHttpRequest.h:350
#19 Send (this=<optimised out>, rv=<optimised out>, this=<optimised out>, 
    aBody=...) at ../../../content/base/src/nsXMLHttpRequest.h:354


jemalloc is asserting;  it looks like a double-free or use-after-free.
Attached file Valgrind output
In particular, double-LastRelease on the document?
Assignee: nobody → bugs
Attached patch doc_release.diffSplinter Review
Would like to see the testcase ofc, but this makes document's Release work the
way it works with other nodes.
Attachment #8341623 - Flags: review?(continuation)
Comment on attachment 8341623 [details] [diff] [review]
doc_release.diff

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

Thanks Nick and Olli for looking into this!

I sent Olli the test case via mail.

::: content/base/src/nsDocument.cpp
@@ +1647,5 @@
>        mNeedsReleaseAfterStackRefCntRelease = true;
>        NS_ADDREF_THIS();
>        return mRefCnt.get();
>      }
>      NS_ASSERT_OWNINGTHREAD(nsDocument);

While you are here, you could delete this second assert that looks redundant.
Attachment #8341623 - Flags: review?(continuation) → review+
Attached patch removed assertSplinter Review
Still testing...
So far got 3 fatal assertions on debug build without the patch and none with the patch.
Now testing opt builds.
opt build without patch crashes, with doesn't.
Comment on attachment 8341686 [details] [diff] [review]
removed assert

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I'd say not easily

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Comment could be to stabilize refcnt during shutdown

Which older supported branches are affected by this flaw?
25+

If not all supported branches, which bug introduced the flaw?
Bug 789919

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The same patch applies on beta/aurora/nightly

How likely is this patch to cause regressions; how much testing does it need?
Should be quite safe

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Bug 789919
User impact if declined: 
crashes
Testing completed (on m-c, etc.): 
NA
Risk to taking this patch (and alternatives if risky): 
Should be quite safe. Adds stabilizing to document's Release. Similar to what it had >=24
String or IDL/UUID changes made by this patch:
NA
Attachment #8341686 - Flags: sec-approval?
Attachment #8341686 - Flags: approval-mozilla-beta?
Attachment #8341686 - Flags: approval-mozilla-aurora?
Is there a way for us to detect this kind of bug in the future?  Perhaps by asserting the non-zero-ness of the refcount in nsNodeUtils::LastRelease?
That we could do. nsDocument is very special case here though, see Bug 830975. Other nodes use normal macros.
We're at the end of a cycle and shipping in a week. This needs to go in a couple of weeks into the new cycle.
Whiteboard: [checkin after 12/23]
Comment on attachment 8341686 [details] [diff] [review]
removed assert

sec-approval+ for checkin on 12/23 or later into trunk.
Attachment #8341686 - Flags: sec-approval? → sec-approval+
> sec-approval+ for checkin on 12/23 or later into trunk.

Are you saying this can't be checked into m-c until after 12/23?  That can't be right... surely this should land on m-c and aurora immediately, at least?
The goal is to minimize the time the patch is world visible before we ship the fix.  So if we're not going to ship the fix in the release going out next week, yes, this needs to wait a while.
(In reply to Nicholas Nethercote [:njn] from comment #13)
 
> Are you saying this can't be checked into m-c until after 12/23?  That can't
> be right... surely this should land on m-c and aurora immediately, at least?

This is exactly what I'm saying. The point of sec-approval? is to gate when we check in bad security issues during our six week cycles. Since we're shipping in a week, it is too late to go in now. The date was chosen to give time for it to get in without too much exposure. We do this all the time.
Oh, and when it goes into Trunk should be just before it goes into branches since exposure in one place is exposure everywhere to anyone monitoring checkins of hidden bugs.
Summary: Crash involving nsNodeUtils::LastRelease(), possibly due to running it twice → [FIX] Crash involving nsNodeUtils::LastRelease(), possibly due to running it twice
Whiteboard: [checkin after 12/23]
Summary: [FIX] Crash involving nsNodeUtils::LastRelease(), possibly due to running it twice → Crash involving nsNodeUtils::LastRelease(), possibly due to running it twice
blocking-b2g: --- → koi?
Attachment #8341686 - Flags: approval-mozilla-beta?
Attachment #8341686 - Flags: approval-mozilla-beta+
Attachment #8341686 - Flags: approval-mozilla-aurora?
Attachment #8341686 - Flags: approval-mozilla-aurora+
koi+ for sec-critical bug
blocking-b2g: koi? → koi+
With Nicholas' instructions, I reproduced the crash in FF28 from 2013-11-13. It took around 10 page refreshes to repro.

In each fixed branch, I allowed the page to refresh more than 30 times per run - no crash.

Confirmed fixed FF27, FF28 and FF29, 2014-01-18.
Whiteboard: [adv-main27+]
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.