nssutil_ReadSecmodDB() leaks memory

RESOLVED FIXED in 3.15.2

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: kaie, Assigned: Elio Maldonado)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
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.
Attachment #785004 - Flags: review?(rrelyea)

Comment 1

4 years ago
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
Attachment #785004 - Flags: review?(rrelyea) → review+
(Reporter)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 2

4 years ago
pushed to git: https://hg.mozilla.org/projects/nss/rev/b782ffd7f0fa
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 3

4 years ago
s/git/hg/
Keywords: checkin-needed

Updated

4 years ago
Assignee: nobody → emaldona
Priority: -- → P2
Target Milestone: --- → 3.15.2

Comment 4

4 years ago
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.
Attachment #785004 - Flags: review-

Comment 5

4 years ago
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.
Attachment #807002 - Flags: superreview?(rrelyea)
Attachment #807002 - Flags: review?(emaldona)
(Assignee)

Comment 6

4 years ago
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.
Attachment #807002 - Flags: review?(emaldona) → review+
(Assignee)

Comment 7

4 years ago
(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

3 years ago
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.
Attachment #807002 - Flags: superreview?(rrelyea) → superreview+
You need to log in before you can comment on or make changes to this bug.