Note: There are a few cases of duplicates in user autocompletion which are being worked on.

JSOP_(GET|SET)METHOD and JSOP_SETCONST assume atoms indexes < 64K

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({verified1.8.0.12, verified1.8.1.4})

1.8 Branch
verified1.8.0.12, verified1.8.1.4
Points:
---
Bug Flags:
blocking1.8.1.2 -
blocking1.8.1.4 +
wanted1.8.1.x +
blocking1.8.0.10 -
blocking1.8.0.12 +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Comment 2

11 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

11 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

11 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

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

Comment 6

11 years ago
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.
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]

Comment 8

11 years ago
what is the next step on this one?
(Assignee)

Comment 9

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

Comment 11

10 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
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Keywords: fixed1.8.1.4
Resolution: --- → FIXED
(Assignee)

Comment 12

10 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

10 years ago
I removed both patches to fix the older GCC regression.
Keywords: fixed1.8.0.12, fixed1.8.1.4
(Assignee)

Comment 14

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

10 years ago
Attachment #255023 - Flags: approval1.8.1.4+
Attachment #255023 - Flags: approval1.8.0.12+

Updated

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

Comment 16

10 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

10 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

10 years ago
Created attachment 260944 [details]
e4x/Regress/regress-367561-02.js

Comment 19

10 years ago
Created attachment 260945 [details]
js1_5/Regress/regress-367561-01.js

Comment 20

10 years ago
Created attachment 260946 [details]
js1_5/Regress/regress-367561-03.js

Updated

10 years ago
Flags: in-testsuite+

Comment 21

10 years ago
verified fixed 1.8.0, 1.8.1, 1.9.0 browser, shell 2007-04-10
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.12, fixed1.8.1.4 → verified1.8.0.12, verified1.8.1.4
Group: security

Comment 22

10 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.