All users were logged out of Bugzilla on October 13th, 2018

js_SetReservedSlot is too friendly to api abusers

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: timeless, Assigned: timeless)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Assignee)

Description

9 years ago
6462 js_SetReservedSlot(JSContext *cx, JSObject *obj, uint32 index, jsval v)

here we seem to be coddling api abusers:
6468     uint32 limit = JSCLASS_RESERVED_SLOTS(clasp);
6471     if (index >= limit && !ReservedSlotIndexOK(cx, obj, clasp, index, limit))
6472         return false;

here we seem to be fatal to api abusers:
6481         uint32 nslots = JSSLOT_FREE(clasp);
6482         if (clasp->reserveSlots)
6483             nslots += clasp->reserveSlots(cx, obj);
6484         JS_ASSERT(slot < nslots);

this is an oom case, which we have to live with:
6485         if (!js_AllocSlots(cx, obj, nslots)) {
6486             JS_UNLOCK_OBJ(cx, obj);
6487             return false;

I think the first case should also be fatal.
(Assignee)

Comment 1

9 years ago
Created attachment 440239 [details] [diff] [review]
proposal
Assignee: general → timeless
Status: NEW → ASSIGNED
Attachment #440239 - Flags: review?(jorendorff)
Some background: timeless has been running Coverity against SpiderMonkey and feeding back patches for the warnings. We have some internal functions that ignore the return value of JS_SetReservedSlot, knowing that it's infallible in certain precariously constrained circumstances (dependent on the value of JS_INITIAL_NSLOTS even). Coverity issued a warning because it couldn't figure out why we think it's safe; and who can blame it, really.

However, setting aside that warning entirely, there's the question of how this function should behave when the index is out of range. Hence this bug.

I agree with comment 0 that we should tighten up the API and assert rather than report an error and return false. OTOH this is exactly the sort of seemingly harmless, vaguely-motivated change to a longstanding API that timeless has had occasion to complain bitterly about from time to time.

Brendan, if you think the API change is OK, I can vouch for the patch's correctness (it's trivial). I think it should be fine. Calling JS_SetReservedSlot without being certain the index is in range seems like crazy talk.
In fact we're probably more likely to hit this assertion due to a bad bug in SpiderMonkey than for any other reason, our internal reserved-slot uses being far more complicated than those of any embedding I've seen.
I did this out of paranoia over our own internal uses, yes. These seem debugged now. Let's just assert as usual.

/be
Comment on attachment 440239 [details] [diff] [review]
proposal

I'll push this with my next batch of patches.
Attachment #440239 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 7

9 years ago
http://hg.mozilla.org/tracemonkey/rev/b27741421347

is followup to make limit a debug only variable

Comment 8

9 years ago
http://hg.mozilla.org/mozilla-central/rev/bed748189cd0
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Blocks: 564763
(Assignee)

Updated

9 years ago
Blocks: 568007
You need to log in before you can comment on or make changes to this bug.