Closed
Bug 337008
Opened 19 years ago
Closed 19 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)
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)
2.82 KB,
patch
|
nelson
:
review+
wtc
:
superreview+
|
Details | Diff | Splinter Review |
Event returned_null: Function "nssList_Create" returned NULL value (checked 15 out of 17 times)
Updated•19 years ago
|
Hardware: PC → All
Target Milestone: --- → 3.11.2
Updated•19 years ago
|
Priority: -- → P2
Assignee | ||
Updated•19 years ago
|
Assignee: nobody → alexei.volkov.bugs
Assignee | ||
Comment 1•19 years ago
|
||
Attachment #222391 -
Flags: review?(rrelyea)
Assignee | ||
Comment 2•19 years ago
|
||
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)
Assignee | ||
Updated•19 years ago
|
Attachment #222391 -
Flags: superreview?(wtchang)
Comment 3•19 years ago
|
||
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 4•19 years ago
|
||
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+
Comment 5•19 years ago
|
||
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".
Assignee | ||
Comment 6•19 years ago
|
||
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"
Assignee | ||
Comment 7•19 years ago
|
||
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 8•19 years ago
|
||
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 9•19 years ago
|
||
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 10•19 years ago
|
||
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.
Comment 11•19 years ago
|
||
CIDs 306 307 691 880
Whiteboard: "more work needed" → "more work needed" [CID 306 307 691 880]
Assignee | ||
Comment 12•19 years ago
|
||
(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.
Assignee | ||
Comment 13•19 years ago
|
||
Attachment #222391 -
Attachment is obsolete: true
Attachment #222748 -
Attachment is obsolete: true
Attachment #225354 -
Flags: superreview?(wtchang)
Attachment #225354 -
Flags: review?(nelson)
Comment 14•19 years ago
|
||
Comment on attachment 225354 [details] [diff] [review]
changes according nelson comments
r=nelson
Thanks, Alexei.
Attachment #225354 -
Flags: review?(nelson) → review+
Comment 15•19 years ago
|
||
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+
Assignee | ||
Comment 16•19 years ago
|
||
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: 19 years ago
Resolution: --- → FIXED
Updated•19 years ago
|
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"
Updated•14 years ago
|
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.
Description
•