77869 - nss tools should call PL_DestroyOptState to free optstate

Status: ASSIGNED

(NSS :: Tools, defect, P5)

Description

[zhoubin]

Comment 1

[Wan-Teh Chang]

Thank you very much for the bug report.  Yes, these are
memory leaks.  These one-time memory leaks are not serious
and it is not urgent to fix them, so I am setting  the
priority to P5 and target milestone to Future.

If you have patches for these tools, we will be happy to
review them.

Comment 2

[Wan-Teh Chang]

Changed the QA contact to Bishakha.

Comment 3

[Biswatosh Chakraborty]

Attachment: Calls PL_DestroyOptState whereever required in nss/cmd tools

It touches files in nss/cmd only as this problem is not applicable
in others.

In some files, PL_DestroyOptState() was called after the while
loop but then 
not called in the case of error conditions inside the while loop, such as
inside :
  while ((optstatus = PL_GetNextOpt(optstate)) == PL_OPT_OK) {...}
I added the call in those places as well.
Wherever, after such a while loop, I found this call missing, I added 
it there too.

Comment 4

[Julien Pierre]

Comment on attachment 278036 [details] [diff] [review]
Calls PL_DestroyOptState whereever required in nss/cmd tools

The patch may be correct, but this code is not maintainable. Repeating the destroy call so many times in so many different places is going to lead to errors when new options are added and the cleanup is forgotten.

It would be much better to do the cleanup in one place, and have goto statements in this case. The error paths can set a variable before the goto to cause the function to exit after the destroy.

Please rewrite the patch with at most one PL_DestroyOptState call per PL_CreateOptState call.

Comment 5

[Biswatosh Chakraborty]

Attachment: Patch 2

Julien,

Please see whether this patch is doing what you had suggested in
Comment #4.
Although this bug has priority 5 but it covers majority files under 
nss/cmd for which any mistake in my patch could be very crucial.

I tested with all.sh on Linux machine and it passes all tests 
except ssl.sh successfully everytime. It passes ssl.sh many times
but sometimes stops at some line(in stress tests) and does not
proceed for a long time. But, ssl.sh uses tstclnt, strsclnt, selfserv,
certutil, modutil and crlutil only (directly or indirectly) and my patch
doesnot not change any one of those. I even tried all.sh with original
library(unchanged) and a couple of times it fails because of (PR_Bind,
"Address in use") issue or it stops for quite some time in stress tests.
Hence, I think, it is safe enough to give it for review now.

Comment 6

[Julien Pierre]

Biswatosh,

The bind issue is a known pre-existing bug, bug 348198 . Your patch probably didn't affect it. I will take a look at it.

Comment 7

[Julien Pierre]

Comment on attachment 288451 [details] [diff] [review]
Patch 2

I am afraid this patch is bitrotten, so I'm cancelling the review request.