365527 - JSOP_ARGUMENTS does not set obj register

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

The interpreter case for JSOP_ARGUMENTS does not set obj register. As such using arguments() in a function body will push the older value of obj register to the stack. 

This is exploitable AFAICS as a script can organize to GC the old value and override arguments with an arbitrary function that would see as its this value GC-ed object.

Comment 1

[Igor Bukanov]

Attachment: HTML test case that crashes the browser

The test case "hopes" that the branch callback calls the GC at the right moment after executing obj.x getter which unroots the old obj value stored in the register. To see the crash it may be necessary to click continue in the slow script alert.

Comment 2

[Igor Bukanov]

Attachment: Fix v1

The fix sets obj to NULL as arguments should be treated as any other var declared in a function at the call site.

Comment 3

[Igor Bukanov]

The same problem exists on branches.

Comment 4

[Igor Bukanov]

Comment on attachment 250055 [details] [diff] [review]
Fix v1

The patch applies to the branches, 1.8.0 requires a trivial merger as the patch is not applicable there as-is due to too much context.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 250055 [details] [diff] [review]
Fix v1

r=me, should go into branches ASAP as it is a totally safe fix. Thanks,

/be

Comment 6

[Igor Bukanov]

Here is a test case for jsshell to demonstrate the problem:

~/m/files> cat y1.js 
function gc_obj()
{
  obj = null;
  gc();
}

var obj = { x: { valueOf: gc_obj }};

function f()
{
  arguments = function() { this.something };
  // obj.x stores obj in the register and +obj.x calls gc_obj 
  // as valueOf method. That GC the register value.
  +obj.x;
  arguments();
}

f();
~/m/files> ~/m/trunk/mozilla/js/src/Linux_All_DBG.OBJ/js  y1.js
before 27696, after 27696, break 0812a000
Segmentation fault (core dumped)

Comment 7

[Igor Bukanov]

I committed the patch from comment 2 to the trunk:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.316; previous revision: 3.315
done

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 250055 [details] [diff] [review]
Fix v1

Approved for 1.8/1.8.0 branches, a=dveditz for drivers

Comment 9

[Igor Bukanov]

I committed the patch from comment 2 to MOZILLA_1_8_BRANCH:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.79; previous revision: 3.181.2.78
done

Comment 10

[Igor Bukanov]

I committed the patch from comment 2 to MOZILLA_1_8_0_BRANCH:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.17.2.25; previous revision: 3.181.2.17.2.24
done

Comment 11

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Function/regress-365527.js

Comment 12

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.0, 1.8.1, 1.9.0 2007-01-23 win/mac*/linux

Comment 13

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-365527.js,v  <--  regress-365527.js
initial revision: 1.1