Closed Bug 560557 Opened 15 years ago Closed 15 years ago

js_SetReservedSlot is too friendly to api abusers

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: timeless, Assigned: timeless)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

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.
Attached patch proposalSplinter Review
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+
http://hg.mozilla.org/tracemonkey/rev/b27741421347 is followup to make limit a debug only variable
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Blocks: 564763
Blocks: 568007
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: