crash [@ Decompile][@ js_GetSrcNoteOffset] with firebug/jQuery

VERIFIED FIXED

Status

()

Core
JavaScript Engine
P2
critical
VERIFIED FIXED
10 years ago
7 years ago

People

(Reporter: Brian Crowder, Assigned: Brian Crowder)

Tracking

({crash, verified1.9.0.1})

Trunk
crash, verified1.9.0.1
Points:
---
Bug Flags:
wanted1.9.1 +
blocking1.9.0.1 -
wanted1.9.0.x +
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [firebug-p1], crash signature, URL)

Attachments

(2 attachments)

(Assignee)

Description

10 years ago
Surfing to the URL specified causes crashes in Fx3 with firebug 1.2b3, while Decompiling a function from jQuery.


In debug, I get earlier assertions:

#0  JS_Assert (s=0x2d2144 "0", file=0x2db718 "/Users/crowder/mozilla/js/src/jsopcode.cpp", ln=2988) at /Users/crowder/mozilla/js/src/jsutil.cpp:63
#1  0x002714d9 in Decompile (ss=0xbfff84e4, pc=0x18b33890 "\b??9", nb=201, nextop=JSOP_NOP) at /Users/crowder/mozilla/js/src/jsopcode.cpp:2988
#2  0x00279017 in DecompileCode (jp=0x171a1900, script=0x18b33780, pc=0x18b33814 "9", len=201, pcdepth=0) at /Users/crowder/mozilla/js/src/jsopcode.cpp:4681
#3  0x0026c25e in js_DecompileFunction (jp=0x171a1900) at /Users/crowder/mozilla/js/src/jsopcode.cpp:4850
#4  0x001e7dcb in JS_DecompileFunction (cx=0x1762ebb0, fun=0x1672eaf0, indent=4) at /Users/crowder/mozilla/js/src/jsapi.cpp:4861
#5  0x1dc1d167 in jsdScript::GetFunctionSource (this=0x18b339c0, aFunctionSource=@0xbfff87c0) at /Users/crowder/mozilla/js/jsd/jsd_xpc.cpp:1282

The output that has already been printed into the sprinter looks like:
    base = 0x17199a50 "    function () {\n        item = $(this);\n        break;\n        $(\"> li:last-child\", item).remove().end();\n"

Which is an anonymous function in the Drupal code.


More coming up....
(Assignee)

Comment 1

10 years ago
If I open the firebug pane to the "net" tab, I'm able to hit an assertion here:

#0  JS_Assert (s=0x2db954 "top < ss->printer->script->depth", file=0x2db718 "/Users/crowder/mozilla/js/src/jsopcode.cpp", ln=886) at /Users/crowder/mozilla/js/src/jsutil.cpp:63
#1  0x0026a198 in PushOff (ss=0xbfff9ad4, off=89, op=JSOP_ZERO) at /Users/crowder/mozilla/js/src/jsopcode.cpp:886
#2  0x00278dce in Decompile (ss=0xbfff9ad4, pc=0x1f9ba1fb ">HE", nb=826, nextop=JSOP_NOP) at /Users/crowder/mozilla/js/src/jsopcode.cpp:4599
#3  0x00279017 in DecompileCode (jp=0x1eba25d0, script=0x1f9b9e00, pc=0x1f9b9f3c "T", len=826, pcdepth=0) at /Users/crowder/mozilla/js/src/jsopcode.cpp:4681
#4  0x0026c25e in js_DecompileFunction (jp=0x1eba25d0) at /Users/crowder/mozilla/js/src/jsopcode.cpp:4850
#5  0x001e7dcb in JS_DecompileFunction (cx=0x191bb750, fun=0x16886850, indent=4) at /Users/crowder/mozilla/js/src/jsapi.cpp:4861
#6  0x1d366167 in jsdScript::GetFunctionSource (this=0x17b4ac30, aFunctionSource=@0xbfff9db0) at /Users/crowder/mozilla/js/jsd/jsd_xpc.cpp:1282


The output in the sprinter during this crash looks like:

    base = 0x6b41b0 "    function (i, elem) {\n"

(Assignee)

Comment 2

10 years ago
If I ignore the assertions hit in comment 0, I eventually crash here:

