681803 - non-AS3 Array.prototype.push property modification loop wraps around

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Library, defect)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

Followup work from Bug 681399 (and mentioned in passing in Bug 661330).

Right now our Array.prototype.push doing arithmetic mod 2^32.

This has consequences which we probably do not intend.  Compliance with ECMA-262 and with the majority of web browsers entails not using modular arithmetic in the overflowing cases.

See:
  Bug 681399, comment 5
  Bug 681399, comment 15
  Bug 681399, comment 22

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch B v2: versioned [[Put]] variation in non-AS3 .push

This patch assumes that one has already pushed attachment 556063 [details] [diff] [review] from Bug 661330.

(It is not strictly necessary to fix Bug 661330 in order to fix this bug; it just seemed like the natural way to get the ECMA-262 behavior.  Its easy to remove the !bug661330 conjunct, and then we'd just avoid throwing the RangeError.)

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #1)
> Created attachment 556065 [details] [diff] [review]
> patch B v2: versioned [[Put]] variation in non-AS3 .push

(oh, its v2 because its a refinement of patch B v1, attachment 555262 [details] [diff] [review] on Bugzilla 681399.)

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch B v3: versioned [[Put]] variation in non-AS3 .push

(miniscule fix to mistyped BZ number in a comment.)

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #3)
> Created attachment 556067 [details] [diff] [review]
> patch B v3: versioned [[Put]] variation in non-AS3 .push
> 
> (miniscule fix to mistyped BZ number in a comment.)

(I probably need to change the return value of push in the overflow case as well.  I'm not going to update the patch for this immediately; I just wanted to note it ahead of any review.)

Comment 5

[Lars T Hansen]

Comment on attachment 556067 [details] [diff] [review]
patch B v3: versioned [[Put]] variation in non-AS3 .push

Looks OK and is, from what I understand, in line with commonly observed behavior in most browsers.

The casts in this line are redundant:

  const overflowed_length:Number = Number(n) + Number(argc);

Reason: There are no int or uint values in the language, just Number values (int and uint are predicates on locations).  Once n and argc have been read from their variables, the resulting values are numbers.

The only nagging issue is that Array can be subclassed and the length setter can be overridden in the subclass.  This code will hit the "this is Array" case for a subclassed vector and the overridden setter will therefore observe a clamped length rather than one that's wrapped around.  I think that's OK.

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

An odd detail: This test:

  function WithLength(l) { this.length = l; }
  var a = new WithLength(uint.MAX_VALUE - 2);
  Array.prototype.public::push.call(a,1,2,3,4,5);
  print(a.length);

currently prints 2 on the following hosts:

  32-bit release interpreter (-Dinterp)
  32-bit release jit (-Ojit)
  64-bit release jit (-Ojit)

but on a 64-bit interpreter, it prints 4294967298.

(Thats not even a uint!)

It might be worth checking if this is a jit/interpreter discrepancy that is inadvertently exposed here...

Comment 7

[Lars T Hansen]

We might want to keep bug #601426 in mind here.

Comment 8

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch B v4: versioned [[Put]] variation with guarded set_length call

Addressed feedback from comment 5.

This introduces a dependence on Bug 661330, so I'll mark that bug as blocking this one.

Also, there's a follow-on patch that fixes the length_mods.as test case that was posted to Bug 661330; I'll be posting that here next.

I didn't version-check the assignment of the overflowed-length to a non-array, but if anyone wants me to, I will.  Its observable when you apply this generic function to a non-array object that has a length property of type *.  (If we do version it, the question will then become whether to assign a clamped length or a wrapped one.  It seems simpler to just continue down the path we've already chosen of "just fix it" for these oddball length cases.)

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch T v1: fixes test from Bug 661330.

here's the test-fixing patch mentioned in comment 8.

Comment 10

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 559450 [details] [diff] [review]
patch B v4: versioned [[Put]] variation with guarded set_length call

(Ed: feel free to reassign yourself as SR? and put someone else as R?.)

This is meant to be applied atop Bug 661330's attachment 559447 [details] [diff] [review].

Comment 11

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 559452 [details] [diff] [review]
patch T v1: fixes test from Bug 661330.

(This is meant to be applied atop Bug 661330's attachment 559294 [details] [diff] [review])

This test specifically only deals with .length modification.  It is not checking what the state of the push receiver is otherwise.

For Bug 681803, we need a separate test that inspects how the property modification loop in push is dealing with the overflow cases (because ECMA-262 says that the overflowed indices get written without wrapping around, writing properties with keys that are not array indices).  That would be a good follow-on task for QE.

Comment 12

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 559450 [details] [diff] [review]
patch B v4: versioned [[Put]] variation with guarded set_length call

(revising reviewers based on advice from dan.  as stated above, probably best to review in parallel with patch from Bug 661330.)

Comment 13

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 559452 [details] [diff] [review]
patch T v1: fixes test from Bug 661330.

(retracting review request for this revision of the test; it doesn't pass and so I think I messed something up; probably minor but no reason to waste time reviewing broken code.)

Comment 14

[Tamarin Bot]

changeset: 6709:ac91969bcc46
user:      Felix S Klock II <[e-mail]>
summary:   Bug 661330: acceptance tests for modifying Array .length property (r=dschaffe).

http://hg.mozilla.org/tamarin-redux/rev/ac91969bcc46

Comment 15

[Felix S. Klock II [:pnkfelix, :fklock]]

(resolved wrong bug)

Comment 16

[Felix S. Klock II [:pnkfelix, :fklock]]

This is too risky for me to push today.  But someone should pick up the work here, the patch has already gotten some review, and we certainly don't want to put the bogus semantics into AS4.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.