Hang if Intl.getCanonicalLocales gets an Array-like-object with negative length

RESOLVED FIXED in Firefox 49

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

unspecified
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
This code hangs SpiderMonkey:

```

var x = {}; 
Object.defineProperty(x, 'length', {get: function(){return -5}});

Intl.getCanonicalLocales(x);

```

Same happens with any of the Intl formatters if you pass x as the first param, because it's the same operation.

Seems like something is going wrong with https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/Intl.js#664

:waldo, any ideas?
(Assignee)

Updated

2 years ago
Flags: needinfo?(jwalden+bmo)
To be clear, when you say "hang", you mean in the shell only, right?  And if you try in a browser, you get a slow script dialog?  If the latter doesn't happen we consider that a bug, but we don't care about the former.  A script that runs forever *should* offer the opportunity to cancel in the browser, but it *should* keep running as long as the user allows it to.

As for the actual issue here.

Prior versions of the spec did ToUint32(O.length), to determine how many iterations to run grabbing locales off the locale-list value.  That would convert -1 to uint32_t(-1), i.e. a big value.  Latest spec iteration, on the other hand, seems to use ToLength instead -- which says, "If len ≤ +0, return +0."

So the fix is approximately s/TO_UINT32/ToLength/ here (plus a test), as long as ToLength is defined for self-hosting code use.  I think it is, but you'll have to double-check.

Note that a better form of the test would be this (so the test fails fast, rather than taking forever and a day to fail by timing out):

  var obj = { get 0() { throw new Error("must not be gotten!"); },
              length: -Math.pow(2, 32) + 1 };

  // With older ECMA-402 editions, TO_UINT32(obj.length) would be 1, so obj[0]
  // would be accessed.  But newer editions' ToLength(obj.length) is +0, so
  // obj[0] must not be accessed.
  Intl.getCanonicalLocales(obj);
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 2

2 years ago
Cool! It did fix the issue.

I'm about to land test262 tests for getCanonicalLocales which will cover those scenarios. Do you want me to also add local tests for this with the fix, or is test262 coverage enough?
Flags: needinfo?(jwalden+bmo)
(Assignee)

Updated

2 years ago
Assignee: nobody → gandalf
Add my seven-liner test in both places.  Eventually we'll have test262 importation be turnkey, but there's tc39-side coordination to do first, and we want the test in advance of that.
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 4

2 years ago
Created attachment 8759783 [details] [diff] [review]
bug1275183.diff
Attachment #8759783 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
Attachment #8759783 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 5

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a2fe25f4c67
"Hang if Intl.getCanonicalLocales gets an Array-like-object with negative length". r=waldo
Keywords: checkin-needed

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0a2fe25f4c67
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.