throw from xml filter leaves pending block objects unput

VERIFIED FIXED

Status

()

defect
P1
critical
VERIFIED FIXED
12 years ago
12 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

({verified1.8.1.13})

Trunk
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
blocking1.8.1.13 -
wanted1.8.1.x +
blocking1.8.0.next -
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:high])

Attachments

(2 attachments, 2 obsolete attachments)

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
Flags: blocking1.9?
Flags: blocking1.8.1.13?
Flags: wanted1.8.1.x+
Flags: blocking1.9?
Flags: blocking1.9+
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.
Priority: -- → P1
On the trunk this was fixed as a consequence of addressing the bug 309894.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Regarding the severity of the bug: it allows for a script to read/write freed heap memory.
Flags: blocking1.8.1.13? → blocking1.8.1.13+
Whiteboard: [sg:high]
Flags: in-testsuite+
Flags: in-litmus-
v
Status: RESOLVED → VERIFIED
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
Depends on: 309894
Flags: blocking1.8.1.14?
Flags: blocking1.8.1.13-
Flags: blocking1.8.1.13+
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.
Posted patch fix for 1.8 (obsolete) — Splinter Review
The fix is match simpler and safer then I have expected.
Attachment #306598 - Flags: review?(brendan)
Posted patch fix for 1.8 for real (obsolete) — Splinter Review
The previous patch contained unrelated changes.
Attachment #306598 - Attachment is obsolete: true
Attachment #306600 - Flags: review?(brendan)
Attachment #306598 - Flags: review?(brendan)
Shouldn't that be JSFRAME_FILTERING, not JSFRAME_GENERATOR?

/be
(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 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
Attachment #306600 - Flags: review?(brendan)
Attachment #306600 - Flags: review+
Attachment #306600 - Flags: approval1.8.1.14?
Attachment #306600 - Flags: approval1.8.1.13?
Comment on attachment 306600 [details] [diff] [review]
fix for 1.8 for real

approved for 1.8.1.13, a=dveditz for release-drivers
Attachment #306600 - Flags: approval1.8.1.14?
Attachment #306600 - Flags: approval1.8.1.13?
Attachment #306600 - Flags: approval1.8.1.13+
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
Keywords: fixed1.8.1.13
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
Depends on: 420880
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
 
Keywords: fixed1.8.1.13
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.
Attachment #306600 - Attachment is obsolete: true
Attachment #307295 - Flags: review?(brendan)
Comment on attachment 307295 [details] [diff] [review]
very targeted fix for 1.8

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

/be
Attachment #307295 - Flags: review?(brendan)
Attachment #307295 - Flags: review+
Attachment #307295 - Flags: approval1.8.1.13?
Comment on attachment 307295 [details] [diff] [review]
very targeted fix for 1.8

approved for 1.8.1.13, a=dveditz for release-drivers
Attachment #307295 - Flags: approval1.8.1.13? → approval1.8.1.13+
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
Keywords: fixed1.8.1.13
BC, can you verify this with the rc1 for Firefox 2.0.0.13?
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.
Flags: blocking1.8.0.15+
js 1.7 doesn't exist on the 1.8.0 branch. not applicable afaict.
Flags: blocking1.8.0.15+ → blocking1.8.0.15-
(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?
Group: security
/cvsroot/mozilla/js/tests/js1_7/regress/regress-416705.js,v  <--  regress-416705.js
initial revision: 1.1
Clearing 1.8.1.15 blocking request, this one landed in a previous release
Flags: blocking1.8.1.15?
You need to log in before you can comment on or make changes to this bug.