potential crash [@ nsDocument::AdoptNode] dereferencing oldDocument

RESOLVED FIXED

Status

()

Core
DOM
--
minor
RESOLVED FIXED
12 years ago
12 days ago

People

(Reporter: timeless, Assigned: timeless)

Tracking

(Blocks: 1 bug, {coverity})

Trunk
x86
Linux
coverity
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 obsolete attachments)

(Assignee)

Description

12 years ago
 
(Assignee)

Comment 1

12 years ago
Created attachment 238874 [details] [diff] [review]
optimistic
Assignee: general → timeless
Status: NEW → ASSIGNED
Attachment #238874 - Flags: review?(mats.palmgren)
(Assignee)

Comment 2

12 years ago
Created attachment 238875 [details] [diff] [review]
pesimistic
Attachment #238875 - Flags: review?(mats.palmgren)
Comment on attachment 238875 [details] [diff] [review]
pesimistic

We would definitely need the pessimistic patch, or we would crash in
nsContentUtils::GetContextFromDocument():
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/base/src/nsContentUtils.cpp&rev=1.184&root=/cvsroot&mark=934#910
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/base/src/nsContentUtils.cpp&rev=1.184&root=/cvsroot&mark=861#859

I'm not sure we want to continue if |oldDocument| is null though -
won't nsNodeUtils::Adopt() think this is the "sameDocument" case
when the JS context/scopes are null?
I don't know this code well enough to say what should happen...

I'm guessing something like this would be the safest approach:

  nsIDocument *oldDocument = adoptedNode->GetOwnerDoc();
+ if (!oldDocument) {
+   BlastSubtreeToPieces(adoptedNode);
+   return NS_ERROR_xxx;
+ }
Attachment #238875 - Flags: review?(mats.palmgren) → review?(peterv)
Attachment #238874 - Attachment is obsolete: true
Attachment #238874 - Flags: review?(mats.palmgren)
Comment on attachment 238875 [details] [diff] [review]
pesimistic

nsNodeUtils::Adopt will work correctly (no old ownerDocument, so no need to reparent wrappers).
Attachment #238875 - Flags: superreview+
Attachment #238875 - Flags: review?(peterv)
Attachment #238875 - Flags: review+
(Assignee)

Updated

12 years ago
Summary: hopefully useless null check in [@ nsDocument::AdoptNode] of oldDocument → potential crash [@ nsDocument::AdoptNode] dereferencing oldDocument
(Assignee)

Comment 5

12 years ago
Comment on attachment 238875 [details] [diff] [review]
pesimistic

mozilla/content/base/src/nsDocument.cpp 	3.686
Attachment #238875 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.