351973 - GC hazard with unrooted ids in Object.toSource

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Consider the following example for js shell:

function removeAllProperties(o)
{
  for (var prop in o)
    delete o[prop];
  for (var i = 0; i != 50*1000; ++i) {
    var tmp = Math.sqrt(i+0.2);
    tmp = 0;
  }
  if (typeof gc == "function")
    gc();
}

function test()
{

  var o = {};
  o.first = { toSource: function() { removeAllProperties(o); } };
  for (var i = 0; i != 10; ++i) {
    o[Math.sqrt(i + 0.1)] = 1;
  }
  return o.toSource();
}

print(test());

When executed, it crashes the shell:
~/build/1.8.1/dbg/dist/bin> run-mozilla.sh xpcshell ~/s/x.js
Type Manifest File: /home/igor/build/1.8.1/dbg/dist/bin/components/xpti.dat
nsNativeComponentLoader: autoregistering begins.
nsNativeComponentLoader: autoregistering succeeded
nsNativeComponentLoader: registering deferred (0)
before 913968, after 9232, break 08127000
./run-mozilla.sh: line 131: 10065 Segmentation fault      "$prog" ${1+"$@"}

The reason for the crash is that js_EnterSharpObject returns an array of property ids and nothing roots the atoms that can present in the array. 
Thus the GC triggered either through explicit gc() call (shell) or a branch callback (browser) would lead to accessing the freed memory in Object.toSource() when properties are removed during the string assembling loop.

Comment 1

[Igor Bukanov]

Attachment: Fix

The fix prevents atoms from GC for the duration of sharp table presence.

Comment 2

[Igor Bukanov]

Attachment: Fix v2

Comment 3

[Igor Bukanov]

(In reply to comment #2)
> Created an attachment (id=237534) [edit]
> Fix v2
> 

The previous patch did not compile ;)

Comment 4

[Igor Bukanov]

The bug exists in older versions of the browser as well.

Comment 5

[Igor Bukanov]

It looks like all usages of JS_Enumerate() has to be monitored. For example, AFAICS with my limited knowledge of XPConnect internals http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcwrappedjsclass.cpp#343 has the same bug.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 237534 [details] [diff] [review]
Fix v2

Looks good, thanks.

My memory is failing me: didn't you have a patch to protect (via the mark phase) the JSIdArrays returned by JS_Enumerate?

/be

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> My memory is failing me: didn't you have a patch to protect (via the mark
> phase) the JSIdArrays returned by JS_Enumerate?

Fix for bug 340129 roots the elements of the mark table, not the ids array.

Comment 8

[Mike Schroepfer]

Comment on attachment 237534 [details] [diff] [review]
Fix v2

a=schrep for js mem issue

Comment 9

[Igor Bukanov]

I committed the patch from comment 2 to the trunk.

Comment 10

[Igor Bukanov]

I committed the patch from comment 2 to MOZILLA_1_8_BRANCH:

Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.208.2.32; previous revision: 3.208.2.31
done

Comment 11

[Igor Bukanov]

Comment on attachment 237534 [details] [diff] [review]
Fix v2

The patch applies to  1.8.0 branch as is. It maybe too late for 1.8.0.7 but I still ask.

Comment 12

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/GC/regress-351973.js

Comment 13

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 20060914 windows/linux 1.9 20060914 windows/mac*/linux

Comment 14

[Daniel Veditz [:dveditz]]

Restoring lost blocking flag

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 237534 [details] [diff] [review]
Fix v2

approved for 1.8.0 branch, a=dveditz for drivers

Comment 16

[Igor Bukanov]

I committed the pacth from comment 2 to MOZILLA_1_8_0_BRANCH:

Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.208.2.12.2.14; previous revision: 3.208.2.12.2.13
done

Comment 17

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.0.8 20060927 windows/mac*/linux

Comment 18

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-351973.js,v  <--  regress-351973.js

Comment 19

[Simon Paquet [:sipaq]]

This bug is verified1.8.1. No need for blocking request.