Last Comment Bug 416705 - throw from xml filter leaves pending block objects unput
: throw from xml filter leaves pending block objects unput
Status: VERIFIED FIXED
[sg:high]
: verified1.8.1.13
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Linux
: P1 critical (vote)
: ---
Assigned To: Igor Bukanov
:
:
Mentors:
Depends on: 309894 420880
Blocks:
  Show dependency treegraph
 
Reported: 2008-02-10 10:45 PST by Igor Bukanov
Modified: 2008-04-21 11:26 PDT (History)
7 users (show)
brendan: blocking1.9+
dveditz: blocking1.8.1.13-
brendan: wanted1.8.1.x+
asac: blocking1.8.0.next-
bob: in‑testsuite+
bob: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
js1_7/regress/regress-416705.js (2.51 KB, text/plain)
2008-02-22 03:06 PST, Bob Clary [:bc:]
no flags Details
fix for 1.8 (4.79 KB, patch)
2008-02-29 14:23 PST, Igor Bukanov
no flags Details | Diff | Splinter Review
fix for 1.8 for real (1.49 KB, patch)
2008-02-29 14:26 PST, Igor Bukanov
brendan: review+
dveditz: approval1.8.1.13+
Details | Diff | Splinter Review
very targeted fix for 1.8 (1.73 KB, patch)
2008-03-04 12:49 PST, Igor Bukanov
brendan: review+
dveditz: approval1.8.1.13+
Details | Diff | Splinter Review

Description Igor Bukanov 2008-02-10 10:45:10 PST
Currently js_FilterXMLList after calling js_Interpret resets the scope chain without calling js_PutBlockObject on the pending block objects left from a thrown exception. The following demonstrates the problem:

~/m/ff/mozilla $ cat ~/m/y.js
var g;

function f()
{
    try {
        <><a/><b/></>.(let (a=1, b = 2, c = 3)
                       (g = function() { a += b+c; return a; }, xxx));
    } catch (e) {
    }
}

f();
var actual = g();
if (actual !== 6)
    throw "Bad result: "+uneval(actual);
~/m/ff/mozilla $ ~/m/ff/mozilla/js/src/Linux_All_DBG.OBJ/js ~/m/y.js
segmentation fault
Comment 1 Igor Bukanov 2008-02-11 09:45:02 PST
For the trunk I will fix the bug via non-recursive implementation of JSOP_FILTER/JSOP_ENDFILTER in the bug 309894. If that patch would look rather complex, then a targeted fix should be created for the branch.
Comment 2 Igor Bukanov 2008-02-15 08:05:27 PST
On the trunk this was fixed as a consequence of addressing the bug 309894.
Comment 3 Igor Bukanov 2008-02-15 08:58:36 PST
Regarding the severity of the bug: it allows for a script to read/write freed heap memory.
Comment 4 Bob Clary [:bc:] 2008-02-22 03:06:40 PST
Created attachment 304943 [details]
js1_7/regress/regress-416705.js
Comment 6 Bob Clary [:bc:] 2008-02-26 08:04:06 PST
v
Comment 7 Daniel Veditz [:dveditz] 2008-02-29 12:16:56 PST
I don't think we want bug 309894 on the branch: the patch is fairly big, it had regressions, and the regression was duped to another bug with a big patch and stil-open regressions.

Is there a subset band-aid that safely fixes this bug, or do we have to wait for the trunk to settle down first and take the big changes? Don't need to block 1.8.1.13 for this, but should get it soonish
Comment 8 Igor Bukanov 2008-02-29 12:52:52 PST
A small patch is definitely feasible. In principle a single call to js_PutBlockObjects should cure this. But even this simple code can regress unless one spends time carefully analyzing the consequences.

So as a solution I suggest to simply disable support for let blocks inside xml filters. Such restriction will not be unknown since on 1.8 branch yield can not be used inside xml filter as well.
Comment 9 Igor Bukanov 2008-02-29 14:23:18 PST
Created attachment 306598 [details] [diff] [review]
fix for 1.8

The fix is match simpler and safer then I have expected.
Comment 10 Igor Bukanov 2008-02-29 14:26:18 PST
Created attachment 306600 [details] [diff] [review]
fix for 1.8 for real

The previous patch contained unrelated changes.
Comment 11 Brendan Eich [:brendan] 2008-02-29 19:29:15 PST
Shouldn't that be JSFRAME_FILTERING, not JSFRAME_GENERATOR?

