505819 - find_objects_in_array leaks objects if nssCryptokiObject_Clone fails

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[timeless]

note that objects is essentially its own arena because the first argument to nss_ZNEWARRAY is null:
725         objects = nss_ZNEWARRAY(NULL, nssCryptokiObject *, numMatches + 1);

729 for (oi=0; oi<(PRIntn)numMatches; oi++) { 730 objects[oi] = nssCryptokiObject_Clone(matches[oi]->object); 731 if (!objects[oi]) {
something failed, go to loser:
732 goto loser;

if we don't fail, we free the other arena:
736     nssArena_Destroy(arena);
and return objects:
737     return objects;
738 loser:
but, we're loser:
739     if (objects) {
740         for (--oi; oi>=0; --oi) {
we release the things we managed to clone
741             nssCryptokiObject_Destroy(objects[oi]);
742         }
but don't do anything about objects
743     }
we destroy the other arena
744     nssArena_Destroy(arena);
and leak objects:
745     return (nssCryptokiObject **)NULL;

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Yup.  The objects pointed to by the array get freed, but not the array itself.
loser needs to free the objects array.

Comment 2

[Shailen]

Attachment: Patch V 1

Hi Nelson,

     Please review the patch.

Thanks,
Shailendra

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 437013 [details] [diff] [review]
Patch V 1

Shailendra,  The if test and the for loop shown below are 
also part of nssCryptokiObjectArray_Destroy, so this patch 
effectively causes each object in the objects array to be 
destroyed twice.  It would be better to replace the entire 
body of code shown below with the unconditional call to 
nssCryptokiObjectArray_Destroy.


>     if (objects) {
> 	for (--oi; oi>=0; --oi) {
> 	    nssCryptokiObject_Destroy(objects[oi]);
> 	}
>+	nssCryptokiObjectArray_Destroy(objects);
>     }

Comment 4

[Shailen]

Attachment: Patch Version 2

Nelson,

   Attaching new patch to take care of the suggestions that you made.

Regards,
Shailendra

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 437268 [details] [diff] [review]
Patch Version 2

r=nelson

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Bug 505819: find_objects_in_array leaks objects if nssCryptokiObject_Clone fails
Patch contributed by Shailendra Jain <[e-mail]>

Checking in lib/dev/devutil.c; new revision: 1.34; previous revision: 1.33