Closed Bug 544372 Opened 10 years ago Closed 10 years ago

Uninitialised value use in nsNodeIterator::NodePointer::AdjustAfterRemoval

Categories

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

1.9.2 Branch
x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- wontfix

People

(Reporter: jseward, Assigned: sicking)

References

(Blocks 1 open bug)

Details

(Keywords: valgrind)

Attachments

(2 files, 1 obsolete file)

http://acid3.acidtests.org causes reports of several uninitialised
value errors.  This happens on {x86,amd64}-linux and x86-darwin.
Happens on both 1.9.2 and mozilla-central.  The errors have to do with
nsNodeIterator and look like this:

Conditional jump or move depends on uninitialised value(s)
   at 0x58553AA: nsNodeIterator::NodePointer::AdjustAfterRemoval(nsINode*, nsINode*, nsIContent*, int)+46 (nsNodeIterator.cpp:109)
   by 0x5855456: nsNodeIterator::ContentRemoved(nsIDocument*, nsIContent*, nsIContent*, int)+58 (nsNodeIterator.cpp:376)
   by 0x5856831: nsNodeUtils::ContentRemoved(nsINode*, nsIContent*, int)+261 (nsNodeUtils.cpp:181)
   by 0x584BE7F: nsGenericElement::doRemoveChildAt(unsigned int, int, nsIContent*, nsIContent*, nsIDocument*, nsAttrAndChildArray&, int)+1119 (nsGenericElement.cpp:3338)
   by 0x584BF62: nsGenericElement::RemoveChildAt(unsigned int, int, int)+114 (nsGenericElement.cpp:3266)
   by 0x5845968: nsGenericElement::doRemoveChild(nsIDOMNode*, nsIContent*, nsIDocument*, nsIDOMNode**)+138 (nsGenericElement.cpp:3984)
   by 0x58459D7: nsGenericElement::RemoveChild(nsIDOMNode*, nsIDOMNode**)+43 (nsGenericElement.cpp:3503)
   by 0x58CCE66: nsHTMLHtmlElement::RemoveChild(nsIDOMNode*, nsIDOMNode**)+8 (nsHTMLHtmlElement.cpp:57)
   by 0x553BB7A: nsIDOMNode_RemoveChild(JSContext*, unsigned int, long*)+332 (dom_quickstubs.cpp:3082)
   by 0x68323E5: js_Interpret+42069 (jsops.cpp:2208)
   by 0x6836F7C: js_Invoke+1776 (jsinterp.cpp:1368)
   by 0x68373FB: js_InternalInvoke+127 (jsinterp.cpp:1423)
 Uninitialised value was created by a heap allocation
   at 0x4C25D41: operator new(unsigned long)+130 (vg_replace_malloc.c:261)
   by 0x58272A5: nsDocument::CreateNodeIterator(nsIDOMNode*, unsigned int, nsIDOMNodeFilter*, int, nsIDOMNodeIterator**)+137 (nsDocument.cpp:4790)
   by 0x553D775: nsIDOMDocumentTraversal_CreateNodeIterator(JSContext*, unsigned int, long*)+627 (dom_quickstubs.cpp:11590)
   by 0x68323E5: js_Interpret+42069 (jsops.cpp:2208)
   by 0x6836F7C: js_Invoke+1776 (jsinterp.cpp:1368)
   by 0x68373FB: js_InternalInvoke+127 (jsinterp.cpp:1423)
   by 0x67F3260: JS_CallFunctionValue+28 (jsapi.cpp:5112)
   by 0x59A52EA: nsJSContext::CallEventHandler(nsISupports*, void*, void*, nsIArray*, nsIVariant**)+484 (nsJSEnvironment.cpp:2134)
   by 0x59BED00: nsGlobalWindow::RunTimeout(nsTimeout*)+1036 (nsGlobalWindow.cpp:8075)
   by 0x59BF0AB: nsGlobalWindow::TimerCallback(nsITimer*, void*)+23 (nsGlobalWindow.cpp:8409)
   by 0x5F0AFC4: nsTimerImpl::Fire()+382 (nsTimerImpl.cpp:427)
   by 0x5F0B09D: nsTimerEvent::Run()+49 (nsTimerImpl.cpp:519)
possibly relevant mozconfig bits:

