1655405 - Assertion failure: name.isString() || name.isSymbol() || name.isNumber(), at vm/JSFunction.cpp:2446 with Class and BigInt

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20200727-56082fc4acfa (debug build, run with --fuzzing-safe --ion-offthread-compile=off):

class C66 {
    128n = class {};
}
new C66();

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x0000555555c44833 in js::SetFunctionName(JSContext*, JS::Handle<JSFunction*>, JS::Handle<JS::Value>, js::FunctionPrefixKind) ()
#1  0x0000555555940e5a in Interpret(JSContext*, js::RunState&) ()
#2  0x0000555555932932 in js::RunScript(JSContext*, js::RunState&) ()
[...]
#10 0x00005555557bccea in main ()
rax	0x55555706e515	93825020650773
rbx	0xfffb800000000000	-1266637395197952
rcx	0x5555583dd840	93825041029184
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7fffffffbbe0	140737488337888
rsp	0x7fffffffbba0	140737488337824
r8	0x7ffff7105770	140737338431344
r9	0x7ffff7f9bd40	140737353727296
r10	0x58	88
r11	0x7ffff6dac7a0	140737334921120
r12	0xfff9800000000000	-1829587348619264
r13	0xaa	170
r14	0x7ffff6027000	140737320742912
r15	0xffff800000000000	-140737488355328
rip	0x555555c44833 <js::SetFunctionName(JSContext*, JS::Handle<JSFunction*>, JS::Handle<JS::Value>, js::FunctionPrefixKind)+947>
=> 0x555555c44833 <_ZN2js15SetFunctionNameEP9JSContextN2JS6HandleIP10JSFunctionEENS3_INS2_5ValueEEENS_18FunctionPrefixKindE+947>:	movl   $0x98e,0x0
   0x555555c4483e <_ZN2js15SetFunctionNameEP9JSContextN2JS6HandleIP10JSFunctionEENS3_INS2_5ValueEEENS_18FunctionPrefixKindE+958>:	callq  0x55555584d42e <abort>

Marking s-s until investigated because I don't know if this type confusion has any kind of security implications.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jeff Walden [:Waldo]]

Not security-sensitive. Salient code for js::SetFunctionName is:

bool js::SetFunctionName(JSContext* cx, HandleFunction fun, HandleValue name,
                         FunctionPrefixKind prefixKind) {
  MOZ_ASSERT(name.isString() || name.isSymbol() || name.isNumber());

  // ...irrelevant parts omitted...

  JSAtom* funName = name.isSymbol()
                        ? SymbolToFunctionName(cx, name.toSymbol(), prefixKind)
                        : NameToFunctionName(cx, name, prefixKind);
  if (!funName) {
    return false;
  }

  fun->setInferredName(funName);

  return true;
}

If a bigint flows in, it's going to hit the NameToFunctionName part.

static JSAtom* NameToFunctionName(JSContext* cx, HandleValue name,
                                  FunctionPrefixKind prefixKind) {
  MOZ_ASSERT(name.isString() || name.isNumber());

  if (prefixKind == FunctionPrefixKind::None) {
    return ToAtom<CanGC>(cx, name);
  }

  JSString* nameStr = ToString(cx, name);
  if (!nameStr) {
    return nullptr;
  }

  // ...|name| not used after this, only |nameStr|
}

And both ToAtom and ToString are capable of handling bigints safely.

This is just a botched assertion.

Comment 3

[André Bargull [:anba]]

This just needs to be updated to use isNumeric() instead of isNumber().

Class fields are implemented via normal assignments, so we're executing BytecodeEmitter::emitAssignmentOrInit(), which in turn calls BytecodeEmitter::emitAssignmentRhs() and that one calls BytecodeEmitter::emitAnonymousFunctionWithComputedName(), which leads to emitting JSOp::SetFunName.

---

Spec:
https://tc39.es/proposal-class-fields/#runtime-semantics-class-field-definition-evaluation:

1. Let name be the result of evaluating ClassElementName.

Where ClassElementName

ClassElementName[Yield, Await] : PropertyName[?Yield, ?Await]

And per https://tc39.es/ecma262/#sec-object-initializer-runtime-semantics-evaluation:

LiteralPropertyName:NumericLiteral
1. Let nbr be the NumericValue of NumericLiteral.
2. Return ! ToString(nbr).

Comment 4

[Jeff Walden [:Waldo]]

Hmm, is that right? I eyeballed something an issue, but I could have moved too fast.

Comment 5

[Jeff Walden [:Waldo]]

Oh, right, I didn't see LiteralPropertyName evaluated to a string. Looks like I should close that issue, then.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Bug 1655405 - Properly handle numeric field names initialized with anonymous functions in classes. r=evilpie!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/82268ceb01e9
Properly handle numeric field names initialized with anonymous functions in classes.  r=evilpie

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/82268ceb01e9

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:Waldo, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 10

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as fixed on rev mozilla-central 20200729094932-3059084abf6e.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.