/be
Comment 12 Igor Bukanov 2008-03-01 01:09:05 PST
(In reply to comment #11)
> Shouldn't that be JSFRAME_FILTERING, not JSFRAME_GENERATOR?

The idea is to call PutBlockObjects unless we are inside a generator. This way  the method will be called for a filtering frame when "mark" is null.
Comment 13 Brendan Eich [:brendan] 2008-03-02 23:57:31 PST
Comment on attachment 306600 [details] [diff] [review]
fix for 1.8 for real

Oh, I get it. Hope this is not too late for 1.8.1.13 -- drivers, please clear whichever request is not needed. Thanks,

/be
Comment 14 Daniel Veditz [:dveditz] 2008-03-03 11:30:42 PST
Comment on attachment 306600 [details] [diff] [review]
fix for 1.8 for real

approved for 1.8.1.13, a=dveditz for release-drivers
Comment 15 Igor Bukanov 2008-03-03 12:18:16 PST
I checked in the patch from the comment 10 to MOZILLA_1_8_BRANCH:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.98; previous revision: 3.181.2.97
done
Comment 16 Bob Clary [:bc:] 2008-03-04 08:30:55 PST
this regressed on 1.8.1 js1_7/extensions/regress-353249.js, js1_7/regress/regress-352797-02.js with Assertion failure: fp, at /work/mozilla/builds/1.8.1/mozilla/js/src/jsobj.c:2004
Comment 17 Igor Bukanov 2008-03-04 12:40:41 PST
I backed out the patch from the comment 15 from MOZILLA_1_8_BRANCH:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.99; previous revision: 3.181.2.98
done
 
Comment 18 Igor Bukanov 2008-03-04 12:49:07 PST
Created attachment 307295 [details] [diff] [review]
very targeted fix for 1.8

The new patch just makes sure that any blocks that js_Interpret creates when invoked from js_FilterXMLList are unput.

The previous patch unputs all the blocks on the current scope chain including ones outside xml filter. This is due to the stop condition in PutBlockObjects that just checks for blocks associated with the current frame. Since the filtering invocation shares the same interpreter JSStackFrame, this caused double-unput.
Comment 19 Brendan Eich [:brendan] 2008-03-04 14:02:49 PST
Comment on attachment 307295 [details] [diff] [review]
very targeted fix for 1.8

Better, safer -- should have caught the last prob, sorry.

/be
Comment 20 Daniel Veditz [:dveditz] 2008-03-05 11:20:19 PST
Comment on attachment 307295 [details] [diff] [review]
very targeted fix for 1.8

approved for 1.8.1.13, a=dveditz for release-drivers
Comment 21 Igor Bukanov 2008-03-05 11:34:41 PST
I checked in the patch from the comment 18 to MOZILLA_1_8_BRANCH:

Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.50.2.69; previous revision: 3.50.2.68
done
Comment 22 Al Billings [:abillings] 2008-03-13 18:07:14 PDT
BC, can you verify this with the rc1 for Firefox 2.0.0.13?
Comment 23 Bob Clary [:bc:] 2008-03-17 04:40:24 PDT
v 1.8.1 linux|macintel for  js1_7/regress/regress-416705.js, no regressions in js1_7/extensions/regress-353249.js, js1_7/regress/regress-352797-02.js on linux|macppc|macintel|winxp.
Comment 24 Alexander Sack 2008-03-25 09:06:15 PDT
js 1.7 doesn't exist on the 1.8.0 branch. not applicable afaict.
Comment 25 Igor Bukanov 2008-03-25 14:30:52 PDT
(In reply to comment #24)
> js 1.7 doesn't exist on the 1.8.0 branch. not applicable afaict.

The bug uses is e4X/js1.6 part of the code so as such there is a strong chance that exists on 1.8.0 branch. Have you tried to run the test cases for it?
Comment 26 Bob Clary [:bc:] 2008-03-29 16:11:31 PDT
/cvsroot/mozilla/js/tests/js1_7/regress/regress-416705.js,v  <--  regress-416705.js
initial revision: 1.1
Comment 27 Daniel Veditz [:dveditz] 2008-04-21 11:26:00 PDT
Clearing 1.8.1.15 blocking request, this one landed in a previous release

Note You need to log in before you can comment on or make changes to this bug.