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

RESOLVED FIXED in 3.12

Status

NSS
Tools
P3
normal
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

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.
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.
Attachment #236799 - Flags: superreview?(alexei.volkov.bugs)
Attachment #236799 - Flags: review?(nelson)

Comment 2

11 years ago
I'm just curious, but do we (Firefox, Thunderbird, etc.) even use this code?
AFAIK, libJAR is used only in NSS test tools, such as signtool and perhaps 
one other, not in mozilla browsers or email clients.
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.
Attachment #236799 - Flags: review?(nelson) → review-
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.
Attachment #236799 - Attachment is obsolete: true
Attachment #236918 - Flags: superreview?(alexei.volkov.bugs)
Attachment #236918 - Flags: review?(nelson)
Attachment #236799 - Flags: superreview?(alexei.volkov.bugs)
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.
Attachment #236918 - Flags: review?(nelson) → review-
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?
Attachment #236918 - Attachment is obsolete: true
Attachment #236923 - Flags: superreview?(alexei.volkov.bugs)
Attachment #236923 - Flags: review?(nelson)
Attachment #236918 - Flags: superreview?(alexei.volkov.bugs)
Comment on attachment 236923 [details] [diff] [review]
Patch for memory leaks in jarjart.c (revised)

r=nelsonb
Attachment #236923 - Flags: review?(nelson) → review+
Status: NEW → ASSIGNED

Updated

11 years ago
Attachment #236923 - Flags: superreview?(alexei.volkov.bugs) → superreview+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Priority: -- → P3
Resolution: --- → FIXED
Target Milestone: --- → 3.12
Version: trunk → 3.11
Keywords: qawanted
Keywords: qawanted
Keywords: verifyme
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.