Closed Bug 815500 Opened 7 years ago Closed 7 years ago

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


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

Not set



Tracking Status
firefox19 --- unaffected
firefox20 + fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected
b2g18 --- unaffected


(Reporter: inferno, Assigned: smontagu)



(6 keywords, Whiteboard: [asan][adv-main20-])


(2 files, 1 obsolete file)

Attached file Testcase
Reproduces on trunk, looks like regression from

==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/
    #27 0x7fe108602c75 in MessageLoop::RunHandler() src/ipc/chromium/src/base/
    #28 0x7fe108602b5b in MessageLoop::Run() src/ipc/chromium/src/base/
    #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
Blocks: DirAuto
Severity: normal → critical
Component: General → Layout: Text
Product: Firefox → Core
Whiteboard: [asan]
Could be a dupe of bug 815477.
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → smontagu
Attachment #685810 - Flags: review?(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?
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. together with the patch for bug 815276 and tests for both bugs and bug 815477.
Blocks: 815477
Flags: sec-bounty?
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?
Well, not call RemoveElementFromMap but

      rootNode->SetProperty(nsGkAtoms::dirAutoSetBy, aTextNode);
(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
But that doesn't do the two lines from comment 7, does it?
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?
Woops, what I meant was


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?
Attached patch Patch v.2Splinter Review
Now I see what was missing, sorry to be slow.
Attachment #685810 - Attachment is obsolete: true
Attachment #685810 - Flags: review?(peterv)
Attachment #690402 - Flags: review?(peterv)
Blocks: 819014
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.
Attachment #690402 - Flags: review?(peterv) → review+
Duplicate of this bug: 819014
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Flags: sec-bounty? → sec-bounty+
Whiteboard: [asan] → [asan][adv-main20+]
Whiteboard: [asan][adv-main20+] → [asan][adv-main20-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.