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

RESOLVED FIXED

Status

()

Core
DOM
--
critical
RESOLVED FIXED
11 years ago
8 years ago

People

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

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
crash, testcase, verified1.8.0.12, verified1.8.1.4
Points:
---
Bug Flags:
blocking1.8.1.4 +
wanted1.8.1.x +
blocking1.8.0.12 +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(4 attachments)

(Reporter)

Description

11 years ago
Created attachment 255889 [details]
zipped testcase

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).
(Reporter)

Updated

11 years ago
Blocks: 306663

Updated

11 years ago
Blocks: 326633
No longer blocks: 306663
OS: Windows XP → All
Hardware: PC → All

Comment 1

11 years ago
Created attachment 255962 [details]
reduced testcase

Comment 2

11 years ago
Created attachment 255964 [details]
reduced testcase
(Reporter)

Comment 5

11 years ago
(In reply to comment #3)
> That was hard to reduce.
Heh, yeah, I had a feeling that was hard to reduce, thanks for doing!
(Reporter)

Comment 6

11 years ago
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)

Updated

11 years ago
Assignee: general → mats.palmgren
(Assignee)

Comment 7

11 years ago
Created attachment 256075 [details]
Testcase #4

A similar crash occurs from nsHTMLDocumentSH::DocumentAllTagsNewResolve()
after setting "document.all.tags.__proto__ = null"
(Assignee)

Comment 8

11 years ago
Created attachment 256077 [details] [diff] [review]
Patch rev. 1
(Assignee)

Comment 9

11 years ago
Created attachment 256080 [details] [diff] [review]
Patch rev. 1 (diff -w)

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?
(Assignee)

Updated

11 years ago
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+
(Assignee)

Comment 12

11 years ago
Fixed DocumentAllHelperNewResolve() as suggested above.
Checked in to trunk at 2007-02-22 18:18 PST.

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

Updated

11 years ago
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
(Assignee)

Updated

11 years ago
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+
(Assignee)

Comment 15

10 years ago
Checked in to branches at 2007-04-03 07:47 PDT
Keywords: fixed1.8.0.12, fixed1.8.1.4
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.
Keywords: fixed1.8.1.4 → verified1.8.1.4
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.
Keywords: fixed1.8.0.12 → verified1.8.0.12
Group: security
Flags: in-testsuite?

Comment 18

8 years ago
crash test landed
http://hg.mozilla.org/mozilla-central/rev/b8a601568262
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ LookupUCProperty]
You need to log in before you can comment on or make changes to this bug.