#0  0x0020ddc6 in js_GetSrcNoteOffset (sn=0x0, which=0) at /Users/crowder/mozilla/js/src/jsemit.cpp:6671
#1  0x00271b24 in Decompile (ss=0xbfff74c4, pc=0x174ac2f5 "\a", nb=244, nextop=JSOP_NOP) at /Users/crowder/mozilla/js/src/jsopcode.cpp:3072
#2  0x00279017 in DecompileCode (jp=0x200810e0, script=0x174ac210, pc=0x174ac2b4 "?", len=244, pcdepth=0) at /Users/crowder/mozilla/js/src/jsopcode.cpp:4681
#3  0x0026c25e in js_DecompileFunction (jp=0x200810e0) at /Users/crowder/mozilla/js/src/jsopcode.cpp:4850
#4  0x001e7dcb in JS_DecompileFunction (cx=0x17f666a0, fun=0x1672b2a0, indent=4) at /Users/crowder/mozilla/js/src/jsapi.cpp:4861
#5  0x1d3c0167 in jsdScript::GetFunctionSource (this=0x174ac490, aFunctionSource=@0xbfff77a0) at /Users/crowder/mozilla/js/jsd/jsd_xpc.cpp:1282

And the sprinter here looks like:

    base = 0x20081160 "    function () {\n        opt = jQuery.extend({}, optall);\n        p;\n        hidden = jQuery(this).is(\":hidden\");\n        self = this;\n"


This resembles a crash-reporter crash linked by shaver from IRC:
http://crash-stats.mozilla.com/report/index/607737dc-3e0d-11dd-baf6-001321b13766
(Assignee)

Comment 3

10 years ago
To reproduce this with firebug, I turned on ALL of the following features:

	Console	Support for Console logging.
	Script	Support for JavaScript debugging.
	Net	Support for Network monitoring.

In the initial firebug setup tab.

Updated

10 years ago
Severity: normal → critical
Keywords: crash
Summary: crash (@ Decompile) with firebug/jQuery → crash [@ Decompile][@ js_GetSrcNoteOffset] with firebug/jQuery

Comment 4

10 years ago
Looks like the same as a report from Firebug user:
http://groups.google.com/group/firebug/browse_thread/thread/615f476f9be139ee#

I confirmed the crash using
http://image.baidu.com/i?tn=baiduimage&ct=201326592&lm=-1&cl=2&word=%C1%F5%B7%BC%B7%C6
and hitting the number [2] in the list near the bottom. My stack was:

http://crash-stats.mozilla.com/report/index/3833bec0-3e8e-11dd-9344-001cc45a2ce4?p=1
(Assignee)

Comment 5

10 years ago
Maybe even blocking1.9.1?
Assignee: general → crowder
Flags: wanted1.9.1?
Priority: -- → P2
Crowder, can you give some more details on why you think this should block?  Looking for:  common usage scenario?  exploitable?  Do you think this should go in 1.9.0.x?  
(Assignee)

Comment 7

10 years ago
I'm not clear on whether it's exploitable or not, but it's bound to be very frustrating for firebug users, as it seems to be a relatively frequent crash, hit on common sites.  Firefox + Firebug + Baidu == crash seems bad enough to block, to me (see comment #4).  I haven't had a chance to look at this full-time, but I've poked at it a bit over the last few days and it's weird (at least, to me, with limited decompiler experience, it is).
(Assignee)

Comment 8

10 years ago
It's also worth mentioning that the code causing the crash in at least the first case (comment 0) is from jQuery, which is widely used.
This is sounding more and more like 1.9.0.1 fodder.  Beltzner, what do you think?
Flags: blocking1.9.0.1?

Updated

10 years ago
Whiteboard: [firebug-p1]
(Assignee)

Comment 10

10 years ago
Alright, here's what I understand so far.

The Decompilation that's occurring in the browser-debugger is getting confused
by what looks like a bogus or empty source-note-cache entry.

As we're decompiling opcodes, we wind up on a JSOP_IFEQ, then try to lookup a
src-note, the lookup descends into this section of js_GetSrcNoteCached:

    if (JS_GSN_CACHE(cx).script == script) {
        JS_METER_GSN_CACHE(cx, hits);
        entry = (GSNCacheEntry *)
                JS_DHashTableOperate(&JS_GSN_CACHE(cx).table, pc,
                                     JS_DHASH_LOOKUP);
        return entry->sn;
    }

The problem is that at this point, somehow, entry->sn is NULL.  If I modify
this to only return entry->sn if entry->sn is !NULL, then the crash disappears.

It's not clear to me how the source note cache is getting misled here, unless
it is because the context coming from the debugger is somehow different than
the context coming from the script that's failing.
(Assignee)

Comment 11

