431929 - Memory leaks on error paths in devutil.c

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

Attachment: Proposed patch

By code inspection, I found some memory leaks on error paths
in devutil.c  -- the attempt to free a partially constructed token
cache is not complete.

Also, the objects in the token cache have a pointer to the token,
but the pointer is not a reference.  So when the objects in the
cache are destroyed, we need to prevent the token's reference
count from being decremented.  This file uses two methods to
do that.  One is to set object->token to NULL before destroying
the object.  The other is to call nssToken_AddRef(object->token)
before destroying the object.  This patch also changes the file
to use the first method consistently.  If you don't have time to
review this part of the patch (it may take some time to understand
it), I can omit it.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 319082 [details] [diff] [review]
Proposed patch

Wan-Teh, The remarks about your patch in comment 0 are addressed to "you",
and the patch has no review request.  For whom were those remarks intended?

I studied the code that you patched, and have lots of comments about that
code.  (This is a review of that code, not of your patch.)

1) There is a large body of code duplicated entirely in functions 
get_token_certs_for_cache (lines 542-570) and 
get_token_trust_for_cache (lines 619-647) and 
get_token_crls_for_cache  (lines 692-720).  It just cries out to be turned 
into a common subroutine (or set of common subroutines).  Those differ only
by two values that could be passed to the function as arguments, which are:
- the index to the array cache->objects[], 
   cachedTrust or cachedCerts or cachedCRLs.
- the function called in the loop that creates an array of 
   nssCryptokiObjectAndAttributes from the corresponding members of the 
   array of objects, create_cert() or create_object_array() or create_crl().

2) The function names and structure member variables names are confusing
and non-descriptive.  All 3 functions start by getting an array of 
objects, the array pointer is named "objects" (that's good) and the type
is nssCryptokiObject**.  So far so good.  Then, all 3 functions attempt
to create a corresponding array of things of type 
nssCryptokiObjectAndAttributes.  

In some code, (not in these functions) the variable that points to those 
things is named rvOandA, which seems descriptive, but in these functions 
the array of pointers to those things is not named cache->OandAs, but 
rather cache->objects.  This is confusing, because we're starting with 
objects and we're creating something different, but the something 
different is also called "objects".  

There is a function that allocates this array of OandAs, and its name is
create_object_array, which is simply WRONG.

It would be good to use variables and functions whose names more 
accurately reflect the types they deal with.

3) This code really violates object boundaries.  It has constructors for
OandA's, but not destructors.  The code to destroy OandA's and the code to 
destroy objects are mixed together in the same loop.  There really should
be a destructor function for each.  

4) About this patch, Even after reviewing the patched code for a while,
it's not yet entirely obvious that the value of 
cache->objects[cachedCerts][j]->object->token is the same as the value
of objects[j]->token.  Assuming that is true, it's not yet clear to me 
which of those two values is the one that should be set to NULL.

5) Finally, while studying this code, I found a bug.  I suppose there is 
a Coverity bug about this filed somewhere, because it's the kind of thing
that I'd expect Coverity to report.  In lib/base/arena.c we see

