Last Comment Bug 367561 - JSOP_(GET|SET)METHOD and JSOP_SETCONST assume atoms indexes < 64K
: JSOP_(GET|SET)METHOD and JSOP_SETCONST assume atoms indexes < 64K
Status: VERIFIED FIXED
[sg:critical]
: verified1.8.0.12, verified1.8.1.4
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 1.8 Branch
: All All
: -- critical (vote)
: ---
Assigned To: Igor Bukanov
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-19 17:38 PST by Igor Bukanov
Modified: 2007-06-14 15:20 PDT (History)
6 users (show)
jaymoz: blocking1.8.1.2-
dveditz: blocking1.8.1.4+
brendan: wanted1.8.1.x+
jaymoz: blocking1.8.0.10-
dveditz: blocking1.8.0.12+
brendan: wanted1.8.0.x+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Simple targeted fix for 1.8.1 branch (2.03 KB, patch)
2007-02-13 15:59 PST, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review
Branch fix v2 (2.05 KB, patch)
2007-04-04 18:27 PDT, Igor Bukanov
brendan: review+
dveditz: approval1.8.1.4+
dveditz: approval1.8.0.12+
Details | Diff | Splinter Review
e4x/Regress/regress-367561-02.js (2.21 KB, text/plain)
2007-04-08 02:36 PDT, Bob Clary [:bc:]
no flags Details
js1_5/Regress/regress-367561-01.js (2.54 KB, text/plain)
2007-04-08 02:37 PDT, Bob Clary [:bc:]
no flags Details
js1_5/Regress/regress-367561-03.js (2.51 KB, text/plain)
2007-04-08 02:38 PDT, Bob Clary [:bc:]
no flags Details

Description Igor Bukanov 2007-01-19 17:38:04 PST
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 Brendan Eich [:brendan] 2007-01-20 11:55:53 PST
Igor, what do you think of taking the generic fix in bug 365608 on the branches?

/be
Comment 2 Igor Bukanov 2007-01-20 18:12:03 PST
(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 Jay Patel [:jay] 2007-01-22 14:54:52 PST
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.
Comment 4 Igor Bukanov 2007-01-30 15:05:14 PST
(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.

Comment 5 Brendan Eich [:brendan] 2007-01-30 15:35:25 PST
Strongly recommend to just take the patch.  It's less scary than doing custom ad-hoc alternative fixes for the branch.

/be
Comment 6 Igor Bukanov 2007-02-13 15:59:13 PST
Created attachment 255023 [details] [diff] [review]
Simple targeted fix for 1.8.1 branch

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.
Comment 7 Brendan Eich [:brendan] 2007-02-13 16:30:58 PST
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
Comment 8 chris hofmann 2007-03-08 11:08:12 PST
what is the next step on this one?
Comment 9 Igor Bukanov 2007-03-08 17:26:45 PST
(In reply to comment #8)
> what is the next step on this one?
> 

To get an approval for the patch.
Comment 10 Daniel Veditz [:dveditz] 2007-03-21 15:40:04 PDT
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
Comment 11 Igor Bukanov 2007-04-04 17:34:16 PDT
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
Comment 12 Igor Bukanov 2007-04-04 17:38:12 PDT
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
Comment 13 Igor Bukanov 2007-04-04 18:21:49 PDT
I removed both patches to fix the older GCC regression.
Comment 14 Igor Bukanov 2007-04-04 18:27:15 PDT
Created attachment 260660 [details] [diff] [review]
Branch fix v2

This version replaces #ifdef inside the JS_ASSERT by a duplicated assert code to support older versions of GCC.
Comment 15 Daniel Veditz [:dveditz] 2007-04-06 12:01:06 PDT
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
Comment 16 Igor Bukanov 2007-04-06 13:33:37 PDT
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
Comment 17 Igor Bukanov 2007-04-06 13:39:24 PDT
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
Comment 18 Bob Clary [:bc:] 2007-04-08 02:36:28 PDT
Created attachment 260944 [details]
e4x/Regress/regress-367561-02.js
Comment 19 Bob Clary [:bc:] 2007-04-08 02:37:20 PDT
Created attachment 260945 [details]
js1_5/Regress/regress-367561-01.js
Comment 20 Bob Clary [:bc:] 2007-04-08 02:38:00 PDT
Created attachment 260946 [details]
js1_5/Regress/regress-367561-03.js
Comment 21 Bob Clary [:bc:] 2007-04-11 13:57:20 PDT
verified fixed 1.8.0, 1.8.1, 1.9.0 browser, shell 2007-04-10
Comment 22 Bob Clary [:bc:] 2007-06-14 15:20:18 PDT
/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

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