815500 - Heap-use-after-free in mozilla::RecomputeDirectionality

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Abhishek Arya]

Attachment: Testcase

Reproduces on trunk, looks like regression from http://hg.mozilla.org/mozilla-central/rev/7f2e295e4981

==518== ERROR: AddressSanitizer: heap-use-after-free on address 0x7fe0d36b0a6c at pc 0x7fe0fb4e9640 bp 0x7fff7754b430 sp 0x7fff7754b428
READ of size 4 at 0x7fe0d36b0a6c thread T0
    #0 0x7fe0fb4e963f in nsINode::GetBoolFlag(nsINode::BooleanFlag) const src/content/base/public/nsINode.h:1334
    #1 0x7fe0fd312d5a in nsINode::HasTextNodeDirectionalityMap() const src/../../../dist/include/nsINode.h:1417
    #2 0x7fe0fd30f424 in mozilla::nsTextNodeDirectionalityMap::RemoveElementFromMap(nsINode*, mozilla::dom::Element*) src/content/base/src/DirectionalityUtils.cpp:510
    #3 0x7fe0fd30eaea in mozilla::RecomputeDirectionality(mozilla::dom::Element*, bool) src/content/base/src/DirectionalityUtils.cpp:551
    #4 0x7fe0fe864870 in nsGenericHTMLElement::AfterSetAttr(int, nsIAtom*, nsAttrValue const*, bool) src/content/html/content/src/nsGenericHTMLElement.cpp:1760
    #5 0x7fe0fd8c6951 in mozilla::dom::Element::SetAttrAndNotify(int, nsIAtom*, nsIAtom*, nsAttrValue const&, nsAttrValue&, unsigned char, bool, bool, bool) src/content/base/src/Element.cpp:1883
    #6 0x7fe0fd8c3ab7 in mozilla::dom::Element::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, bool) src/content/base/src/Element.cpp:1775
    #7 0x7fe0fe86703d in nsGenericHTMLElement::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, bool) src/content/html/content/src/nsGenericHTMLElement.cpp:1838
    #8 0x7fe0fd8ac9e1 in mozilla::dom::Element::SetAttribute(nsAString_internal const&, nsAString_internal const&, mozilla::ErrorResult&) src/content/base/src/Element.cpp:747
    #9 0x7fe1035f0c13 in nsIDOMElement_SetAttribute(JSContext*, unsigned int, JS::Value*) src/objdir-ff-asan-sym/js/xpconnect/src/dom_quickstubs.cpp:1992
    #10 0x7fe10f7de089 in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) src/js/src/jscntxtinlines.h:364
    #11 0x7fe10f7de089 in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) src/js/src/jsinterp.cpp:369
    #12 0x7fe10f78d875 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) src/js/src/jsinterp.cpp:2334
    #13 0x7fe10f6ed4ee in js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) src/js/src/jsinterp.cpp:326
    #14 0x7fe10f7eb68d in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) src/js/src/jsinterp.cpp:515
    #15 0x7fe10f7ed34b in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) src/js/src/jsinterp.cpp:552
    #16 0x7fe10ef3cbed in JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, unsigned short const*, unsigned long, JS::Value*) src/js/src/jsapi.cpp:5612
    #17 0x7fe0ffeb6fcb in nsJSContext::EvaluateString(nsAString_internal const&, JSObject*, nsIPrincipal*, nsIPrincipal*, char const*, unsigned int, JSVersion, nsAString_internal*, bool*) src/dom/base/nsJSEnvironment.cpp:1520
    #18 0x7fe100078071 in nsGlobalWindow::RunTimeoutHandler(nsTimeout*, nsIScriptContext*) src/dom/base/nsGlobalWindow.cpp:9686
    #19 0x7fe10002cb3e in nsGlobalWindow::RunTimeout(nsTimeout*) src/dom/base/nsGlobalWindow.cpp:9945
    #20 0x7fe100076186 in nsGlobalWindow::TimerCallback(nsITimer*, void*) src/dom/base/nsGlobalWindow.cpp:10214
    #21 0x7fe1083561a8 in nsTimerImpl::Fire() src/xpcom/threads/nsTimerImpl.cpp:482
    #22 0x7fe108357631 in nsTimerEvent::Run() src/xpcom/threads/nsTimerImpl.cpp:565
    #23 0x7fe10831a32e in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:627
    #24 0x7fe107f8ff7f in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:221
    #25 0x7fe10667907d in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:117
    #26 0x7fe108602e2e in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:215
    #27 0x7fe108602c75 in MessageLoop::RunHandler() src/ipc/chromium/src/base/message_loop.cc:208
    #28 0x7fe108602b5b in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:182
    #29 0x7fe105a8c414 in nsBaseAppShell::Run() src/widget/xpwidgets/nsBaseAppShell.cpp:163
    #30 0x7fe1045ef232 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:290
    #31 0x7fe0f9b28bd4 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:3823
    #32 0x7fe0f9b2e8a9 in XREMain::XRE_main(int, char**, nsXREAppData const*) src/toolkit/xre/nsAppRunner.cpp:3890
    #33 0x7fe0f9b31620 in XRE_main src/toolkit/xre/nsAppRunner.cpp:4084
    #34 0x40c146 in do_main(int, char**) src/browser/app/nsBrowserApp.cpp:174
    #35 0x409980 in main src/browser/app/nsBrowserApp.cpp:279
    #36 0x7fe119da476c in
