Last Comment Bug 365692 - getter/setter bytecodes assume number of atoms is less then 64K
: getter/setter bytecodes assume number of atoms is less then 64K
Status: VERIFIED FIXED
[sg:critical]
: verified1.8.0.10, verified1.8.1.2
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Igor Bukanov
:
:
Mentors:
Depends on: 365608
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-02 12:41 PST by Igor Bukanov
Modified: 2007-08-08 07:49 PDT (History)
5 users (show)
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 for 1.8 branches (1.79 KB, patch)
2007-01-09 06:36 PST, Igor Bukanov
brendan: review+
jaymoz: approval1.8.1.2+
jaymoz: approval1.8.0.10+
Details | Diff | Splinter Review
Patch to commit to 1.8.1 branch (2.54 KB, patch)
2007-01-16 14:47 PST, Igor Bukanov
no flags Details | Diff | Splinter Review
Patch to commit to 1.8.0 branch (2.53 KB, patch)
2007-01-16 15:00 PST, Igor Bukanov
no flags Details | Diff | Splinter Review
js1_5/GetSet/regress-365692.js (2.32 KB, text/plain)
2007-01-17 03:42 PST, Bob Clary [:bc:]
no flags Details
js1_5/GetSet/regress-365692.js (2.41 KB, application/javascript)
2007-01-29 09:23 PST, Bob Clary [:bc:]
no flags Details

Description Igor Bukanov 2007-01-02 12:41:25 PST
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] 2007-01-02 12:52:04 PST
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 2007-01-02 13:58:32 PST
(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 2007-01-08 06:21:43 PST
Fixed on trunk as a consequence of a general reorganisation of atom table access in bug 365608.
Comment 4 Jay Patel [:jay] 2007-01-08 15:41:23 PST
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 2007-01-09 06:36:21 PST
Created attachment 250955 [details] [diff] [review]
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] 2007-01-12 12:26:18 PST
brendan:  Can you get this an r= so we can land it on the branches?
Comment 7 Brendan Eich [:brendan] 2007-01-12 14:54:47 PST
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 2007-01-12 15:01:40 PST
(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] 2007-01-16 14:27:40 PST
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 2007-01-16 14:47:42 PST
Created attachment 251692 [details] [diff] [review]
Patch to commit to 1.8.1 branch

In the patch I removed the no longer applicable comments,
Comment 11 Igor Bukanov 2007-01-16 14:49:48 PST
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 2007-01-16 15:00:03 PST
Created attachment 251695 [details] [diff] [review]
Patch to commit to 1.8.0 branch

Patch with no longer applicable comments removed.
Comment 13 Igor Bukanov 2007-01-16 15:02:20 PST
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:] 2007-01-17 03:42:12 PST
Created attachment 251759 [details]
js1_5/GetSet/regress-365692.js
Comment 15 Bob Clary [:bc:] 2007-01-29 09:18:24 PST
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:] 2007-01-29 09:23:04 PST
Created attachment 253191 [details]
js1_5/GetSet/regress-365692.js

updated to ignore the too large exceptions.
Comment 17 Bob Clary [:bc:] 2007-08-08 07:47:28 PDT
Comment on attachment 253191 [details]
js1_5/GetSet/regress-365692.js

I actually placed this in extensions.
Comment 18 Bob Clary [:bc:] 2007-08-08 07:49:28 PDT
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-365692.js,v  <--  regress-365692.js
initial revision: 1.1

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