Closed
Bug 365692
Opened 18 years ago
Closed 18 years ago
getter/setter bytecodes assume number of atoms is less then 64K
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
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)
1.79 KB,
patch
|
brendan
:
review+
jay
:
approval1.8.1.2+
jay
:
approval1.8.0.10+
|
Details | Diff | Splinter Review |
2.54 KB,
patch
|
Details | Diff | Splinter Review | |
2.53 KB,
patch
|
Details | Diff | Splinter Review |
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•18 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
Comment 1•18 years ago
|
||
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•18 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 ;)
Updated•18 years ago
|
Whiteboard: [sg:critical]
Assignee | ||
Comment 3•18 years ago
|
||
Fixed on trunk as a consequence of a general reorganisation of atom table access in bug 365608.
Status: NEW → RESOLVED
Closed: 18 years ago
Depends on: 365608
Flags: blocking1.8.1.2?
Flags: blocking1.8.0.10?
Resolution: --- → FIXED
Comment 4•18 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•18 years ago
|
||
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•18 years ago
|
Attachment #250955 -
Flags: approval1.8.1.2?
Attachment #250955 -
Flags: approval1.8.0.10?
Comment 6•18 years ago
|
||
brendan: Can you get this an r= so we can land it on the branches?
Comment 7•18 years ago
|
||
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•18 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•18 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•18 years ago
|
||
In the patch I removed the no longer applicable comments,
Assignee | ||
Comment 11•18 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•18 years ago
|
||
Patch with no longer applicable comments removed.
Assignee | ||
Comment 13•18 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•18 years ago
|
||
Updated•18 years ago
|
Flags: in-testsuite+
Comment 15•18 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
Comment 16•18 years ago
|
||
updated to ignore the too large exceptions.
Attachment #251759 -
Attachment is obsolete: true
Updated•17 years ago
|
Group: security
Comment 17•17 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•17 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.
Description
•