0x7fe0d36b0a6c is located 44 bytes inside of 120-byte region [0x7fe0d36b0a40,0x7fe0d36b0ab8)
freed by thread T0 here:
    #0 0x4c37d0 in __interceptor_free
    #1 0x7fe117932405 in moz_free src/memory/mozalloc/mozalloc.cpp:48
    #2 0x7fe0fdbc3c1d in operator delete(void*) src/../../../dist/include/mozilla/mozalloc.h:224
    #3 0x7fe0fdbc3c1d in nsTextNode::~nsTextNode() src/content/base/src/nsTextNode.cpp:123
    #4 0x7fe0fda92462 in nsNodeUtils::LastRelease(nsINode*) src/content/base/src/nsNodeUtils.cpp:257
    #5 0x7fe0fd912887 in nsGenericDOMDataNode::Release() src/content/base/src/nsGenericDOMDataNode.cpp:115
    #6 0x7fe0fdbc4107 in nsTextNode::Release() src/content/base/src/nsTextNode.cpp:127
    #7 0x7fe0f9af2c3b in nsCOMPtr_base::~nsCOMPtr_base() src/objdir-ff-asan-sym/media/webrtc/signaling/signaling_ecc/../../../../dist/include/nsCOMPtr.h:410
    #8 0x7fe0fb79c048 in nsCOMPtr<nsIContent>::~nsCOMPtr() src/../../dist/include/nsCOMPtr.h:449
    #9 0x7fe0fb79bd35 in nsCOMPtr<nsIContent>::~nsCOMPtr() src/../../dist/include/nsCOMPtr.h:449
    #10 0x7fe0fdd54118 in mozilla::dom::FragmentOrElement::RemoveChildAt(unsigned int, bool) src/content/base/src/FragmentOrElement.cpp:890
    #11 0x7fe0fe858020 in nsGenericHTMLElement::SetInnerHTML(nsAString_internal const&, mozilla::ErrorResult&) src/content/html/content/src/nsGenericHTMLElement.cpp:1206
    #12 0x7fe103a5a4f6 in nsIDOMHTMLElement_SetInnerHTML(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>, int, JS::MutableHandle<JS::Value>) src/objdir-ff-asan-sym/js/xpconnect/src/dom_quickstubs.cpp:7938
    #13 0x7fe10fabc165 in js::CallJSPropertyOpSetter(JSContext*, int (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>, int, JS::MutableHandle<JS::Value>), JS::Handle<JSObject*>, JS::Handle<long>, int, JS::MutableHandle<JS::Value>) src/js/src/jscntxtinlines.h:448
    #14 0x7fe10fabc165 in js::Shape::set(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, bool, JS::MutableHandle<JS::Value>) src/js/src/jsscopeinlines.h:333
    #15 0x7fe10faf275d in js::baseops::SetPropertyHelper(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<long>, unsigned int, JS::MutableHandle<JS::Value>, int) src/js/src/jsobj.cpp:4584
    #16 0x7fe10f81cc61 in js::SetPropertyOperation(JSContext*, unsigned char*, JS::Handle<JS::Value>, JS::Handle<JS::Value>) src/js/src/jsinterpinlines.h:364
