469619 - Coverity errors reported in libSMIME

Status: RESOLVED FIXED

Description

[Nelson Bolyard (seldom reads bugmail)]

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"

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

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]

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

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"

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 4

[Shailen]

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

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 6

[timeless]

[e-mail]: 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'.

Comment 7

[Shailen]

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

Comment 8

[Shailen]

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

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 431053 [details] [diff] [review]
Patch Version 3

r=nelson

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

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