Crash [@ NoSuchMethod] [@ js_ComputeThis] with valueOf: gc and __iterator__

VERIFIED FIXED

Status

()

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

People

(Reporter: Jesse Ruderman, Assigned: brendan)

Tracking

(Blocks: 1 bug, {crash, testcase, verified1.8.1})

Trunk
PowerPC
Mac OS X
crash, testcase, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] js1.7 feature, crash signature)

Attachments

(4 attachments)

(Reporter)

Description

11 years ago
js> ( {valueOf: gc} - [function(){}].__iterator__ )()

This usually crashes trying to dereference something near zero, but one I saw it jump to 0x20202020 and once I saw it try to dereference 0x80000007.

Comment 1

11 years ago
Here is a simple example:

js>  ( {valueOf: gc} - [].a )()
 ( {valueOf: gc} - [].a )()
before 9232, after 9232, break 08129000
Segmentation fault (core dumped)

Comment 2

11 years ago
This a bug in the emitter. Consider the following example:

var obj = {valueOf: gc };

function f() {
  ( obj * [].a )();
}

dis(f);
f();


Before crashing it prints the following bytecode for f:

main:
00000:  name "obj"
00003:  name "Object"
00006:  pushobj
00007:  newinit
00008:  one
00009:  initprop "a"
00012:  endinit
00013:  getprop "a"
00016:  mul
00017:  group
00018:  pushobj
00019:  call 0
00022:  pop
00023:  stop

Source notes:
  0:     0 [   0] newline 
  1:    13 [  13] xdelta  
  2:    13 [   0] pcbase   offset 10
  4:    19 [   6] pcbase   offset 19

here pushobj at line 18 pushes to the stack the last value of obj register. In this case it is temporary left from getprop 013 which is already GC-ed. Something is badly broken regarding that pushobj. 

Comment 3

11 years ago
This a bug in js_EmitTree when it deals with the TOK_LP case. It misses the fact that at line 5792 pn2 can be arbitrary expression.

 
(Assignee)

Comment 4

11 years ago
Regression in patch for bug 320032, which reopened bug 41864.  JSOP_GROUP must null the |obj| register if the expression really needed to be parenthesized.  This points to the fix: the parser should eliminate the TOK_RP parenthesizing parent of any member expression.

/be
(Assignee)

Comment 5

11 years ago
Created attachment 240870 [details] [diff] [review]
fix
Assignee: general → brendan
Status: NEW → ASSIGNED
Attachment #240870 - Flags: review?(igor.bukanov)

Comment 6

11 years ago
Comment on attachment 240870 [details] [diff] [review]
fix

Now it works as expected.
Attachment #240870 - Flags: review?(igor.bukanov) → review+

Comment 7

11 years ago
BTW, Rhino is significantly different from SM regarding implementing call bytecode. There the emitter generates special bytecodes to compute both this and function for all syntactical cases of () operator before calling this. It seems cleaner then various "obj = NULL".
(Assignee)

Comment 8

11 years ago
Yes, SpiderMonkey influenced Rhino early on, and didn't keep up with its child ;-) -- file a followup bug?

/be
(Assignee)

Updated

11 years ago
Blocks: 355044
(Assignee)

Comment 9

11 years ago
Safe fix, candidate for now, or if not, for the patch release.  Fixed on trunk:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.295; previous revision: 3.294
done
Checking in jsparse.c;
/cvsroot/mozilla/js/src/jsparse.c,v  <--  jsparse.c
new revision: 3.252; previous revision: 3.251
done

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Flags: blocking1.8.1?
Flags: blocking1.8.1.1?
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Attachment #240870 - Flags: approval1.8.1?

Comment 10

11 years ago
Comment on attachment 240870 [details] [diff] [review]
fix

Approved for RC2.
Attachment #240870 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Comment 11

11 years ago
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.67; previous revision: 3.181.2.66
done
Checking in jsparse.c;
/cvsroot/mozilla/js/src/jsparse.c,v  <--  jsparse.c
new revision: 3.142.2.64; previous revision: 3.142.2.63
done

/be
Keywords: fixed1.8.1
(Assignee)

Comment 12

11 years ago
The patch for this bug caused bug 355101.

/be
(Assignee)

Updated

11 years ago
Blocks: 355101

Comment 13

11 years ago
Created attachment 240906 [details]
js1_7/iterable/regress-355052-01.js

Comment 14

11 years ago
Created attachment 240907 [details]
js1_7/iterable/regress-355052-02.js

Comment 15

11 years ago
Created attachment 240908 [details]
js1_7/iterable/regress-355052-03.js

Updated

11 years ago
Flags: in-testsuite+
Whiteboard: [sg:critical?] js1.7 feature

Comment 16

11 years ago
verified fixed 1.8 1.9 20061002 windows/linux 1.8 macppc
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1 → verified1.8.1
Already in 1.8.1, clearning nomination flags.
Group: security
Flags: blocking1.8.1?
Flags: blocking1.8.1.1?

Comment 18

11 years ago
/cvsroot/mozilla/js/tests/js1_7/extensions/regress-355052-01.js,v  <--  regress-355052-01.js

/cvsroot/mozilla/js/tests/js1_7/extensions/regress-355052-02.js,v  <--  regress-355052-02.js

/cvsroot/mozilla/js/tests/js1_7/extensions/regress-355052-03.js,v  <--  regress-355052-03.js

Comment 19

11 years ago
On the trunk these exceptions have changed from 'TypeError: NaN is not a function' 
to 'TypeError: - is not a function' for 01, 02 and 'TypeError: * is not a function' for 03. Range is 2006-12-27 to 2006-12-28 and looks like bug 363536.

Do we care or should I rewrite the test to handle either case?
(Assignee)

Comment 20

11 years ago
These look like regressions to me. Igor?

/be
(Reporter)

Updated

11 years ago
No longer blocks: 349611
(Reporter)

Updated

11 years ago
Blocks: 349611
Crash Signature: [@ NoSuchMethod] [@ js_ComputeThis]
You need to log in before you can comment on or make changes to this bug.