Closed Bug 1249641 Opened 4 years ago Closed 4 years ago

[Static Analysis][Resource leak] In function JAR_calculate_digest

Categories

(NSS :: Libraries, defect)

defect
Not set

Tracking

(firefox47 affected)

RESOLVED FIXED
Tracking Status
firefox47 --- affected

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1338033)

Attachments

(1 file, 4 obsolete files)

Variable |dig| will leak the memory to witch it points if the following condition evaluates to true md5 == null:

>>    long chunq;
>>    unsigned int md5_length, sha1_length;
>>
>>    if (dig == NULL) {
>>    /* out of memory allocating digest */
>>    return NULL;
>>    }
>>
>>    md5 = PK11_CreateDigestContext(SEC_OID_MD5);
>>    if (md5 == NULL) {
>>    return NULL;
>>    }
Attachment #8721331 - Flags: review?(brian)
Assignee: nobody → bogdan.postelnicu
Attachment #8721331 - Flags: review?(brian) → review?(ekr)
Comment on attachment 8721331 [details]
MozReview Request: Bug 1249641 - free dig if PK11_CreateDigestContext or PK11_CreateDigestContext fail. r?ekr

https://reviewboard.mozilla.org/r/35643/#review32329

::: security/nss/lib/jar/jarsign.c:54
(Diff revision 1)
> +    PORT_ZFree(dig, sizeof(JAR_Digest));
>  	return NULL;
>      }
>      sha1 = PK11_CreateDigestContext(SEC_OID_SHA1);
>      if (sha1 == NULL) {
>  	PK11_DestroyContext(md5, PR_TRUE);
>  	return NULL;
>      }
>  

Don't you have exactly the same leak in the sha1 branch?
Attachment #8721331 - Flags: review?(ekr)
Comment on attachment 8721331 [details]
MozReview Request: Bug 1249641 - free dig if PK11_CreateDigestContext or PK11_CreateDigestContext fail. r?ekr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35643/diff/1-2/
Attachment #8721331 - Attachment description: MozReview Request: Bug 1249641 - free dig if PK11_CreateDigestContext fails. r?bsmith → MozReview Request: Bug 1249641 - free dig if PK11_CreateDigestContext or PK11_CreateDigestContext fail. r?ekr
Attachment #8721331 - Flags: review?(ekr)
https://reviewboard.mozilla.org/r/35643/#review32329

> Don't you have exactly the same leak in the sha1 branch?

I left this intentiaonally as it was because i'm doing a statistic of Coverity's actual bugs vs false positives vs unidentified in order to know how reliable it's checkers are. The behavior that i expect from Coverity is that even tough it didn't find the 2nd issue in the current test there are chances to detect it in future scans where it considers taking other branches of execution.
In that case, you need to file a bug and add a comment to this code with the bug #
Thanks for looking into coverity issues Bogdan.

Unfortunately we can't use reviewboard for NSS things at the moment. Your patch is against the NSS version in Firefox, which probably differs from the development version of NSS [1]. So it would be great if you can provide patches against [1] so we can land it directly.

[1] https://hg.mozilla.org/projects/nss
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #6)
> Thanks for looking into coverity issues Bogdan.
> 
> Unfortunately we can't use reviewboard for NSS things at the moment. Your
> patch is against the NSS version in Firefox, which probably differs from the
> development version of NSS [1]. So it would be great if you can provide
> patches against [1] so we can land it directly.
> 
> [1] https://hg.mozilla.org/projects/nss

Thanks for letting me know, i didn't know this, i will make the patche agains the version that you mentioned and upload it here, bypassing reviewboard.
Attachment #8721331 - Attachment is obsolete: true
Attachment #8721331 - Flags: review?(ekr)
Attached patch Bug 1249641.diff (obsolete) — Splinter Review
Attachment #8721923 - Flags: review?(franziskuskiefer)
Comment on attachment 8721923 [details] [diff] [review]
Bug 1249641.diff

Review of attachment 8721923 [details] [diff] [review]:
-----------------------------------------------------------------

thanks!

as ekr said, please file a bug and add a comment that there's another leak in the sha1 branch.

::: lib/jar/jarsign.c
@@ +50,5 @@
>      }
>  
>      md5 = PK11_CreateDigestContext(SEC_OID_MD5);
>      if (md5 == NULL) {
> +       PORT_ZFree(dig, sizeof(JAR_Digest));

indentation

@@ +56,5 @@
>      }
>      sha1 = PK11_CreateDigestContext(SEC_OID_SHA1);
>      if (sha1 == NULL) {
>  	PK11_DestroyContext(md5, PR_TRUE);
> +       PORT_ZFree(dig, sizeof(JAR_Digest));

indentation
Attachment #8721923 - Flags: review?(franziskuskiefer)
Blocks: 1250214
Attachment #8722064 - Flags: review?(franziskuskiefer)
Comment on attachment 8722064 [details] [diff] [review]
free dig if PK11_CreateDigestContext or PK11_CreateDigestContext fail

Review of attachment 8722064 [details] [diff] [review]:
-----------------------------------------------------------------

can you mark the other patch as obsolete?

thanks for filing the other bug. A comment in the code with bug 1250214 at the line where that leak is would be nice now.
Attachment #8722064 - Flags: review?(franziskuskiefer)
Attached patch Bug 1249641.diff (obsolete) — Splinter Review
Attachment #8721923 - Attachment is obsolete: true
Attachment #8722064 - Attachment is obsolete: true
Attachment #8722907 - Flags: review?(franziskuskiefer)
Comment on attachment 8722907 [details] [diff] [review]
Bug 1249641.diff

Review of attachment 8722907 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with fixed indent

::: lib/jar/jarsign.c
@@ +50,5 @@
>      }
>  
>      md5 = PK11_CreateDigestContext(SEC_OID_MD5);
>      if (md5 == NULL) {
> +       PORT_ZFree(dig, sizeof(JAR_Digest));

indentation

@@ +56,5 @@
>      }
>      sha1 = PK11_CreateDigestContext(SEC_OID_SHA1);
>      if (sha1 == NULL) {
>  	PK11_DestroyContext(md5, PR_TRUE);
> +       /* added due to bug Bug 1250214 - prevent the 2nd memory leak */

ah, now I understand, I thought you wanted to not fix this one and leave it open. If you fix it here already, we don't need the comment I guess, but it doesn't hurt.

@@ +57,5 @@
>      sha1 = PK11_CreateDigestContext(SEC_OID_SHA1);
>      if (sha1 == NULL) {
>  	PK11_DestroyContext(md5, PR_TRUE);
> +       /* added due to bug Bug 1250214 - prevent the 2nd memory leak */
> +       PORT_ZFree(dig, sizeof(JAR_Digest));

indentation
Attachment #8722907 - Flags: review?(franziskuskiefer) → review+
Keywords: checkin-needed
just a reminder in order to check in when the tree will be open again.
Flags: needinfo?(franziskuskiefer)
https://hg.mozilla.org/projects/nss/rev/e3abd1b9ce00
Flags: needinfo?(franziskuskiefer)
Keywords: checkin-needed
Target Milestone: --- → 3.24
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.