1388110 - UnsafeSetReservedSlot self-hosting intrinsic is less safe than expected

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jon Coppeard (:jonco)]

The implementation of intrinsic_UnsafeSetReservedSlot uses setReservedSlot to set the slot, but when it's inlined by the JIT MStoreFixedSlot is used.  This breaks for objects with > 16 reserved slots.

The same applies to the associated get intrinsics.

Comment 2

[Jan de Mooij [:jandem]]

I don't think we call these functions on arbitrary objects, so it doesn't really matter if some embedding has objects with more than 16 reserved slots.

Comment 3

[Nicolas B. Pierron [:nbp] {away}]

That sounds right, these are self-hosted intrinsic and self-hosting code is not extendable through any API yet.

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Nicolas B. Pierron [:nbp] from comment #1)
> I do not think we have any in the JS Shell

I bumped into this adding more reserved slots to ModuleObject in bug 1374239.

Comment 5

[Boris Zbarsky [:bzbarsky]]

This is not an issue for the DOM because the DOM doesn't really do self-hosted code so far.

If we were to start doing that, it could absolutely be an issue; there is nothing preventing a DOM object from having more than 16 slots.  In addition to that, some DOM objects are proxies and I'm quite sure this intrinsic does the wrong thing for a proxy, for which the slot-to-fixes-slot mapping is different from a normal object...

I can't speak to Servo's situation; jdm might know.

Comment 6

[Josh Matthews [:jdm]]

No self-hosted code in Servo yet either.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: bug1388110-slot-access-intrinsics

Here's a minimal fix that stops us inlining in this case.

Comment 8

[Nicolas B. Pierron [:nbp] {away}]

Comment on attachment 8894941 [details] [diff] [review]
bug1388110-slot-access-intrinsics

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

Sounds ok

::: js/src/jit/MCallOptimize.cpp
@@ +2890,5 @@
>          return InliningStatus_NotInlined;
>      uint32_t slot = uint32_t(arg->toConstant()->toInt32());
>  
> +    // Don't inline if it's not a fixed slot.
> +    if (slot >= 16)

nit: use NativeObject::MAX_FIXED_SLOT

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0d805b314ba
Fix slot access intrinsics for objects with > 16 reserved slots r=nbp

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/e0d805b314ba

Comment 11

[Shu-yu Guo [:shu]]

This bug has a quality title.