Closed Bug 337008 Opened 15 years ago Closed 15 years ago

Coverity OOM crash [@ nssList_Add - STAN_InitTokenForSlotInfo - STAN_LoadDefaultNSS3TrustDomain][@ nssList_Clone - nssList_CreateIterator - STAN_LoadDefaultNSS3TrustDomain] Dereferencing possibly NULL "(td)->tokenList"

Categories

(NSS :: Libraries, defect, P2)

3.11
All
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.11.2

People

(Reporter: timeless, Assigned: alvolkov.bgs)

References

()

Details

(Keywords: coverity, crash, Whiteboard: "more work needed" [CID 306 307 691 880])

Crash Data

Attachments

(1 file, 2 obsolete files)

Event returned_null: Function "nssList_Create" returned NULL value (checked 15 out of 17 times)
Hardware: PC → All
Target Milestone: --- → 3.11.2
Priority: -- → P2
Assignee: nobody → alexei.volkov.bugs
Attachment #222391 - Flags: review?(rrelyea)
Comment on attachment 222391 [details] [diff] [review]
if list is null release locks and return failure

Asking Nelson for review. Bob is on vacation.
Attachment #222391 - Flags: review?(rrelyea) → review?(nelson)
Attachment #222391 - Flags: superreview?(wtchang)
Comment on attachment 222391 [details] [diff] [review]
if list is null release locks and return failure

r=nelson
Attachment #222391 - Flags: review?(nelson) → review+
Comment on attachment 222391 [details] [diff] [review]
if list is null release locks and return failure

r=wtc.  This patch should silence Coverity.  But there are
still two issues.

1. This function has two other failure points that aren't
checked.

  td->tokens = nssList_CreateIterator(td->tokenList);

  g_default_crypto_context = NSSTrustDomain_CreateCryptoContext(td, NULL);

Ideally, we should check for the failure of these two
function calls, destroy what we have created so far,
and return PR_FAILURE.

2. We should review NSSTrustDomain_Destroy to ensure that
it can destroy a partially constructed NSSTrustDomain
object safely.  For example, it now says:

        /* Destroy each token in the list of tokens */
        if (td->tokens) {
            nssListIterator_Destroy(td->tokens);
            nssList_Clear(td->tokenList, token_destructor);
            nssList_Destroy(td->tokenList);
        }

This means if td->tokens is NULL but td->tokenList is
not NULL, which happens if the nssList_CreateIterator
call I mentioned above fails, we will fail to destroy
td->tokenList.

Since it'll take extraordinary work to inject artificial
failures to test the error handling code, I am not sure
if it's worthwhile to address the above two issues.  (This
is OOM during NSS initialization.)
Attachment #222391 - Flags: superreview?(wtchang) → superreview+
Wan-Teh, I agree with your remarks in comment 4, and think they're serious
enough that your review should be construed as r-, rather than r+.

Alexei, please make another patch that addresses those additional comments.
We might as well "head Coverity off at the pass".
The patch is already checked in. But will leave the bug open and create one more patch to fix remaining issues.

trunk:
/cvsroot/mozilla/security/nss/lib/pki/pki3hack.c,v  <--  pki3hack.c
new revision: 1.87; previous revision: 1.86

3.11 branch:
/cvsroot/mozilla/security/nss/lib/pki/pki3hack.c,v  <--  pki3hack.c
new revision: 1.86.28.1; previous revision: 1.86
Whiteboard: "more work needed"
Attached patch incorporate wtc comments (obsolete) — Splinter Review
Remove tokenList initialization outside of locks. So far tokenList is local for the function and nssList_Create does not involve any initializations outside of the function scope.

Check nssList_CreateIterator and NSSTrustDomain_CreateCryptoContext return values.

Modification to  NSSTrustDomain_Destroy to clear and to destroy tokenList in case when "!td->tokens".
Attachment #222748 - Flags: superreview?(wtchang)
Attachment #222748 - Flags: review?(nelson)
Comment on attachment 222748 [details] [diff] [review]
incorporate wtc comments

Two comments:

>Index: pki3hack.c

>     g_default_trust_domain = td;
>     g_default_crypto_context = NSSTrustDomain_CreateCryptoContext(td, NULL);
>+    if (!g_default_crypto_context) {
>+	goto loser;

1. If we go to loser here, then we leave g_default_trust_domain pointing at 
a destroyed td.  So, instead we should not set g_default_trust_domain to td
until after NSSTrustDomain_CreateCryptoContext has succeeded.

>+    }
>     return PR_SUCCESS;
>+
>+  loser:
>+    NSSTrustDomain_Destroy(td);
>+    return PR_FAILURE;
> }

