Closed
Bug 367561
Opened 17 years ago
Closed 17 years ago
JSOP_(GET|SET)METHOD and JSOP_SETCONST assume atoms indexes < 64K
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: igor, Assigned: igor)
Details
(Keywords: verified1.8.0.12, verified1.8.1.4, Whiteboard: [sg:critical])
Attachments
(4 files, 1 obsolete file)
2.05 KB,
patch
|
brendan
:
review+
dveditz
:
approval1.8.1.4+
dveditz
:
approval1.8.0.12+
|
Details | Diff | Splinter Review |
2.21 KB,
text/plain
|
Details | |
2.54 KB,
text/plain
|
Details | |
2.51 KB,
text/plain
|
Details |
This is a spin-off of the bug 366122 comment 10. JSOP_(GET|SET)METHOD and JSOP_SETCONST implementation on 1.8.* branches assume that their atom index is bellow 64K. The following example demonstrate the problem with js shell: ~/m/1.8.1/mozilla/js/src $ cat ~/m/files/y1.js var N = 1 << 16; var src = 'var x = /'; var array = Array(); for (var i = 0; i != N/2; ++i) array[i] = i; src += array.join('/;x=/')+'/; x="'; src += array.join('";x="')+'";'; src += 'y.some_function();'; var f = Function(src); var src2 = f.toString(); ~/m/1.8.1/mozilla/js/src $ ./Linux_All_OPT.OBJ/js ~/m/files/y1.js segmentation fault ~/m/1.8.1/mozilla/js/src $ cat ~/m/files/y2.js var N = 1 << 16; var src = 'var x = /'; var array = Array(); for (var i = 0; i != N/2; ++i) array[i] = i; src += array.join('/;x=/')+'/; x="'; src += array.join('";x="')+'";'; src += 'y.function::some_function = 1;'; var f = Function(src); var src2 = f.toString(); ~/m/1.8.1/mozilla/js/src $ ./Linux_All_OPT.OBJ/js ~/m/files/y2.js segmentation fault ~/m/1.8.1/mozilla/js/src $ cat ~/m/files/y3.js var N = 1 << 16; var src = 'var x = /'; var array = Array(); for (var i = 0; i != N/2; ++i) array[i] = i; src += array.join('/;x=/')+'/; x="'; src += array.join('";x="')+'";'; src += 'const some_const = 10'; eval(src); ~/m/1.8.1/mozilla/js/src $ ./Linux_All_OPT.OBJ/js ~/m/files/y3.js segmentation fault The bug does not exist on the trunk due to the generic fix in bug 365608.
Comment 1•17 years ago
|
||
Igor, what do you think of taking the generic fix in bug 365608 on the branches? /be
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.2?
Flags: blocking1.8.0.10?
Assignee | ||
Comment 2•17 years ago
|
||
(In reply to comment #1) > Igor, what do you think of taking the generic fix in bug 365608 on the > branches? This is the safest option. Any workaround for [getmethod] problem requires non-trivial code and as such it is better to backport the generic fix.
Comment 3•17 years ago
|
||
The patch in bug 365608 looks a bit scary for the branches, but if there is any way we can fix this with a simpler patch, we can consider it, but not blocking for it.
Flags: blocking1.8.1.2?
Flags: blocking1.8.1.2-
Flags: blocking1.8.0.10?
Flags: blocking1.8.0.10-
Assignee | ||
Comment 4•17 years ago
|
||
(In reply to comment #3) > The patch in bug 365608 looks a bit scary for the branches, but if there is any > way we can fix this with a simpler patch, we can consider it, but not blocking > for it. The problem with any workaround is that it requires non-trivial code in any case. Surely, the targeted fix will be smaller, but it will be non-trivial change in any case. If application of the well-tested fix from bug 365608 to the branches is no go, then let me know and I tried to make a minimal patch.
Assignee | ||
Updated•17 years ago
|
Assignee: general → igor
Comment 5•17 years ago
|
||
Strongly recommend to just take the patch. It's less scary than doing custom ad-hoc alternative fixes for the branch. /be
Assignee | ||
Comment 6•17 years ago
|
||
Now I feel stupid since the targeted fix is so simple. For some reasons I assumed that LITOPX can not be used with JOF_NAME|JOF_PROP bytecode. But this is not the case. In fact the interpreter and decompiler already got proper support for huge atoms with (GET|SET)METHOD and SETCONST and the bug was really with EmitAtomIndexOp when it selected wrong literal prefix. On the other hand all those function:: bugs surfaced during development of various workarounds.
Attachment #255023 -
Flags: review?(brendan)
Comment 7•17 years ago
|
||
Comment on attachment 255023 [details] [diff] [review] Simple targeted fix for 1.8.1 branch Why didn't I see that? When I hacked LITOPX etc. in, I was convinced the modes distinguished the cases. Not so, as this patch shows. Glad it came to you in time for the branch. /be
Attachment #255023 -
Flags: review?(brendan)
Attachment #255023 -
Flags: review+
Attachment #255023 -
Flags: approval1.8.1.3?
Attachment #255023 -
Flags: approval1.8.0.11?
Updated•17 years ago
|
Whiteboard: [sg:critical]
Comment 8•17 years ago
|
||
what is the next step on this one?
Assignee | ||
Comment 9•17 years ago
|
||
(In reply to comment #8) > what is the next step on this one? > To get an approval for the patch.
Updated•17 years ago
|
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
Updated•17 years ago
|
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12+
Comment 10•17 years ago
|
||
Comment on attachment 255023 [details] [diff] [review] Simple targeted fix for 1.8.1 branch approved for 1.8.0.12 and 1.8.1.4, a=dveditz for release-drivers
Attachment #255023 -
Flags: approval1.8.1.4?
Attachment #255023 -
Flags: approval1.8.1.4+
Attachment #255023 -
Flags: approval1.8.0.12?
Attachment #255023 -
Flags: approval1.8.0.12+
Assignee | ||
Comment 11•17 years ago
|
||
I committed the patch from comment 6 to MOZILLA_1_8_BRANCH: Checking in jsemit.c; /cvsroot/mozilla/js/src/jsemit.c,v <-- jsemit.c new revision: 3.128.2.68; previous revision: 3.128.2.67 done
Assignee | ||
Comment 12•17 years ago
|
||
I committed the patch from comment 6 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.18; previous revision: 3.128.2.3.2.17 done
Keywords: fixed1.8.0.12
Assignee | ||
Comment 13•17 years ago
|
||
I removed both patches to fix the older GCC regression.
Keywords: fixed1.8.0.12,
fixed1.8.1.4
Assignee | ||
Comment 14•17 years ago
|
||
This version replaces #ifdef inside the JS_ASSERT by a duplicated assert code to support older versions of GCC.
Attachment #255023 -
Attachment is obsolete: true
Attachment #260660 -
Flags: review?(brendan)
Attachment #260660 -
Flags: approval1.8.1.4?
Attachment #260660 -
Flags: approval1.8.0.12?
Assignee | ||
Updated•17 years ago
|
Attachment #255023 -
Flags: approval1.8.1.4+
Attachment #255023 -
Flags: approval1.8.0.12+
Updated•17 years ago
|
Attachment #260660 -
Flags: review?(brendan) → review+
Comment 15•17 years ago
|
||
Comment on attachment 260660 [details] [diff] [review] Branch fix v2 approved for 1.8.0.12 and 1.8.1.4, a=dveditz for release-drivers
Attachment #260660 -
Flags: approval1.8.1.4?
Attachment #260660 -
Flags: approval1.8.1.4+
Attachment #260660 -
Flags: approval1.8.0.12?
Attachment #260660 -
Flags: approval1.8.0.12+
Assignee | ||
Comment 16•17 years ago
|
||
I committed the patch from comment 14 to MOZILLA_1_8_BRANCH: Checking in jsemit.c; /cvsroot/mozilla/js/src/jsemit.c,v <-- jsemit.c new revision: 3.128.2.70; previous revision: 3.128.2.69 done
Keywords: fixed1.8.1.4
Assignee | ||
Comment 17•17 years ago
|
||
I committed the patch from comment 14 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.20; previous revision: 3.128.2.3.2.19 done
Keywords: fixed1.8.0.12
Comment 18•17 years ago
|
||
Comment 19•17 years ago
|
||
Comment 20•17 years ago
|
||
Updated•17 years ago
|
Flags: in-testsuite+
Comment 21•17 years ago
|
||
verified fixed 1.8.0, 1.8.1, 1.9.0 browser, shell 2007-04-10
Status: RESOLVED → VERIFIED
Updated•17 years ago
|
Group: security
Comment 22•17 years ago
|
||
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-367561-01.js,v <-- regress-367561-01.js initial revision: 1.1 /cvsroot/mozilla/js/tests/js1_5/Regress/regress-367561-03.js,v <-- regress-367561-03.js initial revision: 1.1
You need to log in
before you can comment on or make changes to this bug.
Description
•