[FIX] Crash [@ nsXULDocument::SynchronizeBroadcastListener] with command

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
DOM
--
critical
VERIFIED FIXED
9 years ago
6 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: smaug)

Tracking

(4 keywords)

Trunk
mozilla1.9.2a1
crash, testcase, verified1.9.0.9, verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1 -
blocking1.9.0.9 +
wanted1.9.0.x +
wanted1.8.1.x -
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:investigate] post 1.8-branch, crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

594 bytes, application/vnd.mozilla.xul+xml
Details
3.93 KB, text/plain
Details
v2
3.29 KB, patch
Neil Deakin (not available until Aug 9)
: review+
neil@parkwaycc.co.uk
: superreview+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
See testcase, which crashes current trunk build and Firefox 3 within 100ms.

http://crash-stats.mozilla.com/report/index/69096ca8-9fbd-11dd-9496-001a4bd43e5c?p=1
0  	xul.dll  	nsXULDocument::SynchronizeBroadcastListener  	 content/xul/document/src/nsXULDocument.cpp:703
1 	xul.dll 	nsTArray_base::SwapArrayElements 	obj-firefox/xpcom/build/nsTArray.cpp:205
2 	xul.dll 	nsXULDocument::EndUpdate 	content/xul/document/src/nsXULDocument.cpp:3271
(Reporter)

Comment 1

9 years ago
Created attachment 344193 [details]
testcase
Created attachment 344512 [details]
backtrace from trunk crash

I'm having trouble determining the exploitability of this bug.  Here's a backtrace from the crash on a fresh Linux m-c debug build.
(Reporter)

Updated

9 years ago
Flags: blocking1.9.1?
(Assignee)

Updated

9 years ago
Assignee: nobody → Olli.Pettay
(Assignee)

Comment 3

9 years ago
Created attachment 347769 [details] [diff] [review]
create a list of to-be-broadcasted attributes before broadcasting

...and make sure the attribute still there in broadcaster before setting
it to listener.
Attachment #347769 - Flags: superreview?(neil)
Attachment #347769 - Flags: review?

Comment 4

9 years ago
Comment on attachment 347769 [details] [diff] [review]
create a list of to-be-broadcasted attributes before broadcasting

From whom did you intend review? ;-)
Attachment #347769 - Flags: superreview?(neil) → superreview+
(Assignee)

Updated

9 years ago
Attachment #347769 - Flags: review? → review?(enndeakin)
Comment on attachment 347769 [details] [diff] [review]
create a list of to-be-broadcasted attributes before broadcasting

>-            const nsAttrName* attrName = broadcaster->GetAttrNameAt(count);
>+        nsTArray<nsAttrNameInfo> attributes(count);
>+        for (PRUint32 i = 0; i < count; ++i) {
>+            const nsAttrName* attrName = broadcaster->GetAttrNameAt(i);
>             PRInt32 nameSpaceID = attrName->NamespaceID();
>             nsIAtom* name = attrName->LocalName();
> 
>             // _Don't_ push the |id|, |ref|, or |persist| attribute's value!
>             if (! CanBroadcast(nameSpaceID, name))
>                 continue;
> 
>+            attributes.AppendElement(nsAttrNameInfo(attrName->NamespaceID(),
>+                                                    attrName->LocalName(),
>+                                                    attrName->GetPrefix()));

You should either use the temporary variables in both cases or neither.
>-                              mInitialLayoutComplete);
>+            if (broadcaster->GetAttr(nameSpaceID, name, value)) {
>+              listener->SetAttr(nameSpaceID, name, attributes[count].mPrefix,
>+                                value, mInitialLayoutComplete);
>+            }

What if the attribute has a value on the listener but not on the broadcaster? Is this creating different behaviour in some situation?
(Assignee)

Comment 6

