Closed Bug 366123 Opened 13 years ago Closed 13 years ago

compiling long XML filtering predicate hangs

Categories

(Core :: JavaScript Engine, defect, P1, critical)

1.8 Branch
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: sync2d, Assigned: igor)

Details

(Keywords: hang, verified1.8.0.10, verified1.8.1.2, Whiteboard: [sg:critical?])

Attachments

(2 files, 1 obsolete file)

$ 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)
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.
Assignee: general → igor.bukanov
Group: security
Attached patch Fix v1 (obsolete) — Splinter Review
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.
Attachment #250693 - Flags: review?(brendan)
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.
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
Assignee: igor.bukanov → brendan
Attachment #250693 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #250718 - Flags: review?(igor.bukanov)
Attachment #250718 - Flags: approval1.8.1.2?
Attachment #250718 - Flags: approval1.8.0.10?
Attachment #250693 - Flags: review?(brendan)
Flags: blocking1.8.1.2?
Flags: blocking1.8.0.10?
Priority: -- → P1
Version: Trunk → 1.8 Branch
Attachment #250718 - Flags: review?(igor.bukanov) → review+
Comment on attachment 250718 [details] [diff] [review]
simpler alterna-patch

Minusing for now: I saw in the debugger an arbitrary op value.
Attachment #250718 - Flags: review+ → review-
Comment on attachment 250718 [details] [diff] [review]
simpler alterna-patch

+ again after false alarm, that op=garbage was with partially modified my patch.
Attachment #250718 - Flags: review- → review+
Flags: blocking1.8.1.2?
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10?
Flags: blocking1.8.0.10+
Comment on attachment 250718 [details] [diff] [review]
simpler alterna-patch

Approved for both branches, a=jay for drivers.
Attachment #250718 - Flags: approval1.8.1.2?
Attachment #250718 - Flags: approval1.8.1.2+
Attachment #250718 - Flags: approval1.8.0.10?
Attachment #250718 - Flags: approval1.8.0.10+
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Whiteboard: [sg:critical?]
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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. 

For the real fix should we just add JSOP_FILTERX? Or perhaps try some generic approach like one for atom indexes? 
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
(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.  
Depends on: 309894
Assignee: brendan → igor
Status: REOPENED → NEW
Flags: blocking1.9?
Group: security
Didn't notice that this was not fixed on trunk.
Group: security
what's needed to get this on the trunk and close out as fixed?
(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.
I am going to commit the patch from comment 4 to close the bug.
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
Status: NEW → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
verified fixed 1.9.0 20070320 win/mac*/linux
Status: RESOLVED → VERIFIED
Group: security
/cvsroot/mozilla/js/tests/e4x/Expressions/regress-366123.js,v  <--  regress-366123.js
initial revision: 1.1
The bug should not depend on 309894 as 309894 is not a regression.
No longer depends on: 309894
You need to log in before you can comment on or make changes to this bug.