603376 - obj_toSource leaks ida when js_NewString fails

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[timeless]

473 obj_toSource(JSContext *cx, uintN argc, Value *vp)
478     JSIdArray *ida;
498     if (!obj || !(he = js_EnterSharpObject(cx, obj, &ida, &chars))) {
501     }
502     if (IS_SHARP(he)) {
517         goto make_string;

this cleanup code is skipped:
783  	  error:
784  	    js_LeaveSharpObject(cx, &ida);

787   make_string:
788     str = js_NewString(cx, chars, nchars);
789     if (!str) {
790         js_free(chars);
791         ok = JS_FALSE;
792         goto out;

796   out:
797     return ok;

Comment 1

[timeless]

ok, trying to trace this further, i've convinced myself coverity got confused.

js_EnterSharpObject can return an he which IS_SHARP(), when IS_SHARP() is true, that means map->depth != 0 so MarkSharpObjects (which creates ida/idap) is skipped; JS_HashTableRawLookup returned hep/he, thus JS_HashTableRawAdd was skipped; and the JS_Enumerate in out: was skipped.

I have a feeling I've mistraced this code before (probably 5-7 years ago at a previous employer).

Comment 2

[Brendan Eich [:brendan]]

Code's too complicated -- could reopen for a patch to simplify it till coverity (and sixgill?) like it.

/be

Comment 3

[timeless]

I don't think I have the energy to do it, sorry. I have a manager (not mine, it's an indirect request) who wants me to get rid of 400 Coverity issues (I haven't even figured out where his list of 400 is, there are something like 1800 unreviewed issues for our Gecko and I'm the only person looking).

Unless it's as simple as adding a direct lookup to obj_source between null checking obj and js_EnterSharpObject to special case the this edge case. If it's that simple, I could probably do it. It wouldn't make the code much simpler, but it should quiet Coverity and be a bit easier for people to read (maybe).