679614 - memory leak in symkeyutil.c

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[David Volgyes]

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

Steps to reproduce:

cppcheck found a memory leak in security/nss/cmd/symkeyutil/symkeyutil.c
at line #753.


Actual results:

A string is allocated in line 753:
        certPrefix = strdup(symKeyUtil.options[opt_dbPrefix].arg);
but there are several exist points below, and the memory is not released there.


Expected results:

You should free the memory, when it is not necessary anymore.

Comment 1

[Atul Aggarwal]

Attachment: v1 patch to free char array memory

Wan-Teh, please assign the review to another person if you are busy.

Comment 2

[(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 3

[Atul Aggarwal]

Remainder for code review.

Comment 4

[Kai Engert (:KaiE:)]

Comment on attachment 556400 [details] [diff] [review]
v1 patch to free char array memory

r-

First, this code isn't in a library, it's in a little utility that will only ever run for a very short amount of time, therefore the programmer probably didn't worry too much about leaks.

Therefore I think the approach to add widespread cleanup code is extreme.

We should find a simpler solution because certPrefix is only used once.

I believe the original programmer simply wanted a "shortcut variable name", instead of having to type that long statement twice.

As we can see in the code, arguments like "symKeyUtil.options[opt_TokenName].arg" are used all over the file, so it's not strictly necessary to duplicate that string.

I suspect the programmer was playing safe, because the argument certPrefix is passed twice to the init function, and he was worried that something might go wrong.

But as far as I can tell, the init function will not keep the string. It will simply use it for the durating of the function call, and then it's no longer required.

Ok, here is my proposal for a better change:

- remove the call to strdup, in other words change
     certPrefix = strdup(symKeyUtil.options[opt_dbPrefix].arg);
  to
     certPrefix = symKeyUtil.options[opt_dbPrefix].arg;

  (this simply produce an alias pointer, without allocation)

- and keep everything else in its original state

Comment 5

[Atul Aggarwal]

Attachment: Patch v1.1

Totally agreed. Fixing as suggested.

Comment 6

[Kai Engert (:KaiE:)]

Comment on attachment 606560 [details] [diff] [review]
Patch v1.1

r=kaie

Comment 7

[Kai Engert (:KaiE:)]

Checking in security/nss/cmd/symkeyutil/symkeyutil.c;
/cvsroot/mozilla/security/nss/cmd/symkeyutil/symkeyutil.c,v  <--  symkeyutil.c
new revision: 1.14; previous revision: 1.13
done