The default bug view has changed. See this FAQ.

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

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({verified1.8.0.10, verified1.8.1.2})

Trunk
verified1.8.0.10, verified1.8.1.2
Points:
---
Bug Flags:
blocking1.8.1.2 +
blocking1.8.0.10 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

10 years ago
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.
(Assignee)

Updated

10 years ago
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
(Assignee)

Comment 2

10 years ago
(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]
(Assignee)

Comment 3

10 years ago
Fixed on trunk as a consequence of a general reorganisation of atom table access in bug 365608.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Depends on: 365608
Flags: blocking1.8.1.2?
Flags: blocking1.8.0.10?
Resolution: --- → FIXED

Comment 4

10 years ago
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+
(Assignee)

Comment 5

10 years ago
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.
Attachment #250955 - Flags: review?(brendan)
(Assignee)

Updated

10 years ago
Attachment #250955 - Flags: approval1.8.1.2?
Attachment #250955 - Flags: approval1.8.0.10?

Comment 6

10 years ago
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+
(Assignee)

Comment 8

10 years ago
(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

10 years ago
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+
(Assignee)

Comment 10

10 years ago
Created attachment 251692 [details] [diff] [review]
Patch to commit to 1.8.1 branch

In the patch I removed the no longer applicable comments,
(Assignee)

Comment 11

10 years ago
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
(Assignee)

Comment 12

10 years ago
Created attachment 251695 [details] [diff] [review]
Patch to commit to 1.8.0 branch

Patch with no longer applicable comments removed.
(Assignee)

Comment 13

10 years ago
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

Comment 14

10 years ago
Created attachment 251759 [details]
js1_5/GetSet/regress-365692.js

Updated

10 years ago
Flags: in-testsuite+

Comment 15

10 years ago
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
Keywords: fixed1.8.0.10, fixed1.8.1.2 → verified1.8.0.10, verified1.8.1.2

Comment 16

10 years ago
Created attachment 253191 [details]
js1_5/GetSet/regress-365692.js

updated to ignore the too large exceptions.
Attachment #251759 - Attachment is obsolete: true
Group: security

Comment 17

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

I actually placed this in extensions.
Attachment #253191 - Attachment is obsolete: true

Comment 18

10 years ago
/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.