10 years ago
Created attachment 326743 [details] [diff] [review]
tag the cache from script->code, instead of script

This causes the cache to be validated based on its association to the bytecode vector itself, rather than the script pointer, since we sometimes rewrite the bytecode vector (due to traps, especially).
Attachment #326743 - Flags: review?(igor)

Updated

10 years ago
Attachment #326743 - Flags: review?(igor) → review+
Comment on attachment 326743 [details] [diff] [review]
tag the cache from script->code, instead of script

>diff -r fc0d7b238e44 js/src/jscntxt.h
>--- a/js/src/jscntxt.h	Tue Jun 24 13:45:12 2008 -0700
>+++ b/js/src/jscntxt.h	Wed Jun 25 11:12:24 2008 -0700
>@@ -61,10 +61,12 @@
> 
> /*
>  * js_GetSrcNote cache to avoid O(n^2) growth in finding a source note for a
>- * given pc in a script.
>+ * given pc in a script.  We use the script->code pointer to tag the cache,

Nit: eschew French spacing in comments.

/be
(Assignee)

Comment 13

10 years ago
changeset:   15530:4aeb5932fc5a

Woops.  Actually landed this before Brendan's nit.  Will fix in hg.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

10 years ago
changeset:   15532:f171c57e016e  (for the comment nit)
Not 1.9.0.1 fodder, but maybe 1.9.0.2.
Flags: wanted1.9.1?
Flags: wanted1.9.1+
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.1?
Flags: blocking1.9.0.1-
Whiteboard: [firebug-p1] → [firebug-p1][1.9.0.2?]

Comment 16

10 years ago
We are sad....
Why wasn't this 1.9.0.1 fodder?  It's the dominant crash for Firebug users, easy to trigger with jQuery sites. :/

(See comments 7, 8, 9 from a week ago.)

It's too late for 3.0.1 now, but I'd like to know what more this bug needed to be in order to block, I must say...
Is this really too late? I'm with shaver, there has to be a specific reason for not taking it. We have more than one bit of brain to think about risk and fixes, even in a firedrill situation, which we're not really in.

/be
(Assignee)

Updated

10 years ago
Attachment #326743 - Flags: approval1.9.0.1?
(In reply to comment #18)
> Is this really too late? I'm with shaver, there has to be a specific reason for
> not taking it. We have more than one bit of brain to think about risk and
> fixes, even in a firedrill situation, which we're not really in.

Shaver emailed release-drivers, which was the right thing to do, and we checked with QA and determined that the risk/reward was worth it. Please land ASAP so it can bake.

In the future, to ensure your patch is considered for a branch release, please nominate it for approval and restate (or point to comments) the assessed risk and reward.

(In reply to comment #18)
> not taking it. We have more than one bit of brain to think about risk and
> fixes, even in a firedrill situation, which we're not really in.

In fact, we sort of are. As has been declared in several places, 3.0.1 is a quick turn release in order to hurry out three security fixes. It's not a firedrill since it isn't a "real" zero day situation, but we are treating it as if it were.
Comment on attachment 326743 [details] [diff] [review]
tag the cache from script->code, instead of script

a=beltzner, please land before noon PDT today.
Attachment #326743 - Flags: approval1.9.0.1? → approval1.9.0.1+
(Assignee)

Comment 21

10 years ago
Created attachment 327626 [details] [diff] [review]
the patch for CVS head

Recording for posterity.  The patch applies trivially in cvs.m.o with only a bit of fuzz, but just in case it needs to be backed out, here it is.

jscntxt.h:  3.205
jsscript.c: 3.181
Beltzner: I don't know if you are arguing, or agreeing. We took the fix (yay, thanks for approving), so we're not in the kind of firedrill where no risk apart from one fix can be tolerated. But it seems that we were acting as if -- why?

/be
(Assignee)

Updated

10 years ago
Keywords: fixed1.9.0.1
Whiteboard: [firebug-p1][1.9.0.2?] → [firebug-p1]

Comment 23

10 years ago
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.1) Gecko/2008070206 Firefox/3.0.1

Verified using given URL and settings indicated in comment #3. No longer crashes.
Status: RESOLVED → VERIFIED

Updated

10 years ago
Keywords: fixed1.9.0.1 → verified1.9.0.1

Updated

10 years ago
Flags: in-testsuite-
Flags: in-litmus-
Crash Signature: [@ Decompile] [@ js_GetSrcNoteOffset]
You need to log in before you can comment on or make changes to this bug.