366123 - compiling long XML filtering predicate hangs

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[shutdown]

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

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]

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

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

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

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]

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

Comment on attachment 250718 [details] [diff] [review]
simpler alterna-patch

Approved for both branches, a=jay for drivers.

Comment 8

[Brendan Eich [:brendan]]

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

Attachment: e4x/Expressions/regress-366123.js

Comment 10

[Bob Clary [:bc] (inactive)]

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]

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

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

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]

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

Didn't notice that this was not fixed on trunk.

Comment 16

[chris hofmann]

what's needed to get this on the trunk and close out as fixed?

Comment 17

[Igor Bukanov]

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

I am going to commit the patch from comment 4 to close the bug.

Comment 19

[Igor Bukanov]

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

verified fixed 1.9.0 20070320 win/mac*/linux

Comment 21

[Bob Clary [:bc] (inactive)]

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

Comment 22

[Igor Bukanov]

The bug should not depend on 309894 as 309894 is not a regression.