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)

1.8 Branch
defect
Not set
critical

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)

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.
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?
(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.
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-
(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: general → igor
Strongly recommend to just take the patch.  It's less scary than doing custom ad-hoc alternative fixes for the branch.

/be
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 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?
Whiteboard: [sg:critical]
what is the next step on this one?
(In reply to comment #8)
> what is the next step on this one?
> 

To get an approval for the patch.
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12+
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+
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
Status: NEW → RESOLVED
Closed: 17 years ago
Keywords: fixed1.8.1.4
Resolution: --- → FIXED
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
I removed both patches to fix the older GCC regression.
Attached patch Branch fix v2Splinter Review
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?
Attachment #255023 - Flags: approval1.8.1.4+
Attachment #255023 - Flags: approval1.8.0.12+
Attachment #260660 - Flags: review?(brendan) → review+
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+
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
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
Flags: in-testsuite+
verified fixed 1.8.0, 1.8.1, 1.9.0 browser, shell 2007-04-10
Status: RESOLVED → VERIFIED
Group: security
/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.