Last Comment Bug 366123 - compiling long XML filtering predicate hangs
: compiling long XML filtering predicate hangs
Status: VERIFIED FIXED
[sg:critical?]
: hang, verified1.8.0.10, verified1.8.1.2
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 1.8 Branch
: All All
: P1 critical (vote)
: ---
Assigned To: Igor Bukanov
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-06 01:19 PST by shutdown
Modified: 2008-02-11 09:59 PST (History)
6 users (show)
dveditz: blocking1.9?
jaymoz: blocking1.8.1.2+
jaymoz: blocking1.8.0.10+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix v1 (3.84 KB, patch)
2007-01-06 05:24 PST, Igor Bukanov
no flags Details | Diff | Splinter Review
simpler alterna-patch (1.47 KB, patch)
2007-01-06 15:43 PST, Brendan Eich [:brendan]
igor: review+
jaymoz: approval1.8.1.2+
jaymoz: approval1.8.0.10+
Details | Diff | Splinter Review
e4x/Expressions/regress-366123.js (2.14 KB, text/plain)
2007-01-17 04:03 PST, Bob Clary [:bc:]
no flags Details

Description shutdown 2007-01-06 01:19:07 PST
$ cat 020-long-filtering-predicate.txt
function exploit() {
  var code = "foo = <x/>.(", obj = {};
  for(var i = 0; i < 0x10000; i++) {
    code += "0, ";
  }
  code += "0);";
  Function(code);
}
exploit();

$ dbg.obj/js 020-long-filtering-predicate.txt
Assertion failure: 0, at jsemit.c:808

$ opt.obj/js 020-long-filtering-predicate.txt
...(never returns)
Comment 1 Igor Bukanov 2007-01-06 04:48:10 PST
The bug is caused by the lack of JSOP_FILTERX. Making as security-sensitive as it is not clear if an exploit is not possible.
Comment 2 Igor Bukanov 2007-01-06 05:24:13 PST
Created attachment 250693 [details] [diff] [review]
Fix v1

The fix adds JSOP_FILTERX and adjusts code accordingly. There is no need to change XDR version as the patch does not change the meaning of the current bytecode.
Comment 3 Igor Bukanov 2007-01-06 14:12:12 PST
This fix conflicts in jsopcode.tbl with the patch for bug 365608. Since the fix is much smaller and easier to commit to branches, I suggest to commit it to the trunk first.
Comment 4 Brendan Eich [:brendan] 2007-01-06 15:43:42 PST
Created attachment 250718 [details] [diff] [review]
simpler alterna-patch

My fault again, I'll take this to let Igor get on with more important work.  This is a simpler patch that suffices for 1.8 and 1.8.0 branches, no new opcode needed. An absurdly long XML filtering predicate expression is not something we need to support, and an error beats an exploitable crash any day.

/be
Comment 5 Igor Bukanov 2007-01-06 16:35:36 PST
Comment on attachment 250718 [details] [diff] [review]
simpler alterna-patch

Minusing for now: I saw in the debugger an arbitrary op value.
Comment 6 Igor Bukanov 2007-01-06 16:39:51 PST
Comment on attachment 250718 [details] [diff] [review]
simpler alterna-patch

+ again after false alarm, that op=garbage was with partially modified my patch.
Comment 7 Jay Patel [:jay] 2007-01-08 15:42:34 PST
Comment on attachment 250718 [details] [diff] [review]
simpler alterna-patch

Approved for both branches, a=jay for drivers.
Comment 8 Brendan Eich [:brendan] 2007-01-09 16:01:33 PST
Fixed on the branches:

1.8.0: js/src/jsemit.c rev 3.128.2.3.2.13
1.8:   js/src/jsemit.c rev 3.128.2.62

/be
Comment 9 Bob Clary [:bc:] 2007-01-17 04:03:12 PST
Created attachment 251762 [details]
e4x/Expressions/regress-366123.js
Comment 10 Bob Clary [:bc:] 2007-01-29 09:03:23 PST
verified fixed 1.8.0, 1.8.1 2007-01-23 win/mac*/linux but I show this failing on trunk and I can't tell from the comments or lxr that it was checked into the trunk.
Comment 11 Igor Bukanov 2007-01-30 15:37:39 PST
(In reply to comment #10)
> verified fixed 1.8.0, 1.8.1 2007-01-23 win/mac*/linux but I show this failing
> on trunk and I can't tell from the comments or lxr that it was checked into the
> trunk.

Of cause the patch for bug 365608 does not fix this. This bug is related to the lack of long-jump support for xml filtering code while bug fixed once and for all the problem of big atom indexes. 

Comment 12 Igor Bukanov 2007-01-30 15:48:04 PST
For the real fix should we just add JSOP_FILTERX? Or perhaps try some generic approach like one for atom indexes? 
Comment 13 Brendan Eich [:brendan] 2007-01-30 15:54:48 PST
The span-dependent extended jump selection is generic, modulo different formats. We don't want a prefix and suffix op just for JSOP_FILTER, and we do not want to re-do and add overhead to the extended jump scheme.

See bug 309894 and please feel free to take it and dup this bug against it.

/be
Comment 14 Igor Bukanov 2007-01-30 16:07:42 PST
(In reply to comment #13)
> See bug 309894 and please feel free to take it and dup this bug against it.

For now I just record that bug as a blocker for this one: with endfilter we will need endfilterx as well.  
Comment 15 Daniel Veditz [:dveditz] 2007-02-28 13:27:24 PST
Didn't notice that this was not fixed on trunk.
Comment 16 chris hofmann 2007-03-08 11:12:02 PST
what's needed to get this on the trunk and close out as fixed?
Comment 17 Igor Bukanov 2007-03-10 13:07:05 PST
(In reply to comment #16)
> what's needed to get this on the trunk and close out as fixed?

I think the best is to land the patch already applied to the trunk. The real fix should be done as a part of bug 309894.
Comment 18 Igor Bukanov 2007-03-19 09:35:38 PDT
I am going to commit the patch from comment 4 to close the bug.
Comment 19 Igor Bukanov 2007-03-19 14:48:02 PDT
I committed Brendan's patch from comment 4 to the trunk:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.241; previous revision: 3.240
done
Comment 20 Bob Clary [:bc:] 2007-03-21 15:42:54 PDT
verified fixed 1.9.0 20070320 win/mac*/linux
Comment 21 Bob Clary [:bc:] 2007-05-06 01:34:39 PDT
/cvsroot/mozilla/js/tests/e4x/Expressions/regress-366123.js,v  <--  regress-366123.js
initial revision: 1.1
Comment 22 Igor Bukanov 2008-02-11 09:59:25 PST
The bug should not depend on 309894 as 309894 is not a regression.

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