Closed Bug 1388110 Opened 4 years ago Closed 4 years ago

UnsafeSetReservedSlot self-hosting intrinsic is less safe than expected


(Core :: JavaScript Engine: JIT, defect)

55 Branch
Not set



Tracking Status
firefox57 --- fixed


(Reporter: jonco, Assigned: jonco)




(1 file)

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.
Flags: needinfo?(till)
Flags: needinfo?(bzbarsky)
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.
That sounds right, these are self-hosted intrinsic and self-hosting code is not extendable through any API yet.
Flags: needinfo?(till)
Flags: needinfo?(bzbarsky)
(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.
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.
Flags: needinfo?(josh)
No self-hosted code in Servo yet either.
Flags: needinfo?(josh)
Here's a minimal fix that stops us inlining in this case.
Assignee: nobody → jcoppeard
Attachment #8894941 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8894941 [details] [diff] [review]

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
Attachment #8894941 - Flags: review?(nicolas.b.pierron) → review+
Pushed by
Fix slot access intrinsics for objects with > 16 reserved slots r=nbp
Blocks: 1374239
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
This bug has a quality title.
You need to log in before you can comment on or make changes to this bug.