301596 - js_FilterXMLList leaves fp->spbase NULL due to reuse of frame in nested js_Interpret

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mike Shaver (:shaver emeritus)]

js_FilterXMLList nests a call to js_Interpret on the current frame. 
js_Interpret resets fp->spbase to NULL when it exits, so if you throw after a
filter has run, you will crash trying to work with the stack after we reset it
from spbase.

Testcase:

try {
  <xml/>.(@a == 1);
  throw 5;
} catch (e) { }

One possible fix is to save off spbase in js_FilterXMLList and restore it in the
out:-labelled basic block.  Another would be to make js_Interpret do the saving
and restoring, rather then just NULLing out the frame's spbase.  I think I
prefer the latter as a more systematic fix, but, somewhat oddly, I have a patch
to do the former in my local tree.

Brendan: what's your preference?

Comment 1

[Brendan Eich [:brendan]]

Attachment: fix

Easily fixed.  The fp->spbase setting and clearning in js_Interpret is
necessary to coordinate with the GC when scanning operand stack space owned by
stack frames on each context.  Only js_FilterXMLList reuses the top frame to
interpret a part of a script, so only it needs to save and restore fp->spbase.

/be

Comment 2

[Mike Shaver (:shaver emeritus)]

Comment on attachment 190044 [details] [diff] [review]
fix

I put together pretty much the same patch. r+a=shaver.

Comment 3

[Brendan Eich [:brendan]]

Fixed, thanks!

/be

Comment 4

[Brendan Eich [:brendan]]

Shaver pasted a less-reduced testcase in IRC earlier today, and I wanted to show
my modified version of it:

try {
  var x = <foo><bar a="1"/><bar a="1"/></foo>;
  print(x.bar.(@a == "1").toXMLString());
} catch (e) {
  print("fooey: " + e);
}

The changes were to filter x.bar, not x, and to print toXMLString(), not lenth
(sic) or even length.  Latter change is just for clearer output.  Former is a
necessary change to filter at the right level.  Filtering x.* or even x..* would
work too.

/be

Comment 5

[Axel Hecht]

verified, my CVS build eats dogfood nicely now.

Comment 6

[Brendan Eich [:brendan]]

I just found a fix to this bug's patch, to set spbase early (before the first
goto out) and juggle cosmetics accordingly.  Ugh!  At least I found it before
1.8b4.  I checked it into branch and trunk.

/be

Comment 7

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/e4x/Regress/regress-301596.js,v  <--  regress-301596.js
initial revision: 1.1