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

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