Closed Bug 371124 Opened 17 years ago Closed 17 years ago

Crash [@ LookupUCProperty] with unminimised testcase, using stirdom scriptfuzzer and xbl bindings

Categories

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

defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: martijn.martijn, Assigned: MatsPalmgren_bugz)

Details

(4 keywords, Whiteboard: [sg:critical?] null-deref on 1.8 branch, uninitialized mem on 1.8.0.10)

Crash Data

Attachments

(4 files)

Attached file zipped testcase (obsolete) —
To reproduce the crash:
- Extract the zipped testcase
- Open file 'parentframe.htm'

Usually the crash happens within 20 seconds or so.

Talkback ID: TB29540796Z
LookupUCProperty  [mozilla/js/src/jsapi.c, line 2642]
JS_HasUCProperty  [mozilla/js/src/jsapi.c, line 3009]
nsHTMLExternalObjSH::GetProperty  [mozilla/dom/src/base/nsdomclassinfo.cpp, line 8850]
XPC_WN_Helper_GetProperty  [mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp, line 962]
js_GetProperty  [mozilla/js/src/jsobj.c, line 3515]
js_ValueToIterator  [mozilla/js/src/jsiter.c, line 395]
js_Interpret  [mozilla/js/src/jsinterp.c, line 2764]

I haven't tried minimising it yet (this could be very tough) or getting a regression range (which also might be difficult, because I could encounter crashes unrelated to this crash).
Marking security sensitive, because it also crashes on branch (with a different stack).
Blocks: stirdom
No longer blocks: stirdom
OS: Windows XP → All
Hardware: PC → All
Attached file reduced testcase (obsolete) —
Attached file reduced testcase
(In reply to comment #3)
> That was hard to reduce.
Heh, yeah, I had a feeling that was hard to reduce, thanks for doing!
With the reduced testcase, the regression range I get is between 2006-05-18/2006-05-23 on trunk and 2006-07-06/2006-07-07 on branch, so I think this  a regression from the js1.7 landing, see:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-07-06+03&maxdate=2006-07-07+09&cvsroot=%2Fcvsroot
Assignee: general → mats.palmgren
Attached file Testcase #4
A similar crash occurs from nsHTMLDocumentSH::DocumentAllTagsNewResolve()
after setting "document.all.tags.__proto__ = null"
Attached patch Patch rev. 1Splinter Review
This fixes the crash for both testcases on trunk and branches.
On the 1.8.0 branch we don't reach nsHTMLExternalObjSH::GetProperty()
with attachment 255964 [details] for some reason... the code is the same though
so the crash would likely occur if we do get there with a null proto...
Attachment #256080 - Flags: superreview?(bzbarsky)
Attachment #256080 - Flags: review?(bzbarsky)
I don't really know this code well enough to do a fast review.  Ask jst?
Attachment #256080 - Flags: superreview?(jst)
Attachment #256080 - Flags: superreview?(bzbarsky)
Attachment #256080 - Flags: review?(jst)
Attachment #256080 - Flags: review?(bzbarsky)
Comment on attachment 256080 [details] [diff] [review]
Patch rev. 1 (diff -w)

- In nsHTMLDocumentSH::DocumentAllHelperNewResolve():

+    JSObject *proto = ::JS_GetPrototype(cx, obj);
+    if (proto) {

Rather than going past this at all in the case where we have no proto I think I'd prefer to simply bail early here (return JS_TRUE). It's not like we care to expose anything at all in any way if someone sets our document prototype's prototype (i.e. the document.all helper protos proto) to null. If we do what this patch does then an element with name="getElementById" would be reflected as document.getElementById, and that's IMO not desired here.

r+sr=jst with that change.
Attachment #256080 - Flags: superreview?(jst)
Attachment #256080 - Flags: superreview+
Attachment #256080 - Flags: review?(jst)
Attachment #256080 - Flags: review+
Fixed DocumentAllHelperNewResolve() as suggested above.
Checked in to trunk at 2007-02-22 18:18 PST.

-> FIXED
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Flags: blocking1.8.1.3?
I get a different stack in FF2.0.0.2 and FF1.5.0.10 -- FF2 doesn't look too bad, a null-deref in LookupUCProperty. In 1.5.0.10 I crash on an uninitialized cx->fp (0xCCCCCCCC) in js_AllocStack()
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.0.11?
Whiteboard: [sg:critical?] null-deref on 1.8 branch, uninitialized mem on 1.8.0.10
Attachment #256080 - Flags: approval1.8.1.3?
Attachment #256080 - Flags: approval1.8.0.11?
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12+
Comment on attachment 256080 [details] [diff] [review]
Patch rev. 1 (diff -w)

approved for 1.8/1.8.0 branches, a=dveditz for drivers
Attachment #256080 - Flags: approval1.8.1.3?
Attachment #256080 - Flags: approval1.8.1.3+
Attachment #256080 - Flags: approval1.8.0.12?
Attachment #256080 - Flags: approval1.8.0.12+
Checked in to branches at 2007-04-03 07:47 PDT
While trying to verify this bug, I crashed using the first zipped testcase. My Talkback Incident ID=31937866. I was using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.4pre) Gecko/2007050804 BonEcho/2.0.0.4pre on Windows Vista. Martijn filed Bug 371125 to address this. The other testcases do not crash, so verifiying this fixed on the 1.8 branch and adding verified keyword, per discussion with gavin and martijn on IRC.
verified on the 1.8.0 branch using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.0.12pre) Gecko/20070508 Firefox/1.5.0.12pre. I did not crash with the testcases listed, including the original zipped testcase. Adding branch verified keyword.
Group: security
Flags: in-testsuite?
crash test landed
http://hg.mozilla.org/mozilla-central/rev/b8a601568262
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ LookupUCProperty]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.