560557 - js_SetReservedSlot is too friendly to api abusers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[timeless]

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.

Comment 1

[timeless]

Attachment: proposal

Comment 2

[Jason Orendorff [: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.

Comment 3

[Jason Orendorff [:jorendorff]]

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.

Comment 4

[Brendan Eich [:brendan]]

I did this out of paranoia over our own internal uses, yes. These seem debugged now. Let's just assert as usual.

/be

Comment 5

[Jason Orendorff [:jorendorff]]

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

I'll push this with my next batch of patches.

Comment 6

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/bed748189cd0

Comment 7

[timeless]

http://hg.mozilla.org/tracemonkey/rev/b27741421347

is followup to make limit a debug only variable

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/bed748189cd0