previously allocated by thread T0 here:
    #0 0x4c3890 in malloc
    #1 0x7fe117932541 in moz_xmalloc src/memory/mozalloc/mozalloc.cpp:54
    #2 0x7fe0fdbc301f in operator new(unsigned long) src/../../../dist/include/mozilla/mozalloc.h:200
    #3 0x7fe0fdbc301f in NS_NewTextNode(nsIContent**, nsNodeInfoManager*) src/content/base/src/nsTextNode.cpp:105
    #4 0x7fe1013802bd in nsHtml5TreeOperation::AppendText(unsigned short const*, unsigned int, nsIContent*, nsHtml5TreeOpExecutor*) src/parser/html/nsHtml5TreeOperation.cpp:164
    #5 0x7fe10138b506 in nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**) src/parser/html/nsHtml5TreeOperation.cpp:457
    #6 0x7fe1013a9502 in nsHtml5TreeOpExecutor::RunFlushLoop() src/parser/html/nsHtml5TreeOpExecutor.cpp:565
    #7 0x7fe1013e7a99 in nsHtml5ExecutorFlusher::Run() src/parser/html/nsHtml5StreamParser.cpp:129
    #8 0x7fe10831a32e in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:627
    #9 0x7fe107f8ff7f in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:221
Shadow byte and word:
  0x1ffc1a6d614d: fd
  0x1ffc1a6d6148: fd fd fd fd fd fd fd fd
More shadow bytes:
  0x1ffc1a6d6128: 00 00 00 00 00 00 00 00
  0x1ffc1a6d6130: 00 00 00 00 00 00 00 fb
  0x1ffc1a6d6138: fa fa fa fa fa fa fa fa
  0x1ffc1a6d6140: fa fa fa fa fa fa fa fa
=>0x1ffc1a6d6148: fd fd fd fd fd fd fd fd
  0x1ffc1a6d6150: fd fd fd fd fd fd fd fd
  0x1ffc1a6d6158: fa fa fa fa fa fa fa fa
  0x1ffc1a6d6160: fa fa fa fa fa fa fa fa
  0x1ffc1a6d6168: fd fd fd fd fd fd fd fd
Stats: 206M malloced (192M for red zones) by 303102 calls
Stats: 33M realloced by 19469 calls
Stats: 174M freed by 173854 calls
Stats: 135M really freed by 146679 calls
Stats: 214M (54832 full pages) mmaped in 405 calls
  mmaps   by size class: 7:147420; 8:34799; 9:14322; 10:5110; 11:6120; 12:1152; 13:768; 14:544; 15:192; 16:576; 17:452; 18:28; 19:34; 20:21;
  mallocs by size class: 7:204386; 8:50002; 9:23156; 10:7924; 11:9868; 12:2106; 13:1629; 14:1423; 15:297; 16:898; 17:1315; 18:38; 19:38; 20:22;
  frees   by size class: 7:118172; 8:20912; 9:16539; 10:4725; 11:7658; 12:1215; 13:1162; 14:1266; 15:189; 16:633; 17:1300; 18:28; 19:36; 20:19;
  rfrees  by size class: 7:104746; 8:16540; 9:10096; 10:3345; 11:6949; 12:988; 13:899; 14:1174; 15:145; 16:526; 17:1255; 18:11; 19:4; 20:1;
