The default bug view has changed. See this FAQ.

GC hazard with unrooted ids in Object.toSource

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({verified1.8.0.8, verified1.8.1})

Trunk
verified1.8.0.8, verified1.8.1
Points:
---
Bug Flags:
blocking1.8.0.8 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

11 years ago
Created attachment 237533 [details] [diff] [review]
Fix

The fix prevents atoms from GC for the duration of sharp table presence.
Attachment #237533 - Flags: superreview?(brendan)
Attachment #237533 - Flags: review?(mrbkap)
Flags: blocking1.8.1?
Flags: blocking1.8.0.8?
(Assignee)

Comment 2

11 years ago
Created attachment 237534 [details] [diff] [review]
Fix v2
Attachment #237533 - Attachment is obsolete: true
Attachment #237534 - Flags: superreview?(brendan)
Attachment #237534 - Flags: review?(mrbkap)
Attachment #237533 - Flags: superreview?(brendan)
Attachment #237533 - Flags: review?(mrbkap)
(Assignee)

Comment 3

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

The previous patch did not compile ;)
(Assignee)

Comment 4

11 years ago
The bug exists in older versions of the browser as well.
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
(Assignee)

Comment 5

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

Updated

11 years ago
Attachment #237534 - Flags: review?(mrbkap) → review+
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
Attachment #237534 - Flags: superreview?(brendan)
Attachment #237534 - Flags: superreview+
Attachment #237534 - Flags: approval1.8.1?
(Assignee)

Comment 7

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

11 years ago
Comment on attachment 237534 [details] [diff] [review]
Fix v2

a=schrep for js mem issue
Attachment #237534 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Comment 9

11 years ago
I committed the patch from comment 2 to the trunk.
(Assignee)

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 10

11 years ago
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
Keywords: fixed1.8.1
(Assignee)

Comment 11

11 years ago
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.
Attachment #237534 - Flags: approval1.8.0.7?

Comment 12

11 years ago
Created attachment 238402 [details]
js1_5/GC/regress-351973.js

Updated

11 years ago
Flags: in-testsuite+

Comment 13

11 years ago
verified fixed 1.8 20060914 windows/linux 1.9 20060914 windows/mac*/linux
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1 → verified1.8.1
Attachment #237534 - Flags: approval1.8.0.7? → approval1.8.0.8?
Restoring lost blocking flag
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9? → blocking1.8.0.8?
Flags: blocking1.8.0.8? → blocking1.8.0.8+
(Assignee)

Updated

11 years ago
Attachment #237534 - Flags: approval1.8.0.8?
Comment on attachment 237534 [details] [diff] [review]
Fix v2

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #237534 - Flags: approval1.8.0.9?
Attachment #237534 - Flags: approval1.8.0.8?
Attachment #237534 - Flags: approval1.8.0.8+
(Assignee)

Comment 16

11 years ago
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
Keywords: fixed1.8.0.8
Whiteboard: [sg:critical?]

Comment 17

11 years ago
verified fixed 1.8.0.8 20060927 windows/mac*/linux
Keywords: fixed1.8.0.8 → verified1.8.0.8
Group: security

Comment 18

10 years ago
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-351973.js,v  <--  regress-351973.js
This bug is verified1.8.1. No need for blocking request.
Flags: blocking1.8.1?
(Assignee)

Updated

10 years ago
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
You need to log in before you can comment on or make changes to this bug.