900971 - nssutil_ReadSecmodDB() leaks memory

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Kai Engert [:KaiE:]]

Attachment: patch v1 - made by Elio

This memory leak has been reported on RHEL.

The patch has already been reviewed by Bob, but I'll ask him again here, to let him confirm that it should be added here.

Comment 1

[Robert Relyea]

Comment on attachment 785004 [details] [diff] [review]
patch v1 - made by Elio

r+ I thought there was already an upstream bug for this, but just in case let's get this in. Thanks for getting on this Kai.

bob

Comment 2

[Elio Maldonado]

pushed to git: https://hg.mozilla.org/projects/nss/rev/b782ffd7f0fa

Comment 3

[Elio Maldonado]

s/git/hg/

Comment 4

[Wan-Teh Chang]

Comment on attachment 785004 [details] [diff] [review]
patch v1 - made by Elio

Review of attachment 785004 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozilla/security/nss/lib/util/utilmod.c.975755
@@ +358,4 @@
>  	status = PR_Access(olddbname, PR_ACCESS_EXISTS);
>  	if (status == PR_SUCCESS) {
>  	    PR_smprintf_free(olddbname);
> +	    PORT_ZFree(moduleList, useCount*sizeof(char **));

Elio, Bob:

1. This doesn't need to be PORT_ZFree. PORT_Free
should suffice, right?

moduleList points to an array of pointers. Are you
trying to avoid dangling pointers?

2. Each pointer in the moduleList array points to a
string that should also be freed. So I am afraid that
this memory leak fix is incomplete. I think we should
call nssutil_releaseSpecList(moduleList) instead.

Comment 5

[Wan-Teh Chang]

Attachment: nssutil_growList and nssutil_ReadSecmodDB cleanup

nssutil_growList should only update *useCount when it succeeds.

nssutil_ReadSecmodDB should use sizeof(char *) instead of sizeof(char **)
when allocating moduleList because moduleList is an array of char *
pointers. This is just a conceptual error because all pointer types have
the same size.

Comment 6

[Elio Maldonado]

Comment on attachment 807002 [details] [diff] [review]
nssutil_growList and nssutil_ReadSecmodDB cleanup

Review of attachment 807002 [details] [diff] [review]:
-----------------------------------------------------------------

r+ plus from me for this patch as it was submitted.

::: lib/util/utilmod.c
@@ +182,5 @@
>      char *moduleString = NULL;
>      char *paramsValue=NULL;
>      PRBool failed = PR_TRUE;
>  
> +    moduleList = (char **) PORT_ZAlloc(useCount*sizeof(char *));

Yes, 'char *' is the right one to use.

@@ +330,5 @@
>  	    }
>  	}
>  
>  	if (internal) {
> +	    /* XXX should we free moduleList[0] first? */

No, we don't have to free as moduleList[0] is NULL because moduleList was first allocated on 
Line 186 via moduleList = (char **) PORT_ZAlloc(useCount*sizeof(char *)); and 
though on Line 327 it grew via 
rv = nssutil_growList(&moduleList, &useCount,  moduleCount+1);
that didn't change the value moduleList[0], it just zeroized the new array slots.

@@ +360,5 @@
>  	/* old one exists */
>  	status = PR_Access(olddbname, PR_ACCESS_EXISTS);
>  	if (status == PR_SUCCESS) {
>  	    PR_smprintf_free(olddbname);
> +	    PORT_ZFree(moduleList, useCount*sizeof(char *));

Yes.

Comment 7

[Elio Maldonado]

(In reply to Wan-Teh Chang from comment #4)
> Comment on attachment 785004 [details] [diff] [review]
> patch v1 - made by Elio
> 
> Review of attachment 785004 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mozilla/security/nss/lib/util/utilmod.c.975755
> @@ +358,4 @@
> >  	status = PR_Access(olddbname, PR_ACCESS_EXISTS);
> >  	if (status == PR_SUCCESS) {
> >  	    PR_smprintf_free(olddbname);
> > +	    PORT_ZFree(moduleList, useCount*sizeof(char **));
> 
> Elio, Bob:
> 
> 1. This doesn't need to be PORT_ZFree. PORT_Free
> should suffice, right?
> 
> moduleList points to an array of pointers. Are you
> trying to avoid dangling pointers?

I don't thing that was the intent but I shouldn't try to speak for Bob :-)
> 
> 2. Each pointer in the moduleList array points to a
> string that should also be freed. So I am afraid that
> this memory leak fix is incomplete. I think we should
> call nssutil_releaseSpecList(moduleList) instead.

nssutil_releaseSpecList(moduleList) would guarantee that moduleList[moduleCount] would get freed
336	} else {
337	    moduleList[moduleCount] = moduleString;
	    moduleCount++;
	}
which got allocated from a string dup and cat on
316	moduleString = nssutil_DupnCat(moduleString," parameters=", 12);
	if (moduleString == NULL) goto loser;
318	moduleString = nssutil_DupCat(moduleString, paramsValue);

Comment 8

[Robert Relyea]

Comment on attachment 807002 [details] [diff] [review]
nssutil_growList and nssutil_ReadSecmodDB cleanup

Review of attachment 807002 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/util/utilmod.c
@@ +104,5 @@
>  	return SECFailure;
>      }
>      PORT_Memset(&newModuleList[last],0, sizeof(char *)*SECMOD_STEP);
>      *pModuleList = newModuleList;
> +    *useCount = newUseCount;

yeah, we probably shouldn't modify useCount if we couldn't actually get the memory;).

bob

@@ +182,5 @@
>      char *moduleString = NULL;
>      char *paramsValue=NULL;
>      PRBool failed = PR_TRUE;
>  
> +    moduleList = (char **) PORT_ZAlloc(useCount*sizeof(char *));

Fortunately char** and char* are the same size on any platform NSS runs on.

PORT_New would have caught this error...

@@ +330,5 @@
>  	    }
>  	}
>  
>  	if (internal) {
> +	    /* XXX should we free moduleList[0] first? */

Perhaps we should verify that moduleList[0] is NULL in case we have more than one 'internal' module. I'd have to look to see if internal can be set more than once.