679610 - memory leak in crlutil.c

Status: RESOLVED FIXED

(NSS :: Tools, defect)

Description

[David Volgyes]

Attachment: proposed fix.

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:6.0) Gecko/20100101 Firefox/6.0
Build ID: 20110812233755

Steps to reproduce:

cppcheck 1.49 (http://cppcheck.sourceforge.net/) found a resource leak in 
in security/nss/cmd/crlutil/crlutil.c in main()


Actual results:

Memory is allocated with strdup, but it is not released.

          case 't': {
            char *type;
            
            type = strdup(optstate->value);
            crlType = atoi (type);
+           free(type);
            if (crlType != SEC_CRL_TYPE && crlType != SEC_KRL_TYPE) {
                PR_fprintf(PR_STDERR, "%s: invalid crl type\n", progName);
                PL_DestroyOptState(optstate);
                return -1;
            }
            break;



Expected results:

You should free the memory.

Comment 1

[Ed Morley [:emorley]]

Did you mean to set this r?(wtc) ?

Comment 2

[David Volgyes]

(In reply to Ed Morley [:edmorley] from comment #1)
> Did you mean to set this r?(wtc) ?

I do not understand your question.
I meant: you have to insert a free(), because strdup makes an allocation,
and somewhere this memory should be released.

By the way, why did you assigned the task to me? 
I cannot commit to the code, I have no review right, I just was the reporter.
(Actually, I have very limited time, so I hardly could report the bug, and I have no more time to take care of it.)

Comment 3

[Ed Morley [:emorley]]

Sorry, I'll clarify a bit.

By "set this r?(wtc)" I meant did you mean to ask for review (r?) from wtc, who does NSS reviews.

(In reply to David Volgyes from comment #2)
> I cannot commit to the code, I have no review right, I just was the reporter.
> (Actually, I have very limited time, so I hardly could report the bug, and I
> have no more time to take care of it.)

On bugzilla, the assignee field is used for whomever is working on the bug, which is normally the patch submitter. Lots of people forget to change the assignee when working on a bug, which means that people searching for un-owned bugs to work on can't easily exclude those bugs. The person who is assigned the bug does not need to review the patches, nor is commit access necessary, since they can ask someone else to review/commit.

However in this case, if you uploaded the patch just as a help for someone else to work on it - then I'll unassign from you. Do you wish me to do the same for all the others?

Either way - thanks for filing these bugs and providing example fixes, it's really appreciated. The important thing now is to make sure that other people don't think you are working on the bugs and the patches sit here untouched - so I'll likely add a whiteboard notice to all your bugs explaining.

Comment 4

[David Volgyes]

> Either way - thanks for filing these bugs and providing example fixes, it's
> really appreciated. The important thing now is to make sure that other
> people don't think you are working on the bugs and the patches sit here
> untouched - so I'll likely add a whiteboard notice to all your bugs
> explaining.

That would be great. Thanks.

Comment 5

[Torbjörn Andersson]

Maybe I'm missing something terribly obvious, but does it even have to duplicate the string at all? As far as I can see, all it does with the copy is to pass it to atoi() and in that case it could just as well use the original string, couldn't it?

Comment 6

[(no longer active)]

I believe that this code is a tool which runs during our build process and it's not in code shipped inside of Firefox, so it is outside of the scope of the MemShrink project.

Comment 7

[:aceman]

Attachment: patch per comment 5

Comment 8

[Wan-Teh Chang]

Comment on attachment 589663 [details] [diff] [review]
patch per comment 5

r=wtc.

I wonder if we should check for a null pointer as we do in
case 'r' above.

Comment 9

[:aceman]

Print that "Invalid crl" in that case?

Comment 10

[Kai Engert [:KaiE:]]

Checking in security/nss/cmd/crlutil/crlutil.c;
/cvsroot/mozilla/security/nss/cmd/crlutil/crlutil.c,v  <--  crlutil.c
new revision: 1.35; previous revision: 1.34
done