984     PR_Lock(h->arena->lock);
985     if( (PRLock *)NULL == h->arena->lock ) {

Clearly, if that if statement is true, the code will have crashed before
reaching line 985.

Comment 2

[Wan-Teh Chang]

Attachment: Call PR_Lock after null checks

Nelson, I didn't request a review for my previous patch
because it is low priority and understanding the patch
would take a long time.  I submitted the patch merely
for archival purposes.

I found these problems while investigating bug 431628.
I needed to verify it's safe to call nssSlot_Destroy(token->slot)
inside nssToken_Destroy() by inspecting all callers of
nssToken_Destroy().  As you have found, I quickly realized
that this code has many minor issues, so I decided to
backtrack and just fix the simple ones.  Still, they're
simple only after browsing the source code for half an
hour, so I didn't want to waste any of your time reviewing
the patch.

Here is a patch for the PR_Lock problem you found (and
two others I found while fixing it) in lib/base/arena.c.

Comment 3

[Wan-Teh Chang]

(In reply to comment #1, question #4)
The key is this statement:
    cache->objects[cachedCerts][i] = create_cert(objects[i], &status);

This implies that
    cache->objects[cachedCerts][i]->object == objects[i]

Note: this is stronger than
    cache->objects[cachedCerts][i]->object->token == objects[j]->token

So we can rewrite
    nssToken_AddRef(cache->objects[cachedTrust][j]->object->token);
as
    nssToken_AddRef(objects[j]->token);

Now, the purpose of this nssToken_AddRef call is to neutralize the
nssToken_Destroy calls in
    nssCryptokiObjectArray_Destroy(objects);

We can suppress the nssToken_Destroy calls by
    objects[j]->token = NULL;
which is the method used elsewhere in the file.
    

Comment 4

[Wan-Teh Chang]

Attachment: Refectoring patch

This patch refactors the code that Nelson said in comment 1
"cries out to be turned into a common subroutine".  This patch
doesn't fix any leaks or bugs.

1. Replace nssToken_Find{Certificates,TrustObjects,CRLs} by
nssToken_FindObjects.

The key to reviewing this change is to verify that it is okay
to omit this statement in nssToken_Find{TrustObjects,CRLs}:
    nssSession *session = sessionOpt ? sessionOpt : token->defaultSession;
because the sessionOpt argument is ultimately passed to find_objects
and find_objects replaces a null sessionOpt with token->defaultSession,
too.

2. Replace get_token_{certs,trust,crls}_for_cache by
get_token_objects_for_cache.

I hope this patch will make NSS a little smaller.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 320333 [details] [diff] [review]
Call PR_Lock after null checks

r=nelson
Good catches!

Comment 6

[Wan-Teh Chang]

Comment on attachment 320333 [details] [diff] [review]
Call PR_Lock after null checks

I checked in this patch on the NSS trunk (NSS 3.12.1).

Checking in arena.c;
/cvsroot/mozilla/security/nss/lib/base/arena.c,v  <--  arena.c
new revision: 1.12; previous revision: 1.11
done

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 320383 [details] [diff] [review]
Refectoring patch

THis patch doesn't address the issue that, in a single function, some 
variables named "objects" are arrays of one type, and others are arrays
of variables of another type.  But it definitely addresses the code 
duplication, so r++.  :)

Here's one suggested tiny improvement.  The patch changes a certain
section of code to look like this:

>+    PZ_Lock(cache->lock);
>+    if (cache->doObjectType[objectType]) {
>+	status = get_token_objects_for_cache(cache, objectType, objclass);
>+	if (status != PR_SUCCESS) {
>+	    goto unlock;
> 	}
>+	rvObjects = find_objects_in_array(cache->objects[objectType], 
>+	                                  otemplate, otlen, maximumOpt);
>     }
> unlock:
>     PZ_Unlock(cache->lock);

I'd suggest inverting the sense of the status test, and eliminating the 
goto, e.g. 

>+    PZ_Lock(cache->lock);
>+    if (cache->doObjectType[objectType]) {
>+	status = get_token_objects_for_cache(cache, objectType, objclass);
>+	if (status == PR_SUCCESS) {
>+	    rvObjects = find_objects_in_array(cache->objects[objectType], 
>+	                                      otemplate, otlen, maximumOpt);
> 	}
>     }
> unlock:
>     PZ_Unlock(cache->lock);

Comment 8

[Wan-Teh Chang]

Attachment: Refactoring patch (as checked in)

Just writing this patch took three hours of a Saturday morning.
(Because we don't have unit tests for these functions, I had
to refactor the code carefully, in a series of mechanical
code transformations.)  So I only fixed what I think is the most
serious problem.

I checked in the patch on the NSS trunk (NSS 3.12.1) with the
change you suggested.

Checking in dev/dev.h;
/cvsroot/mozilla/security/nss/lib/dev/dev.h,v  <--  dev.h
new revision: 1.40; previous revision: 1.39
done
Checking in dev/devtoken.c;
/cvsroot/mozilla/security/nss/lib/dev/devtoken.c,v  <--  devtoken.c
new revision: 1.47; previous revision: 1.46
done
Checking in dev/devutil.c;
/cvsroot/mozilla/security/nss/lib/dev/devutil.c,v  <--  devutil.c
new revision: 1.30; previous revision: 1.29
done
Checking in pk11wrap/pk11cert.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.165; previous revision: 1.164
done

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Thanks, Wan-Teh, it's a big improvement.  I appreciate it!

Comment 10

[Wan-Teh Chang]

Attachment: Proposed patch v2

To reduce code review burden, I give up on one of the
changes and just fix the memory leaks.

The way to understand this patch is to see that the
block of code I modified is undoing the effects of the
code above it.  So this for loop:

        PRUint32 j;
        for (j=0; j<i; j++) {
            /* sigh */
            nssToken_AddRef(cache->objects[objectType][j]->object->token);
            nssArena_Destroy(cache->objects[objectType][j]->arena);
        }

is undoing this for loop:

    for (i=0; i<numObjects; i++) {
        cache->objects[objectType][i] = create_object_of_type(objects[i],
                                                              objectType,
                                                              &status);
        if (status != PR_SUCCESS) {
            break;
        }
    }

The two lines I added:

+       nss_ZFreeIf(cache->objects[objectType]);
+       cache->objects[objectType] = NULL;

are undoing this:

    cache->objects[objectType] = create_object_array(objects,
                                                     doIt,
                                                     &numObjects,
                                                     &status);

And finally this:

        nssCryptokiObjectArray_Destroy(objects);

is undoing this:

    objects = nssToken_FindObjects(cache->token, NULL, objclass,
                                   nssTokenSearchType_TokenForced,
                                   MAX_LOCAL_CACHE_OBJECTS, &status);

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, when I reviewed the "refactoring" patch, I didn't check it to see
if it plugged any leak.  I assumed that it did.  Does the presence of this
additional patch imply that the refactoring patch did not affect the leak 
that is the subject of this bug?

Comment 12

[Wan-Teh Chang]

Right.  The refactoring patch does nothing but refactoring.
It doesn't fix any bugs or plug any leaks.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 321355 [details] [diff] [review]
Proposed patch v2

r=nelson

Comment 14

[Wan-Teh Chang]

I checked in proposed patch v2 (attachment 321355 [details] [diff] [review]) on the
NSS trunk (NSS 3.12.1).

Checking in devutil.c;
/cvsroot/mozilla/security/nss/lib/dev/devutil.c,v  <--  devutil.c
new revision: 1.31; previous revision: 1.30
done