9 years ago
(In reply to comment #5)
> You should either use the temporary variables in both cases or neither.
Indeed, a left over from a previous (local) version. Fixing.

> What if the attribute has a value on the listener but not on the broadcaster?
> Is this creating different behaviour in some situation?
This is creating different behavior than currently only if someone 
modifies attribute using mutation listeners.
Note, if the element if removed (which if(GetAttr) checks), other code
should already remove the attribute from listener.
So this is IMO fixing the problem when removed attribute is recreated on
listener with empty value.
(Assignee)

Comment 7

9 years ago
Created attachment 347850 [details] [diff] [review]
v2
Attachment #347769 - Attachment is obsolete: true
Attachment #347850 - Flags: review?(enndeakin)
Attachment #347769 - Flags: review?(enndeakin)
(Assignee)

Updated

9 years ago
Summary: Crash [@ nsXULDocument::SynchronizeBroadcastListener] with command → [FIX] Crash [@ nsXULDocument::SynchronizeBroadcastListener] with command
Attachment #347850 - Flags: review?(enndeakin) → review+
Looks like we have a reviewed patch here.  Can we get this landed?
Keywords: checkin-needed
(Reporter)

Comment 9

9 years ago
The patch needs sr+, afaik.
Keywords: checkin-needed
Attachment #347850 - Flags: superreview?(neil)

Updated

9 years ago
Attachment #347850 - Flags: superreview?(neil) → superreview+
Keywords: checkin-needed
(Reporter)

Comment 10

9 years ago
The patch still isn't ready for check-in, afaik.
This bug has to be blocking1.9.1+ or the patch needs to get approval1.9.1 or both.
Ah, indeed!
Keywords: checkin-needed
(Assignee)

Updated

9 years ago
Attachment #347850 - Flags: approval1.9.1?
Attachment #347850 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 347850 [details] [diff] [review]
v2

a191=beltzner
This is ready to go, not marking as a blocker.
Flags: blocking1.9.1? → blocking1.9.1-
(Assignee)

Updated

9 years ago
Whiteboard: [needs-1.9.1-landing]
(Assignee)

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Reporter)

Comment 14

9 years ago
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081204 Minefield/3.2a1pre
Status: RESOLVED → VERIFIED
(Assignee)

Comment 15

9 years ago
Fixed on trunk and 1.9.1
Whiteboard: [needs-1.9.1-landing]
(Assignee)

Updated

9 years ago
Keywords: fixed1.9.1
Verified on 1.9.1 using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090204 Shiretoko/3.1b3pre

Martijn, can we add the testcase to the crashtest harness?
Flags: in-testsuite?
OS: Windows XP → All
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.2a1
Keywords: fixed1.9.1 → verified1.9.1
This is always a null deref in debug builds, but could it be uninitialized in optimized builds? Can't tell, let's just make sure it's fixed on 1.9.0
Flags: wanted1.9.0.x+
Flags: wanted1.8.1.x-
Flags: blocking1.9.0.8+
Whiteboard: [sg:investigate] post 1.8-branch
Olli: does this patch work for the 1.9.0 branch or will it require porting?
(Assignee)

Comment 19

8 years ago
Comment on attachment 347850 [details] [diff] [review]
v2

Applies to 1.9.0 with some small fuzz
Attachment #347850 - Flags: approval1.9.0.8?
Comment on attachment 347850 [details] [diff] [review]
v2

Approved for 1.9.0.8, a=dveditz
Attachment #347850 - Flags: approval1.9.0.8? → approval1.9.0.8+
(Assignee)

Updated

8 years ago
Keywords: fixed1.9.0.8
(Assignee)

Comment 21

8 years ago
Checking in content/xul/document/src/nsXULDocument.cpp;
/cvsroot/mozilla/content/xul/document/src/nsXULDocument.cpp,v  <--  nsXULDocument.cpp
new revision: 1.831; previous revision: 1.830
done
Verified for 1.9.0.8 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.8pre) Gecko/2009031904 GranParadiso/3.0.8pre. Crashes cleanly in 1.9.0.7.
Keywords: fixed1.9.0.8 → verified1.9.0.8
Group: core-security
Crash Signature: [@ nsXULDocument::SynchronizeBroadcastListener]
You need to log in before you can comment on or make changes to this bug.