Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Misc klocwork bugs in lib/ckfw

RESOLVED FIXED in 3.12

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Alexei Volkov)

Tracking

({klocwork})

trunk
3.12
klocwork

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

8.59 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
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.

----
(Reporter)

Comment 1

11 years ago
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
Keywords: klocwork
(Reporter)

Comment 2

11 years ago
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; 
(Reporter)

Updated

11 years ago
Priority: -- → P2
Target Milestone: --- → 3.12
(Assignee)

Comment 3

11 years ago
Created attachment 243858 [details] [diff] [review]
fixes
Assignee: nobody → alexei.volkov.bugs
Status: NEW → ASSIGNED
Attachment #243858 - Flags: review?(nelson)
(Reporter)

Comment 4

11 years ago
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;
Attachment #243858 - Flags: review?(nelson) → review-
(Assignee)

Comment 5

11 years ago
Created attachment 244502 [details] [diff] [review]
add back fwObject->mdToken(was unintentionally removed)
Attachment #243858 - Attachment is obsolete: true
Attachment #244502 - Flags: review?(nelson)
(Reporter)

Comment 6

11 years ago
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?
Attachment #244502 - Flags: review?(nelson) → review+
(Assignee)

Comment 7

11 years ago
/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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.