351408 - Leaks in JAR_JAR_sign_archive (security/nss/lib/jar/jarjart.c)

Status: RESOLVED FIXED

(NSS :: Tools, defect, P3)

Description

[(no longer active)]

Please refer to the sample URL.  The memory allocated at line 335 will leak if the allocation succeeds, but the length of sf is less than 5.  Also, the calls to JAR_open_database()/JAR_close_database() and jar_open_key_database()/jar_close_key_database() are not matched.

Comment 1

[(no longer active)]

Attachment: Patch for memory leaks in jarjart.c

This patch fixes the leak problem, and also matches the calls to open/close functions.

Comment 2

[Adam Guthrie]

I'm just curious, but do we (Firefox, Thunderbird, etc.) even use this code?

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

AFAIK, libJAR is used only in NSS test tools, such as signtool and perhaps 
one other, not in mozilla browsers or email clients.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 236799 [details] [diff] [review]
Patch for memory leaks in jarjart.c


> 
>   out_fn = PORT_Strdup (sf);
> 
>-  if (out_fn == NULL || PORT_Strlen (sf) < 5)

Variable out_fn is never used below here (that I can see).
This patch only freed it in the error paths, not in the main path.  
IMO, it should be eliminated entirely.

Otherwise this patch seemed ok.

Comment 5

[(no longer active)]

Attachment: Patch for memory leaks in jarjart.c (revised)

(In reply to comment #4)
> >   out_fn = PORT_Strdup (sf);
> > 
> >-  if (out_fn == NULL || PORT_Strlen (sf) < 5)
> 
> Variable out_fn is never used below here (that I can see).
> This patch only freed it in the error paths, not in the main path.  
> IMO, it should be eliminated entirely.

Yes, I think you're right.  This patch removed the out_fn variable.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 236918 [details] [diff] [review]
Patch for memory leaks in jarjart.c (revised)

One more observation: 
If you move the input string length check up to the beginning of the functino, before opening the databases, then you won't have to close them when it's bad.

Comment 7

[(no longer active)]

Attachment: Patch for memory leaks in jarjart.c (revised)

(In reply to comment #6)
> (From update of attachment 236918 [details] [diff] [review] [edit])
> One more observation: 
> If you move the input string length check up to the beginning of the functino,
> before opening the databases, then you won't have to close them when it's bad.

OK.  How about this one?

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 236923 [details] [diff] [review]
Patch for memory leaks in jarjart.c (revised)

r=nelsonb

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Fix leaks in jarfile.c (bug 338453), jarjart.c (bug 351408), and
jarver.c (bug 337361). Patch contributed by [e-mail]

Checking in jarfile.c; new revision: 1.6; previous revision: 1.5
Checking in jarjart.c; new revision: 1.7; previous revision: 1.6
Checking in jarver.c;  new revision: 1.12; previous revision: 1.11