Last Comment Bug 900971 - nssutil_ReadSecmodDB() leaks memory
: nssutil_ReadSecmodDB() leaks memory
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.15
: All All
: P2 normal (vote)
: 3.15.2
Assigned To: Elio Maldonado
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-08-02 07:59 PDT by Kai Engert (:kaie)
Modified: 2014-04-21 16:22 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 - made by Elio (443 bytes, patch)
2013-08-02 07:59 PDT, Kai Engert (:kaie)
rrelyea: review+
wtc: review-
Details | Diff | Review
nssutil_growList and nssutil_ReadSecmodDB cleanup (2.41 KB, patch)
2013-09-18 17:53 PDT, Wan-Teh Chang
emaldona: review+
rrelyea: superreview+
Details | Diff | Review

Description Kai Engert (:kaie) 2013-08-02 07:59:34 PDT
Created attachment 785004 [details] [diff] [review]
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 2013-08-02 09:40:39 PDT
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 2013-08-23 07:35:13 PDT
pushed to git: https://hg.mozilla.org/projects/nss/rev/b782ffd7f0fa
Comment 3 Elio Maldonado 2013-08-23 07:36:21 PDT
s/git/hg/
Comment 4 Wan-Teh Chang 2013-09-18 17:47:57 PDT
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 2013-09-18 17:53:11 PDT
Created attachment 807002 [details] [diff] [review]
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 2013-10-02 15:43:04 PDT
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 2013-10-02 15:54:14 PDT
(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 2014-04-21 16:22:38 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.