353912 - Misc klocwork bugs in lib/ckfw

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

ID:       92546
Function: nssCKFWObject_Create
Location: nss/lib/ckfw/object.c : 172

Suspicious dereference of pointer 'fwToken' by passing argument 1 to function 'nssCKFWToken_GetMDObjectHash' at line 172 before NULL check at line 199

IOW, this function will crash at line 172 if fwToken is NULL, so it makes 
no sense to test that pointer for null after that point.  The test should 
preceed line 172.

----

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

ID:       92219
Function: nssCKFWSession_GetOperationStateLen
Location: nss/lib/ckfw/session.c : 1034

Pointer 'fwSession->mdSession->GetOperationStateLen' checked for NULL at 
line 1026 will be dereferenced at line 1034.

We check it for NULL at line 1026, but then go right on and use it anyway
at line 1034.  

----

ID:
92220
Function: nssCKFWSession_CopyObject
Location: nss/lib/ckfw/session.c : 1437

Suspicious dereference of pointer 'fwObject' by passing argument 1 to function 'nssCKFWObject_IsTokenObject' at line 1437 before NULL check at line 1489

After we use fwOjbect at line 1437, we check it for NULL and line 1489.
There's a large block of dead code at line 1489, if fwOjbect cannot be NULL.
So, we should either remove the large block of dead code, or fix line 1437
to not crash if fwObject is NULL

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

ID:       92222
Function: nssCKMDSessionObject_Create
Location: nss/lib/ckfw/sessobj.c : 286

Pointer 'mdso->types' returned from call to function 'nss_ZAlloc' at 
line 283 may be NULL and may be dereferenced at line 286.

283	  mdso->types = nss_ZNEWARRAY(arena, CK_ATTRIBUTE_TYPE, ulCount); 
284	 
285	  for( i = 0; i < ulCount; i++ ) { 
286	    mdso->types[i] = attributes[i].type; 

----

ID:       92223
Function: nssCKMDFindSessionObjects_Create
Location: nss/lib/ckfw/sessobj.c : 1016

Pointer 'rv' returned from call to function 'nss_ZAlloc' at line 1001 
may be NULL and may be dereferenced at line 1016.

1001	  rv = nss_ZNEW(arena, NSSCKMDFindObjects); 
1016	  rv->etc = (void *)mdfso; 

Comment 3

[Alexei Volkov]

Attachment: fixes

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 243858 [details] [diff] [review]
fixes

There's one problem with this patch.

>Index: object.c

>   fwObject->fwToken = fwToken;
>-
>-  if( (NSSCKFWToken *)NULL != fwToken ) {
>-    fwObject->mdToken = nssCKFWToken_GetMDToken(fwToken);
>-  }

I believe the correct change is to remove the conditional test
since fwToken cannot be NULL here, but not to remove the call
to nssCKFWToken_GetMDToken nor the assignment of its result.

>-
>   fwObject->fwInstance = fwInstance;

Comment 5

[Alexei Volkov]

Attachment: add back fwObject->mdToken(was unintentionally removed)

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 244502 [details] [diff] [review]
add back fwObject->mdToken(was unintentionally removed)

r=nelson.

One more question, which is really a question about the correctness of the code before your patch.  

Can nssCKFWToken_GetMDToken(fwToken) return NULL (or some other value that means error/failure)? 
If so, should we test for that here after this call to it?

Comment 7

[Alexei Volkov]

/cvsroot/mozilla/security/nss/lib/ckfw/object.c,v  <--  object.c
new revision: 1.13; previous revision: 1.12
/cvsroot/mozilla/security/nss/lib/ckfw/session.c,v  <--  session.c
new revision: 1.11; previous revision: 1.10
/cvsroot/mozilla/security/nss/lib/ckfw/sessobj.c,v  <--  sessobj.c
new revision: 1.13; previous revision: 1.12