284839 - A patch to improve the responsiveness of SECMOD_WaitForAnyTokenEvent

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Wan-Teh Chang]

SECMOD_WaitForAnyTokenEvent is a new function
added in NSS 3.9.3 to handle token insertion
and removal better (bug 252702).

This bug is a patch from Bob Relyea to improve
the responsiveness of that function.

Comment 1

[Wan-Teh Chang]

Attachment: Bob's patch

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 176326 [details] [diff] [review]
Bob's patch

I have a question about this patch's null pointer checking.
Is it doing the right amount?  Too little?  Too much?

The patch introduces code that follows a chain of 3 pointers,
slot, slot->nssToken, and then slot->nssToken->slot.
It checks the first two for NULL before using them, but not the
third one. 

If the first two pointers are permitted to be NULL legitimately,
but the third pointer may not legitimately be NULL if the first 
two are non-NULL, then this code is correct.  But if the third
pointer may legitimately be NULL, then it would see that an additional
test is needed.  I'm not aware of any document that tells us whether
the third pointer is or is not permitted ot be NULL.  So, I don't
know if this NULL pointer needs to be checked or not.

Any ideas?

This patch is fine, except possibly for this issue.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 176326 [details] [diff] [review]
Bob's patch

r=nelson, with the following suggestions,

>+    /* if we are in the delay period for the "isPresent" call, reset
>+     * the delay since we know things have probably changed... */
>+    if (slot && slot->nssToken) {
>+	nssSlot_ResetDelay(slot->nssToken->slot);
>+    }

I would prefer the following test:

>+    if (slot && slot->nssToken && slot->nssToken->slot) {

or alternatively change nssSlot_ResetDelay to check its input for NULL.

Comment 4

[Wan-Teh Chang]

Attachment: Bob's original patch

Nelson, thanks for the code review.  Your suggested
change is actually in Bob's original patch.  Therefore,
I decided to check in his original patch instead.

The reason I removed the slot->nssToken->slot test
is that by code inspection I concluded that that test
is not necessary, and I've also confirmed with Bob
that that test was defensive programming and wasn't
there to fix a crash he encountered.  But I think it
is more work for me to convince you and other code
reviewers why that test isn't necessary.  So I am
happy to go with defensive programming.

Comment 5

[Wan-Teh Chang]

I checked in Bob's original patch on the tip (NSS 3.10)
and NSS_3_9_BRANCH (NSS 3.9.6).