Closed Bug 365692 Opened 13 years ago Closed 13 years ago

getter/setter bytecodes assume number of atoms is less then 64K

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

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

Attachments

(3 files, 2 obsolete files)

The implementation of JSOP_GETTER/JSOP_SETTER prefix bytecodes assumes that an atom index for the following bytecode is less then 1 << 16 and ignores JSOP_LITERAL and friends bytecodes. 

In particular the interpreter loop asserts on such bytecodes as the following example demonstrates:

~/m/files> cat ~/m/files/y1.js 
function g()
{
  return 10;
}

var N = 100*1000;
var src = 'var x = ["';
var array = Array(N);
for (var i = 0; i != N; ++i)
  array[i] = i;
src += array.join('","')+'"]; x.a getter = g; return x.a;';
var f = Function(src);
if (f() != 10)
  throw "Unexpected result";

~/m/files> ~/m/trunk/mozilla/js/src/Linux_All_DBG.OBJ/js ~/m/files/y1.js
Assertion failure: 0, at jsinterp.c:5347
Trace/breakpoint trap (core dumped)


The example builds a source with over 64K literal strings and adds x.a getter = g to it. Then generates the following bytecode:

803408:  getter
803409:  literal 100001
803413:  setelem

which is not handled by the current implementation.

This is exploitable as in the optimized build the code would access the junk values in various registers.
Summary: getter/setter bytecode prefixes assumes that atom index are less then 64K → getter/setter bytecodes assume number of atoms is less then 64K
Do you want to fix this by itself or as part of the fix for bug 365692?  I'd be happy to see the latter happen, since here is proof that the sunk cost is not all good, or at least not cost-free. I would be willing to take bug 365692 if you want to give it to me. Thanks,

/be
(In reply to comment #1)
> Do you want to fix this by itself or as part of the fix for bug 365692?

For a branch-safe fix I suggest to simply report an error when atomIndex for getter/setter bytecode exceeds 64K. There is no compatibility issues here as the code never worked in any case.

> I'd be
> happy to see the latter happen, since here is proof that the sunk cost is not
> all good, or at least not cost-free. I would be willing to take bug 365692 if
> you want to give it to me. Thanks,

I will attach an updated patch (which still crashed as apparently the transformations were not error proof :( ), where there is js_atombase1..3 so the patch increases the bytecode size only when there are more then 256K of literals. After that its yours ;)


Whiteboard: [sg:critical]
Fixed on trunk as a consequence of a general reorganisation of atom table access in bug 365608.
Status: NEW → RESOLVED
Closed: 13 years ago
Depends on: 365608
Flags: blocking1.8.1.2?
Flags: blocking1.8.0.10?
Resolution: --- → FIXED
Igor:  The patch in bug 365608 looks a bit large and risky, any chance we can come up with a simpler/safer patch for the branches?
Flags: blocking1.8.1.2?
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10?
Flags: blocking1.8.0.10+
A minimal patch to report an error when [getter] and [setter] prefixes atom operation bytecode that exceeds 64K limit.
Attachment #250955 - Flags: review?(brendan)
Attachment #250955 - Flags: approval1.8.1.2?
Attachment #250955 - Flags: approval1.8.0.10?
brendan:  Can you get this an r= so we can land it on the branches?
Comment on attachment 250955 [details] [diff] [review]
Fix v1 for 1.8 branches

> #if JS_HAS_GETTER_SETTER
>         if (op == JSOP_GETTER || op == JSOP_SETTER) {
>             /* We'll emit these prefix bytecodes after emitting the r.h.s. */
>+            if (atomIndex != (jsatomid) -1 && atomIndex >= JS_BIT(16)) {

Since this is now counting on the atomIndex = (jsatomid) -1; statement that dominates all cases under the TOK_ASSIGN label, please remove the "/* quell GCC overwarning */" comment attached to the initializing statement.

/be
Attachment #250955 - Flags: review?(brendan) → review+
(In reply to comment #7)
> Since this is now counting on the atomIndex = (jsatomid) -1; statement that
> dominates all cases under the TOK_ASSIGN label, please remove the "/* quell GCC
> overwarning */" comment attached to the initializing statement.

I did it initially, but then the patch would not apply as-is to 1.8.0 as that comment has a different text there. Plus the comment is still applicable when !HAS_GETTER_SETTER. But I must admit that is a not a good excuse. So I will remove the comment on branches.
Comment on attachment 250955 [details] [diff] [review]
Fix v1 for 1.8 branches

Approved for both branches,  a=jay for drivers.  Please land asap.  Thanks!
Attachment #250955 - Flags: approval1.8.1.2?
Attachment #250955 - Flags: approval1.8.1.2+
Attachment #250955 - Flags: approval1.8.0.10?
Attachment #250955 - Flags: approval1.8.0.10+
In the patch I removed the no longer applicable comments,
I committed the patch from comment 10 to MOZILLA_1_8_BRANCH:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.128.2.65; previous revision: 3.128.2.64
done
Keywords: fixed1.8.1.2
Patch with no longer applicable comments removed.
I committed the patch from comment 12 to MOZILLA_1_8_0_BRANCH:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.128.2.3.2.16; previous revision: 3.128.2.3.2.15
done
Keywords: fixed1.8.0.10
Attached file js1_5/GetSet/regress-365692.js (obsolete) —
Flags: in-testsuite+
verified fixed 1.8.0, 1.8.1, 1.9.0 2007-01-23 win/mac*/linux
no crash, 1.8.0->Internal Error script too large, 1.8.1->Internal Error block too large.

Status: RESOLVED → VERIFIED
Attached file js1_5/GetSet/regress-365692.js (obsolete) —
updated to ignore the too large exceptions.
Attachment #251759 - Attachment is obsolete: true
Group: security
Comment on attachment 253191 [details]
js1_5/GetSet/regress-365692.js

I actually placed this in extensions.
Attachment #253191 - Attachment is obsolete: true
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-365692.js,v  <--  regress-365692.js
initial revision: 1.1
You need to log in before you can comment on or make changes to this bug.