>Index: trustdomain.c

>     if (--td->refCount == 0) {
> 	/* Destroy each token in the list of tokens */
> 	if (td->tokens) {
> 	    nssListIterator_Destroy(td->tokens);
> 	    nssList_Clear(td->tokenList, token_destructor);
> 	    nssList_Destroy(td->tokenList);
>+	} else if (td->tokenList) {
>+	    nssList_Clear(td->tokenList, token_destructor);
>+	    nssList_Destroy(td->tokenList);
> 	}

I think what we want is more like this:

> 	if (td->tokens) {
> 	    nssListIterator_Destroy(td->tokens);
>           td->tokens = NULL;
> 	} 
>       if (td->tokenList) {
> 	    nssList_Clear(td->tokenList, token_destructor);
> 	    nssList_Destroy(td->tokenList);
>           td->tokenList = NULL;
> 	}
Attachment #222748 - Flags: review?(nelson) → review-
Comment on attachment 222748 [details] [diff] [review]
incorporate wtc comments

I agree with Nelson's review comments.  I'm also wondering
if we should move
  td->tokens = nssList_CreateIterator(td->tokenList);
outside the critical region.
Attachment #222748 - Flags: superreview?(wtchang) → superreview-
Comment on attachment 222748 [details] [diff] [review]
incorporate wtc comments

>+  loser:
>+    NSSTrustDomain_Destroy(td);
>+    return PR_FAILURE;
> }

Nit: I believe the prevalent style in NSS is to not
indent labels.
CIDs 306 307 691 880
Whiteboard: "more work needed" → "more work needed" [CID 306 307 691 880]
(In reply to comment #9)
> (From update of attachment 222748 [details] [diff] [review] [edit])
> I'm also wondering if we should move
>   td->tokens = nssList_CreateIterator(td->tokenList);
> outside the critical region.

Looks like we can not do so. nssList_CreateIterator calls funsion nssList_Clone
which access the data on the original list, that in this case is one of the things  protected by the lock. 

Attachment #222391 - Attachment is obsolete: true
Attachment #222748 - Attachment is obsolete: true
Attachment #225354 - Flags: superreview?(wtchang)
Attachment #225354 - Flags: review?(nelson)
Comment on attachment 225354 [details] [diff] [review]
changes according nelson comments

r=nelson
Thanks, Alexei.
Attachment #225354 - Flags: review?(nelson) → review+
Comment on attachment 225354 [details] [diff] [review]
changes according nelson comments

r=wtc.  Good patch.

In trustdomain.c, NSSTrustDomain_Destroy, since
you are already nulling td's pointer members
after destroying the objects they point to, you
might as well add
  	td->tokensLock = NULL;
after
  	NSSRWLock_Destroy(td->tokensLock);
Attachment #225354 - Flags: superreview?(wtchang) → superreview+
tip:

/cvsroot/mozilla/security/nss/lib/pki/pki3hack.c,v  <--  pki3hack.c
new revision: 1.88; previous revision: 1.87
/cvsroot/mozilla/security/nss/lib/pki/trustdomain.c,v  <--  trustdomain.c
new revision: 1.52; previous revision: 1.51

3.11 branch:
/cvsroot/mozilla/security/nss/lib/pki/pki3hack.c,v  <--  pki3hack.c
new revision: 1.86.28.2; previous revision: 1.86.28.1
/cvsroot/mozilla/security/nss/lib/pki/trustdomain.c,v  <--  trustdomain.c
new revision: 1.51.28.1; previous revision: 1.51
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Summary: OOM crash [@ nssList_Add - STAN_InitTokenForSlotInfo - STAN_LoadDefaultNSS3TrustDomain][@ nssList_Clone - nssList_CreateIterator - STAN_LoadDefaultNSS3TrustDomain] Dereferencing possibly NULL "(td)->tokenList" → Coverity OOM crash [@ nssList_Add - STAN_InitTokenForSlotInfo - STAN_LoadDefaultNSS3TrustDomain][@ nssList_Clone - nssList_CreateIterator - STAN_LoadDefaultNSS3TrustDomain] Dereferencing possibly NULL "(td)->tokenList"
Crash Signature: [@ nssList_Add - STAN_InitTokenForSlotInfo - STAN_LoadDefaultNSS3TrustDomain] [@ nssList_Clone - nssList_CreateIterator - STAN_LoadDefaultNSS3TrustDomain]
You need to log in before you can comment on or make changes to this bug.