416705 - throw from xml filter leaves pending block objects unput

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Igor Bukanov]

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]

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]

On the trunk this was fixed as a consequence of addressing the bug 309894.

Comment 3

[Igor Bukanov]

Regarding the severity of the bug: it allows for a script to read/write freed heap memory.

Comment 4

[Bob Clary [:bc] (inactive)]

Attachment: js1_7/regress/regress-416705.js

Comment 6

[Bob Clary [:bc] (inactive)]

v

Comment 7

[Daniel Veditz [:dveditz]]

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]

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]

Attachment: fix for 1.8

The fix is match simpler and safer then I have expected.

Comment 10

[Igor Bukanov]

Attachment: fix for 1.8 for real

The previous patch contained unrelated changes.

Comment 11

[Brendan Eich [:brendan]]

Shouldn't that be JSFRAME_FILTERING, not JSFRAME_GENERATOR?

/be

Comment 12

[Igor Bukanov]

(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]]

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]]

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]

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] (inactive)]

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]

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]

Attachment: 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]]

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]]

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]

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 - ex-MoCo]]

BC, can you verify this with the rc1 for Firefox 2.0.0.13?

Comment 23

[Bob Clary [:bc] (inactive)]

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]

js 1.7 doesn't exist on the 1.8.0 branch. not applicable afaict.

Comment 25

[Igor Bukanov]

(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] (inactive)]

/cvsroot/mozilla/js/tests/js1_7/regress/regress-416705.js,v  <--  regress-416705.js
initial revision: 1.1

Comment 27

[Daniel Veditz [:dveditz]]

Clearing 1.8.1.15 blocking request, this one landed in a previous release