Last Comment Bug 353912 - Misc klocwork bugs in lib/ckfw
: Misc klocwork bugs in lib/ckfw
Status: RESOLVED FIXED
: klocwork
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: trunk
: All All
: P2 normal (vote)
: 3.12
Assigned To: Alexei Volkov
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-09-22 18:31 PDT by Nelson Bolyard (seldom reads bugmail)
Modified: 2007-01-04 16:23 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
fixes (8.54 KB, patch)
2006-10-27 16:45 PDT, Alexei Volkov
nelson: review-
Details | Diff | Review
add back fwObject->mdToken(was unintentionally removed) (8.59 KB, patch)
2006-11-02 14:49 PST, Alexei Volkov
nelson: review+
Details | Diff | Review

Description Nelson Bolyard (seldom reads bugmail) 2006-09-22 18:31:55 PDT
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) 2006-09-22 18:38:08 PDT
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) 2006-09-23 19:56:39 PDT
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 2006-10-27 16:45:00 PDT
Created attachment 243858 [details] [diff] [review]
fixes
Comment 4 Nelson Bolyard (seldom reads bugmail) 2006-10-30 15:23:22 PST
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 2006-11-02 14:49:13 PST
Created attachment 244502 [details] [diff] [review]
add back fwObject->mdToken(was unintentionally removed)
Comment 6 Nelson Bolyard (seldom reads bugmail) 2006-11-02 15:19:06 PST
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 2007-01-04 16:23:38 PST
/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

Note You need to log in before you can comment on or make changes to this bug.