Bad assumptions about Array elements while iterating

VERIFIED FIXED

Status

()

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

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({verified1.8.1})

Trunk
verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 +
blocking1.8.0.8 -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] js1.7 feature gc hazard)

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

12 years ago
This is a spin-off of the bug 354151. See comments there related to incorrect assumptions about Array instances while iterating. To summaries, the are 2 independent bugs caused by jsinterp.c calling js_CallIteratorNext with 2 pointers to unrooted locations where GC-things can be stored.

Example 1 (bug 354151 comment 3):
--------------------------------------------
 ~/m/trunk/mozilla/js/src> cat ~/m/files/y.js 
var obj = {get a(){ return new Object(); }};

function setter(v)
{
  // Push out obj.a from all temp roots
  var tmp = { get toString() { return new Object(); }};
  try { String(tmp); } catch (e) {  }
  gc();
}

Array.prototype.__defineGetter__(0, function() { });
Array.prototype.__defineSetter__(0, setter);

for (var i in Iterator(obj))
  print(uneval(i));

~/m/trunk/mozilla/js/src> ./Linux_All_DBG.OBJ/js ~/m/files/y.js
Key value
before 9232, after 9232, break 08142000
Segmentation fault

--------------------------------------------

Example 2 (bug 354151 comment 13):
--------------------------------------------

~/m/trunk/mozilla/js/src> cat ~/m/files/y.js
function get_value()
{
  // Unroot the first element
  this[0] = 0;

  // Call gc to collect atom corresponding to Math.sqrt(2)
  gc();
}

var counter = 2;
Iterator.prototype.next = function()
{
  if (counter-- <= 0) throw StopIteration;
  var a = [Math.sqrt(2), 1];
  a.__defineGetter__(1, get_value);
  return a; 
};

for (i in [1]);

~/m/trunk/mozilla/js/src> js ~/m/files/y.js 
before 9232, after 9232, break 08180000
Segmentation fault
(Assignee)

Comment 1

12 years ago
I think the best way to address this would be to move the current code that follows the call to js_CallIteratorNext in jsinterp.c until the code that assigns the result using switch before end_forinloop into js_CallIteratorNext itself. 

This still would require to replace &rval by a pointer to a rooted location as described in  bug 354151 comment 8, but the end result would more managable code I believe. For example, in the case of JSITER_ENUMERATE, only fid would eventially becomes rval and there is no need to read rval from the [key, value] pair in CheckKeyValueReturn.

But to avoid passing to js_CallIteratorNext way to much parameters I need to clarify the following:

1. jsinterp.c starting from line 2731 contains the following comments:

                /*
                 * Store the current object below the iterator for generality:
                 * with the iteration protocol, we cannot assume that a native
                 * iterator was found or created by js_ValueToIterator, so we
                 * can't use its parent slot to track the current object being
                 * iterated along origobj's prototype chain.  We need another
                 * stack slot, which JSOP_STARTITER allocated for us.
                 */
                vp[-1] = OBJECT_TO_JSVAL(obj);

But as far as I understand, it is necessary to track the prototype chain only for JSITER_ENUMERATE iterator. And that can only be the standard native iterator, which has obj in its parent slot. Did I miss something?

