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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

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.

Comment 1

[Brendan Eich [:brendan]]

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

Comment 2

[Igor Bukanov]

(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 ;)

Comment 3

[Igor Bukanov]

Fixed on trunk as a consequence of a general reorganisation of atom table access in bug 365608.

Comment 4

[Jay Patel [:jay]]

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?

Comment 5

[Igor Bukanov]

Attachment: Fix v1 for 1.8 branches

A minimal patch to report an error when [getter] and [setter] prefixes atom operation bytecode that exceeds 64K limit.

Comment 6

[Jay Patel [:jay]]

brendan:  Can you get this an r= so we can land it on the branches?

Comment 7

[Brendan Eich [:brendan]]

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

Comment 8

[Igor Bukanov]

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

[Jay Patel [:jay]]

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!

Comment 10

[Igor Bukanov]

Attachment: Patch to commit to 1.8.1 branch

In the patch I removed the no longer applicable comments,

Comment 11

[Igor Bukanov]

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

Comment 12

[Igor Bukanov]

Attachment: Patch to commit to 1.8.0 branch

Patch with no longer applicable comments removed.

Comment 13

[Igor Bukanov]

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

Comment 14

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/GetSet/regress-365692.js

Comment 15

[Bob Clary [:bc] (inactive)]

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.

Comment 16

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/GetSet/regress-365692.js

updated to ignore the too large exceptions.

Comment 17

[Bob Clary [:bc] (inactive)]

Comment on attachment 253191 [details]
js1_5/GetSet/regress-365692.js

I actually placed this in extensions.

Comment 18

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-365692.js,v  <--  regress-365692.js
initial revision: 1.1