Closed Bug 1275183 Opened 4 years ago Closed 4 years ago

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


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox49 --- fixed


(Reporter: zbraniecki, Assigned: zbraniecki)



(1 file)

This code hangs SpiderMonkey:


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



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

:waldo, any ideas?
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.
Flags: needinfo?(jwalden+bmo)
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: 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)
Attached patch bug1275183.diffSplinter Review
Attachment #8759783 - Flags: review?(jwalden+bmo)
Attachment #8759783 - Flags: review?(jwalden+bmo) → review+
Pushed by
"Hang if Intl.getCanonicalLocales gets an Array-like-object with negative length". r=waldo
Keywords: checkin-needed
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.