Coverity errors reported in libSMIME

RESOLVED FIXED in 3.12.7

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Shailen)

Tracking

({coverity, memory-leak, regression})

3.12
3.12.7
coverity, memory-leak, regression

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

5.16 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
Coverity reports some new bugs in libSMIME.  Appear to be regressions.
Some of the code changes appear to have been part of the work done for 
bug 401928 - Support generalized PKCS#5 v2 PBEs
I will add info about those bugs as comments to this bug

CID 1352 - memory leak

In file nss/lib/pkcs7/p7local.c, function sec_PKCS7CreateDecryptObject
Coverity reports:

127 	cryptoMechType = PK11_GetPBECryptoMechanism(algid, &param, pwitem);
128 	if (cryptoMechType == CKM_INVALID_MECHANISM) {
129 	    PORT_Free(result);
130  	    return NULL;
131  	}

Event leaked_storage: Returned without freeing storage "param"
(Reporter)

Comment 1

10 years ago
CID 1351 - memory leak

In file nss/lib/pkcs7/p7local.c, function sec_PKCS7CreateEncryptObject
(same problem as CID 1352 above, except different function.)

202  	cryptoMechType = PK11_GetPBECryptoMechanism(algid, &param, pwitem);
203  	if (cryptoMechType == CKM_INVALID_MECHANISM) {
204  	    PORT_Free(result);
205  	    return NULL;
206  	}

Event leaked_storage: Returned without freeing storage "param"
Also see events: [alloc_arg]
(Reporter)

Comment 2

10 years ago
CID 1353 - memory leak

File nss/lib/smime/cmscipher.c Function NSS_CMSCipherContext_StartDecrypt

102  	cryptoMechType = PK11_GetPBECryptoMechanism(algid, &param, pwitem);
103  	if (cryptoMechType == CKM_INVALID_MECHANISM) {
104  	    return NULL;
105  	}

Event leaked_storage: Returned without freeing storage "param"
(Reporter)

Comment 3

10 years ago
CID 1354 - memory leak

file nss/lib/smime/cmscipher.c function NSS_CMSCipherContext_StartEncrypt

172  	cryptoMechType = PK11_GetPBECryptoMechanism(algid, &param, pwitem);
173  	if (cryptoMechType == CKM_INVALID_MECHANISM) {
174  	    return NULL;
175  	}

Event leaked_storage: Returned without freeing storage "param"

Since all 4 of these CIDs are due to PK11_GetPBECryptoMechanism, 
I'm assigning this bug to Bob
Assignee: nobody → rrelyea
Priority: -- → P2
(Reporter)

Updated

9 years ago
Target Milestone: 3.12.3 → 3.12.5
(Assignee)

Comment 4

8 years ago
Created attachment 430914 [details] [diff] [review]
Patch V 1

Nelson:

 Can you please review the patch and let me know if anything is missing.

Regards,
Shailendra
Attachment #430914 - Flags: review?(nelson)
Comment on attachment 430914 [details] [diff] [review]
Patch V 1

param must be initialized to NULL in all these cases, or else it can be uninitialized when it is passed to SECITEM)FreeItem.
Attachment #430914 - Flags: review?(nelson) → review-

Comment 6

8 years ago
shailen.n.jain@gmail.com: i've given you canconfirm+editbugs. please use these bits wisely. mostly for now, it'll mean that when you attach a patch, bugzilla will let you change the owner to yourself and change the state to 'assigned'.
Assignee: rrelyea → shailen.n.jain
Status: NEW → ASSIGNED
(Assignee)

Comment 7

8 years ago
Created attachment 431050 [details] [diff] [review]
Patch Version 2

Nelson :

  I have initialized the param variable.

  Please review and let me know if any changes required.

Thanks,
Shailendra
Attachment #430914 - Attachment is obsolete: true
Attachment #431050 - Flags: review?(nelson)
(Assignee)

Comment 8

8 years ago
Created attachment 431053 [details] [diff] [review]
Patch Version 3

Nelson :

 For some reason the file path for cmscipher.c was selected as lib/pk11wrap/ instead of ./lib/smime in the patch that I posted.

  So I am attaching the newer version of the patch.

Thanks and Regards,
Shailendra
Attachment #431050 - Attachment is obsolete: true
Attachment #431053 - Flags: review?(nelson)
Attachment #431050 - Flags: review?(nelson)
Comment on attachment 431053 [details] [diff] [review]
Patch Version 3

r=nelson
Attachment #431053 - Flags: review?(nelson) → review+
Checking in pkcs7/p7local.c;   new revision: 1.14; previous revision: 1.13
Checking in smime/cmscipher.c; new revision: 1.13; previous revision: 1.12
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: 3.12.5 → 3.12.7
You need to log in before you can comment on or make changes to this bug.