Last Comment Bug 351408 - Leaks in JAR_JAR_sign_archive (security/nss/lib/jar/jarjart.c)
: Leaks in JAR_JAR_sign_archive (security/nss/lib/jar/jarjart.c)
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Tools (show other bugs)
: 3.11
: All All
: P3 normal (vote)
: 3.12
Assigned To: :Ehsan Akhgari
:
Mentors:
http://bonsai.mozilla.org/cvsblame.cg...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-09-05 06:13 PDT by :Ehsan Akhgari
Modified: 2009-02-01 00:32 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch for memory leaks in jarjart.c (1.15 KB, patch)
2006-09-05 06:16 PDT, :Ehsan Akhgari
nelson: review-
Details | Diff | Splinter Review
Patch for memory leaks in jarjart.c (revised) (1.38 KB, patch)
2006-09-05 22:11 PDT, :Ehsan Akhgari
nelson: review-
Details | Diff | Splinter Review
Patch for memory leaks in jarjart.c (revised) (1.31 KB, patch)
2006-09-05 22:43 PDT, :Ehsan Akhgari
nelson: review+
alvolkov.bgs: superreview+
Details | Diff | Splinter Review

Description :Ehsan Akhgari 2006-09-05 06:13:29 PDT
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 :Ehsan Akhgari 2006-09-05 06:16:53 PDT
Created attachment 236799 [details] [diff] [review]
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 2006-09-05 19:12:16 PDT
I'm just curious, but do we (Firefox, Thunderbird, etc.) even use this code?
Comment 3 Nelson Bolyard (seldom reads bugmail) 2006-09-05 19:24:48 PDT
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) 2006-09-05 19:32:41 PDT
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 :Ehsan Akhgari 2006-09-05 22:11:27 PDT
Created attachment 236918 [details] [diff] [review]
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) 2006-09-05 22:34:23 PDT
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 :Ehsan Akhgari 2006-09-05 22:43:35 PDT
Created attachment 236923 [details] [diff] [review]
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) 2006-09-05 22:53:36 PDT
Comment on attachment 236923 [details] [diff] [review]
Patch for memory leaks in jarjart.c (revised)

r=nelsonb
Comment 9 Nelson Bolyard (seldom reads bugmail) 2006-09-25 12:22:57 PDT
Fix leaks in jarfile.c (bug 338453), jarjart.c (bug 351408), and
jarver.c (bug 337361). Patch contributed by ehsan.akhgari@gmail.com

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

Note You need to log in before you can comment on or make changes to this bug.