2. Line 2851 contains:
            if (flags & JSITER_FOREACH) {
                /* Clear the local foreach flag set by our prefix bytecode. */
                flags = 0;

But flags are not used after that AFAICS. Is this line really necessary?
(Assignee)

Updated

12 years ago
Depends on: 354750
(In reply to comment #1)
> But as far as I understand, it is necessary to track the prototype chain only
> for JSITER_ENUMERATE iterator. And that can only be the standard native
> iterator, which has obj in its parent slot. Did I miss something?

No, I did -- and this allows us to shrink the stack budget allocated by JSOP_STARTITER and freed (along with another slot) by JSOP_ENDITER by one slot.

I should have seen this -- thanks for pointing it out.  Can you patch this bug today?  I'll do it otherwise, but I think I should fire myself as enumeration vs. iteration hacker.

> 2. Line 2851 contains:
>             if (flags & JSITER_FOREACH) {
>                 /* Clear the local foreach flag set by our prefix bytecode. */
>                 flags = 0;
> 
> But flags are not used after that AFAICS. Is this line really necessary?

Evidently not -- a holdover from previous revisions, or always-useless code.

/be

Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?
Whiteboard: [sg:critical?] gc hazard
(Assignee)

Comment 3

11 years ago
This is 1.8.1 and later bug as it affects only the new code implementing JS 1.7 iterator protocol. In 1.8.0 and earlier versions the enumerator never creates any temporary arrays.

Thus "blocking 1.8.0.9" flag can be set to "-".
(Assignee)

Comment 4

11 years ago
About a fix. I would like to do it after fixing bug 354982 as it would be much simpler to do after the cleanup.
(Assignee)

Updated

11 years ago
Blocks: 355410
(Assignee)

Comment 5

11 years ago
Created attachment 241230 [details] [diff] [review]
Fix v1

A minimal fix. Strictly speaking the rooting in CheckKeyValueReturn is not necessary as I already learned from bug 354982 that the rval is not used. But I want to have a bulletproof fix.
Attachment #241230 - Flags: review?(brendan)
Comment on attachment 241230 [details] [diff] [review]
Fix v1

Is the atom (not its key) GC-safe?  Only if a last ditch GC is the only kind of GC that could nest.  If a full GC could be forced, the atom (id) could be unreachable (if the property is deleted) and then collected.

/be
(Assignee)

Comment 7

11 years ago
Note that this would be conflicting with patch for 354982 but the remedy there is trivial: continue to remove the CheckKeyValueReturn and keep the fix for NewKeyValuePair.
(Assignee)

Comment 8

11 years ago
Created attachment 241264 [details] [diff] [review]
Fix v2

Fix against the trunk. Unless the patch from bug 354982 is also landed, the fix for 1.8.1 would aslo require rooting of the atom in CheckKeyValueReturn.
Attachment #241230 - Attachment is obsolete: true
Attachment #241264 - Flags: review?(brendan)
Attachment #241230 - Flags: review?(brendan)
(Assignee)

Comment 9

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

The patch applies to 1.8.1 as is and fixes explotable GC hazard.
Attachment #241264 - Flags: approval1.8.1?
Comment on attachment 241264 [details] [diff] [review]
Fix v2

I like it, but we usually don't make such explicit comments for patches to s-s bugs.  Not that it adds much info, but ....

Drivers: if you don't approve for 1.8.1, please nominate for 1.8.1.1.

/be
Attachment #241264 - Flags: review?(brendan) → review+
(Assignee)

Comment 11

11 years ago
Created attachment 241271 [details] [diff] [review]
Fix v2 without comments

Patch to commit with comments removed.
Attachment #241264 - Attachment is obsolete: true
Attachment #241264 - Flags: approval1.8.1?
(Assignee)

Comment 12

11 years ago
I committed the patch from comment 11 to the trunk:

Checking in jsiter.c;
/cvsroot/mozilla/js/src/jsiter.c,v  <--  jsiter.c
new revision: 3.49; previous revision: 3.48
done
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

11 years ago
Comment on attachment 241271 [details] [diff] [review]
Fix v2 without comments

Asking for approval for the committed patch.
Attachment #241271 - Flags: approval1.8.1?
Attachment #241271 - Flags: approval1.8.1.1?
(Assignee)

Comment 14

11 years ago
Created attachment 241272 [details] [diff] [review]
Fix v2 without comments and smaller diff context

This a diff with smaller context so the patch can be applied to 1.8.1 with or without the cleanup patch from bug 355410.
Attachment #241271 - Attachment is obsolete: true
Attachment #241272 - Flags: approval1.8.1?
Attachment #241272 - Flags: approval1.8.1.1?
Attachment #241271 - Flags: approval1.8.1?
Attachment #241271 - Flags: approval1.8.1.1?
(In reply to comment #14)
> Created an attachment (id=241272) [edit]
> Fix v2 without comments and smaller diff context
> 
> This a diff with smaller context so the patch can be applied to 1.8.1 with or
> without the cleanup patch from bug 355410.

The cleanup patch from bug 354982, if I'm following.

/be
(Assignee)

Comment 16

11 years ago
(In reply to comment #15)
> > This a diff with smaller context so the patch can be applied to 1.8.1 with or
> > without the cleanup patch from bug 355410.
> 
> The cleanup patch from bug 354982, if I'm following.

Yes, bug 354982. Late night hacking, you know.

Comment 17

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

Comment 18

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

I could not reproduce a crash with this test.

Updated

11 years ago
Flags: blocking1.8.0.9? → in-testsuite+

Updated

11 years ago
Flags: blocking1.8.1?

Comment 19

11 years ago
verified fixed 1.9 20061005 windows/linux
Status: RESOLVED → VERIFIED
Blocking for Fx2 RC3
Flags: blocking1.8.1? → blocking1.8.1+

Comment 21

11 years ago
Comment on attachment 241272 [details] [diff] [review]
Fix v2 without comments and smaller diff context

Approved for RC3.
Attachment #241272 - Flags: approval1.8.1? → approval1.8.1+

Updated

11 years ago
Flags: blocking1.8.1.1?

Updated

11 years ago
Attachment #241272 - Flags: approval1.8.1.1?
(Assignee)

Comment 22

11 years ago
I committed the patch from comment 11 to MOZILLA_1_8_BRANCH:

Checking in jsiter.c;
/cvsroot/mozilla/js/src/jsiter.c,v  <--  jsiter.c
new revision: 3.17.2.24; previous revision: 3.17.2.23
done
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.1
Flags: blocking1.8.0.8-
Whiteboard: [sg:critical?] gc hazard → [sg:critical?] js1.7 feature gc hazard

Comment 23

11 years ago
verified fixed 20061009 1.8 windows/linux/mac* 1.9 windows/linux
note to self: for the -01 test the harness failed to report the test results but did not crash.
Keywords: fixed1.8.1 → verified1.8.1

Comment 24

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

reset Array.prototype[0]
Attachment #241306 - Attachment is obsolete: true
Group: security

Comment 25

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

/cvsroot/mozilla/js/tests/js1_7/extensions/regress-354499-02.js,v  <--  regress-354499-02.js
You need to log in before you can comment on or make changes to this bug.