Stats: malloc large: 2608 small slow: 3842
==518== ABORTING

Comment 1

[(no longer active)]

Could be a dupe of bug 815477.

Comment 2

[Simon Montagu :smontagu]

Attachment: Patch

Comment 3

[Peter Van der Beken [:peterv]]

Could you explain why we're crashing, because I don't understand. It looks like the textNode is dead, but it's still in the dirAutoSetBy property of an element?

Comment 4

[Simon Montagu :smontagu]

Yes, exactly. As far as I can figure out, it's a race condition where the line "test1.innerHTML =..." causes the dirAutoSetBy property of test2 to change, but the timeOut'd "test2.setAttribute("dir", "&locale.dir;")" triggers a call to RecomputeDirectionality while the dirAutoSetBy is still pointing to the old textnode which has meanwhile been freed.

Comment 5

[Simon Montagu :smontagu]

https://tbpl.mozilla.org/?tree=Try&rev=4392e3725778 together with the patch for bug 815276 and tests for both bugs and bug 815477.

Comment 6

[Peter Van der Beken [:peterv]]

I still don't understand how the fix is right. ResetNodeDirection is called when setting the innerHTML because that removes the textNode child of test1, but WalkDescendantsSetDirectionFromText doesn't find a textNode. Shouldn't we at that point call RemoveElementFromMap on the textNode that's being removed?

Comment 7

[Peter Van der Beken [:peterv]]

Well, not call RemoveElementFromMap but

      rootNode->SetProperty(nsGkAtoms::dirAutoSetBy, aTextNode);
      rootNode->SetHasDirAutoSet();

Comment 8

[Simon Montagu :smontagu]

(In reply to Peter Van der Beken [:peterv] from comment #6)
> I still don't understand how the fix is right. ResetNodeDirection is called
> when setting the innerHTML because that removes the textNode child of test1,
> but WalkDescendantsSetDirectionFromText doesn't find a textNode. Shouldn't
> we at that point call RemoveElementFromMap on the textNode that's being
> removed?

But we already do, in effect, as a consequence of ResetNodeDirection returning PL_DHASH_REMOVE to EnumerateEntries

Comment 9

[Peter Van der Beken [:peterv]]

But that doesn't do the two lines from comment 7, does it?

Comment 10

[Simon Montagu :smontagu]

I'm not sure I understand comment 7. If WalkDescendantsSetDirectionFromText returns a text node, we do those two lines in AddEntry, called from AddEntryToMap. If it doesn't return a text node, why would we want to do them?

Comment 11

[Peter Van der Beken [:peterv]]

Woops, what I meant was

        rootNode->ClearHasDirAutoSet();
        rootNode->UnsetProperty(nsGkAtoms::dirAutoSetBy);

That is, I don't see where that happens if the text node that's stored in rootNode's dirAutoSetBy property is replaced with a new text node that doesn't have strong directional characters. Could you point it out?

Comment 12

[Simon Montagu :smontagu]

Attachment: Patch v.2

Now I see what was missing, sorry to be slow.

Comment 13

[Peter Van der Beken [:peterv]]

Comment on attachment 690402 [details] [diff] [review]
Patch v.2

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

We should check in the testcase as a crashtest too.

::: content/base/src/DirectionalityUtils.cpp
@@ +450,5 @@
>        mElements.Remove(aElement);
>  
> +      if (mElements.IsEmpty()) {
> +        aTextNode->ClearHasTextNodeDirectionalityMap();
> +      }

I think I'd actually prefer to not do this (and the change in nsCheapSets) since, unless I'm missing something, it seems unneeded to fix this bug. Just because mElements is empty doesn't mean that there is no map anymore since you don't actually remove the property. I'd like the flag to reflect the real state.

Comment 15

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0804d5dc4403
https://hg.mozilla.org/integration/mozilla-inbound/rev/1392f08d1000

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0804d5dc4403
https://hg.mozilla.org/mozilla-central/rev/1392f08d1000