1220766 - Incorrect use of UnsafeGetInt32FromReservedSlot in CreateListIterator

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

Test case:
---
iter = getSelfHostedValue("CreateListIterator")([]);
iter.next();
iter.next();
---

Asserts with:
Assertion failure: vp->isInt32(), at /home/andre/git/mozilla-central/js/src/vm/SelfHosting.cpp:389


Stack trace:
---
#0  0x0000000000c86442 in intrinsic_UnsafeGetInt32FromReservedSlot (cx=0x7ffff691b000, argc=2, vp=0x7ffff3c62148) at /home/andre/git/mozilla-central/js/src/vm/SelfHosting.cpp:389
#1  0x0000000000bdeba8 in js::CallJSNative (cx=0x7ffff691b000, native=0xc863d3 <intrinsic_UnsafeGetInt32FromReservedSlot(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /home/andre/git/mozilla-central/js/src/jscntxtinlines.h:235
#2  0x0000000000bba7a2 in js::Invoke (cx=0x7ffff691b000, args=..., construct=js::NO_CONSTRUCT) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:489
#3  0x0000000000bc9c14 in Interpret (cx=0x7ffff691b000, state=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:2798
#4  0x0000000000bba38e in js::RunScript (cx=0x7ffff691b000, state=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:430
#5  0x0000000000bba862 in js::Invoke (cx=0x7ffff691b000, args=..., construct=js::NO_CONSTRUCT) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:507
#6  0x0000000000bc9c14 in Interpret (cx=0x7ffff691b000, state=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:2798
#7  0x0000000000bba38e in js::RunScript (cx=0x7ffff691b000, state=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:430
#8  0x0000000000bbb979 in js::ExecuteKernel (cx=0x7ffff691b000, script=..., scopeChainArg=..., thisv=..., newTargetValue=..., type=js::EXECUTE_GLOBAL, evalInFrame=..., 
    result=0x7fffffffd500) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:703
#9  0x0000000000bbbd53 in js::Execute (cx=0x7ffff691b000, script=..., scopeChainArg=..., rval=0x7fffffffd500) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:742
#10 0x00000000009b648b in ExecuteScript (cx=0x7ffff691b000, scope=..., script=..., rval=0x7fffffffd500) at /home/andre/git/mozilla-central/js/src/jsapi.cpp:4388
#11 0x00000000009b67e3 in JS_ExecuteScript (cx=0x7ffff691b000, scriptArg=..., rval=...) at /home/andre/git/mozilla-central/js/src/jsapi.cpp:4414
#12 0x0000000000431edc in EvalAndPrint (cx=0x7ffff691b000, 
    bytes=0x7ffff6914f80 "iter = getSelfHostedValue(\"CreateListIterator\")([]); iter.next(); iter.next()\n", '\344' <repeats 50 times>, "\300\063\346\367\377\177\374\377", 
    length=78, lineno=1, compileOnly=false, out=0x7ffff6ef7740 <_IO_2_1_stdout_>) at /home/andre/git/mozilla-central/js/src/shell/js.cpp:536
#13 0x00000000004323a2 in ReadEvalPrintLoop (cx=0x7ffff691b000, in=0x7ffff6ef6980 <_IO_2_1_stdin_>, out=0x7ffff6ef7740 <_IO_2_1_stdout_>, compileOnly=false)
    at /home/andre/git/mozilla-central/js/src/shell/js.cpp:600
#14 0x0000000000432562 in Process (cx=0x7ffff691b000, filename=0x0, forceTTY=true) at /home/andre/git/mozilla-central/js/src/shell/js.cpp:632
#15 0x0000000000446ad7 in ProcessArgs (cx=0x7ffff691b000, op=0x7fffffffda20) at /home/andre/git/mozilla-central/js/src/shell/js.cpp:5986
#16 0x0000000000447eca in Shell (cx=0x7ffff691b000, op=0x7fffffffda20, envp=0x7fffffffdc28) at /home/andre/git/mozilla-central/js/src/shell/js.cpp:6320
#17 0x0000000000449314 in main (argc=1, argv=0x7fffffffdc18, envp=0x7fffffffdc28) at /home/andre/git/mozilla-central/js/src/shell/js.cpp:6677
---

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1220766-list-iterator

Yes, we can't assume we have an int32 there.

Comment 2

[Shu-yu Guo [:shu]]

Comment on attachment 8682420 [details] [diff] [review]
bug1220766-list-iterator

Review of attachment 8682420 [details] [diff] [review]:
-----------------------------------------------------------------

Eh. I feel like... just because the fuzzers can generate this doesn't mean we shouldn't be able to assume it's always an Int32. Can it ever be non-Int32 except through API misuse and fuzzing?

Comment 3

[Shu-yu Guo [:shu]]

Comment on attachment 8682420 [details] [diff] [review]
bug1220766-list-iterator

Review of attachment 8682420 [details] [diff] [review]:
-----------------------------------------------------------------

I thought this was a fuzz bug but it isn't. Self-hosted functions are allowed to have stricter contracts for use and it's fine to crash when misusing them.

Comment 4

[André Bargull [:anba]]

The list iterator will get exposed to user script when Reflect.enumerate is implemented, so it seems useful to fix this bug now.

Comment 5

[Shu-yu Guo [:shu]]

(In reply to André Bargull from comment #4)
> The list iterator will get exposed to user script when Reflect.enumerate is
> implemented, so it seems useful to fix this bug now.

And Reflect.enumerate can return an iterator whose internal next slot isn't an int32?

Comment 6

[André Bargull [:anba]]

From ES2015:
26.1.5 Reflect.enumerate calls the internal [[Enumerate]] method, [[Enumerate]] for module namespace exotic objects (9.4.6.11) returns the result of CreateListIterator (7.4.8).

The current implementation of the list iterator's next method in js/src/builtins/Iterator.js sets the ITERATOR_SLOT_NEXT_INDEX reserved slot to Infinity when the iterator is drained. So if next() is called after the iterator has finished, the line `let index = UnsafeGetInt32FromReservedSlot(this, ITERATOR_SLOT_NEXT_INDEX)` will trigger an assertion, because ITERATOR_SLOT_NEXT_INDEX is Infinity which is not an int32. 

With proper support for Reflect.enumerate, I'd use the following STR:
---
1. Create javascript module file "testcase.js":
```
import* as self from "testcase.js";

var iter = Reflect.enumerate(self);
iter.next();
iter.next();
```

2. Execute module "testcase.js".
---

Unfortunately Reflect.enumerate is not yet supported, so the only way to demonstrate the bug right now is to use the internal getSelfHostedValue() function.

Comment 7

[Jon Coppeard (:jonco)]

CreateListIterator is also used for iteration of module namespaces and it's possible to trigger this assertion with a testcase based on modules so I think we need to fix this.  Re-flagging for review.

Comment 8

[André Bargull [:anba]]

Oh, I totally forgot about the built-in module iteration. New test case without getSelfHostedValue():
---
var moduleRepo = Object.create(null);
setModuleResolveHook(function(module, specifier) {
  if (specifier in moduleRepo)
    return moduleRepo[specifier];
  throw "Module " + specifier + " not found";
});
var m = parseModule("import* as self from 'a'; var iter = self[Symbol.iterator](); iter.next(); iter.next();");
moduleRepo["a"] = m;
m.declarationInstantiation();
m.evaluation();
---

Comment 9

[Shu-yu Guo [:shu]]

Comment on attachment 8682420 [details] [diff] [review]
bug1220766-list-iterator

Review of attachment 8682420 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for posting other STR.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ea27dc60ccd

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7ea27dc60ccd