throw from xml filter leaves pending block objects unput

VERIFIED FIXED

Status

()

Core
JavaScript Engine
P1
critical
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({verified1.8.1.13})

Trunk
x86
Linux
verified1.8.1.13
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)

(Assignee)

Description

10 years ago
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?

Updated

10 years ago
Flags: wanted1.8.1.x+
Flags: blocking1.9?
Flags: blocking1.9+
(Assignee)

Comment 1

10 years ago
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.

Updated

10 years ago
Priority: -- → P1
(Assignee)

Comment 2

10 years ago
On the trunk this was fixed as a consequence of addressing the bug 309894.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 3

10 years ago
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]

Comment 4

10 years ago
Created attachment 304943 [details]
js1_7/regress/regress-416705.js

Updated

10 years ago
Flags: in-testsuite+
Flags: in-litmus-

Comment 6

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

Comment 8

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

Comment 9

10 years ago
Created attachment 306598 [details] [diff] [review]
fix for 1.8

The fix is match simpler and safer then I have expected.
Attachment #306598 - Flags: review?(brendan)
(Assignee)

Comment 10

10 years ago
Created attachment 306600 [details] [diff] [review]
fix for 1.8 for real

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

Comment 12

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

Comment 15

9 years ago
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

Comment 16

9 years ago
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

Updated

9 years ago
Depends on: 420880
(Assignee)

Comment 17

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

Comment 18

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

Comment 21

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

Updated

9 years ago
Keywords: fixed1.8.1.13
BC, can you verify this with the rc1 for Firefox 2.0.0.13?

Comment 23

9 years ago
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.
Keywords: fixed1.8.1.13 → verified1.8.1.13

Updated

9 years ago
Flags: blocking1.8.0.15+

Comment 24

9 years ago
js 1.7 doesn't exist on the 1.8.0 branch. not applicable afaict.
Flags: blocking1.8.0.15+ → blocking1.8.0.15-
(Assignee)

Comment 25

9 years ago
(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

Comment 26

9 years ago
/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.