ac_add_options --enable-optimize="-g -O -freorder-blocks"
ac_add_options --disable-jemalloc
This appears to be to do with the constructors for 'struct
NodePointer', viz, NodePointer::NodePointer() and
NodePointer::NodePointer(nsINode *aNode, PRBool aBeforeNode).

The former only initialises 1 field out of 4, the latter initialises 2
out of 4.  The blunt-instrument patch below causes both constructors
to initialise all 4 fields, and this stops Valgrind complaining.  I
have no idea if this is a valid fix or merely masks some other
problem.  But Fx does still seem to work with it applied.


--- a/content/base/src/nsNodeIterator.cpp       Fri Jan 29 18:41:39 2010 -0800
+++ b/content/base/src/nsNodeIterator.cpp       Fri Feb 05 02:01:15 2010 +0100
@@ -60,6 +60,7 @@
                                          PRBool aBeforeNode) :
     mNode(aNode),
     mBeforeNode(aBeforeNode)
+,mIndexInParent(0),mNodeParent(0)
 {
 }

diff -r 5d7909e22a27 content/base/src/nsNodeIterator.h
--- a/content/base/src/nsNodeIterator.h Fri Jan 29 18:41:39 2010 -0800
+++ b/content/base/src/nsNodeIterator.h Fri Feb 05 02:01:15 2010 +0100
@@ -74,7 +74,7 @@

 private:
     struct NodePointer {
-        NodePointer() : mNode(nsnull) {};
+        NodePointer() : mNode(nsnull)  ,mNodeParent(0),mBeforeNode(0),mIndexInParent(0)  {};
         NodePointer(nsINode *aNode, PRBool aBeforeNode);

         PRBool MoveToNext(nsINode *aRoot);
Component: Layout → DOM
QA Contact: layout → general
Blocks: 132824
This comes from bug 132824. Jonas, should we just initialize those members?
Attached patch Patch to fix (obsolete) — Splinter Review
I think this is the right fix. Should add a test as well. Also included a tiny perf optimization in nsNodeIterator::NodePointer::AdjustAfterRemoval
Attachment #426562 - Flags: review?(peterv)
Isn't the constructor with arguments only called with Root as the argument? So the constructor should be able to force the parent to null. Could put an assert to check that the parent is actually null.
No, the root that we initialize to can be any node in the tree and thus have a parent. See

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#4942

However it really shouldn't matter what the parent index is of the root, since we should never need to even look at the nodes outside our root.

But the current code actually does. It looks like AdjustAfterRemoval can adjust mPointer.mNode to be outside mRoot. That is a bug (though iirc the spec is vague on the matter).

Craig: Mind looking into fixing this aspect too?
Comment on attachment 426562 [details] [diff] [review]
Patch to fix

Don't forget the test.
Attachment #426562 - Flags: review?(peterv) → review+
(In reply to comment #6)
> No, the root that we initialize to can be any node in the tree and thus have a
> parent. See
> 
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#4942
> 
> However it really shouldn't matter what the parent index is of the root, since
> we should never need to even look at the nodes outside our root.
> 
> But the current code actually does. It looks like AdjustAfterRemoval can adjust
> mPointer.mNode to be outside mRoot. That is a bug (though iirc the spec is
> vague on the matter).
> 
> Craig: Mind looking into fixing this aspect too?

Can you be more specific? AdjustAfterRemoval takes the root as an argument and makes some effort to prevent it from walking past it. Though I can't say for sure it covers everything. It's been too long since I wrote this code.
(In reply to comment #4)
> Created an attachment (id=426562) [details]
> Patch to fix
> 
> I think this is the right fix. Should add a test as well. Also included a tiny
> perf optimization in nsNodeIterator::NodePointer::AdjustAfterRemoval

Yes, it fixes the problem for me.  +1 for it.
Attached patch Patch v2Splinter Review
Ah, now I understand this code better. I missed the fact that we only get notified about mutations happening under the root.

The code can result in mNodeParent dangling in other cases too (when we traverse up to the root). However the code is actually correct in that while we sometimes read an uninitialized mNodeParent/mIndexInParent, we never actually use it for anything.
Attachment #426562 - Attachment is obsolete: true
Attachment #428803 - Flags: review?(peterv)
Comment on attachment 428803 [details] [diff] [review]
Patch v2

>+    // The mNode != aRoot check isn't strictly needed as if mNode is the root,

s/as/because/

>+        // mNodes index in mNodeParent. Uninitialized if mNodeParent is null or

mNode's
Attachment #428803 - Flags: review?(peterv) → review+
Checked in
http://hg.mozilla.org/mozilla-central/rev/75f0ca59e730

Thanks for finding this!
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 549224
Jonas, thanks for the Patch v2.  It certainly reduces the number of
UMRs reported, but unfortunately does not get rid of all of them.  In
particular I am now seeing the following error, which looks closely
related.  Notice that the point the uninitialised value was created --
in nsDocument::CreateNodeIterator -- is the same as it was originally.
So it looks like a bit more uninitialised-value leakage.

   at 0x5934685: nsNodeIterator::ContentInserted(nsIDocument*, nsIContent*, nsIContent*, int) (nsNodeIterator.cpp:103)
   by 0x593665C: nsNodeUtils::ContentInserted(nsINode*, nsIContent*, int) (nsNodeUtils.cpp:159)
   by 0x592CDB0: nsGenericElement::doInsertChildAt(nsIContent*, unsigned int, int, nsIContent*, nsIDocument*, nsAttrAndChildArray&) (nsGenericElement.cpp:3307)
   by 0x592006D: nsGenericDOMDataNode::SplitData(unsigned int, nsIContent**, int) (nsGenericDOMDataNode.cpp:897)
   by 0x5920141: nsGenericDOMDataNode::SplitText(unsigned int, nsIDOMText**) (nsGenericDOMDataNode.cpp:908)
   by 0x59419B0: nsRange::InsertNode(nsIDOMNode*) (nsRange.cpp:1819)
   by 0x6103EC3: NS_InvokeByIndex_P (xptcinvoke_x86_64_unix.cpp:208)
   by 0x556E377: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (xpcwrappednative.cpp:2727)
   by 0x55739E7: XPC_WN_CallMethod(JSContext*, JSObject*, unsigned int, long*, long*) (xpcwrappednativejsops.cpp:1770)
   by 0x6B19D56: js_Invoke (jsinterp.cpp:1370)
   by 0x6B0A07F: js_Interpret (jsops.cpp:2277)
   by 0x6B19FF7: js_Invoke (jsinterp.cpp:1378)
 Uninitialised value was created by a heap allocation
   at 0x4C25D41: operator new(unsigned long) (vg_replace_malloc.c:261)
   by 0x5903E13: nsDocument::CreateNodeIterator(nsIDOMNode*, unsigned int, nsIDOMNodeFilter*, int, nsIDOMNodeIterator**) (nsDocument.cpp:4820)
   by 0x55D5218: nsIDOMDocumentTraversal_CreateNodeIterator(JSContext*, unsigned int, long*) (dom_quickstubs.cpp:18679)
   by 0x6B104F3: js_Interpret (jsops.cpp:2243)
   by 0x6B19FF7: js_Invoke (jsinterp.cpp:1378)
   by 0x6B1A567: js_InternalInvoke (jsinterp.cpp:1435)
   by 0x6ACEFB9: JS_CallFunctionValue (jsapi.cpp:4951)
   by 0x5AB0B18: nsJSContext::CallEventHandler(nsISupports*, void*, void*, nsIArray*, nsIVariant**) (nsJSEnvironment.cpp:2164)
   by 0x5AC6EF3: nsGlobalWindow::RunTimeout(nsTimeout*) (nsGlobalWindow.cpp:8362)
   by 0x5AC7300: nsGlobalWindow::TimerCallback(nsITimer*, void*) (nsGlobalWindow.cpp:8706)
   by 0x60F7FB5: nsTimerImpl::Fire() (nsTimerImpl.cpp:427)
   by 0x60F805F: nsTimerEvent::Run() (nsTimerImpl.cpp:519)
Ugh, yeah, we need the same |mNode != aRoot| check there. You wanna write up a patch?
Really you could immediately return from both functions if mNode == aRoot. Just combine it with the !mNode check at the top. The ContentIsDescendantOf check will always exit the function if mNode is root anyway. So if you're going to check for root you might as well just bail out right away.
This patch fixes the insertion function. Also changes the removal function to just do the root check up front and skip the rest of the function.
Attachment #429474 - Flags: review?
Attachment #429474 - Flags: review? → review?(jonas)
Attachment #429474 - Flags: review?(jonas) → review+
Should this be backported to the branches?
Assignee: nobody → jonas
I don't think that's important given that there was no behavioural bug here.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.