cached certs are forced updated wantonly

RESOLVED FIXED in 3.7.1

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: Ian McGreer, Assigned: Ian McGreer)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

2.24 KB, patch
Wan-Teh Chang
: review+
Robert Relyea
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

15 years ago
When searching a trust domain (set of tokens) for certs, NSS first puts all
matching certs from the cache into a collection.  Then, each matching cert
instance from a token is added to the collection.  When multiple instances of
the same cert are detected,  NSS forces a refresh of the CERTCertificate.

However, this also occurs when the same instance of a cached cert is detected. 
There is no need to refresh in this case, as we have just located the cached
instance we already knew about.

This has a side effect of not allowing the value of cert->trust to be set
temporarily, as the refresh constantly overwrites that value.
(Assignee)

Comment 1

15 years ago
Created attachment 110623 [details] [diff] [review]
suggested patch

Comment 2

15 years ago
This patch makes it so that the trust is preserved, but I'm seeing *tons* of run
time asserts in PR_Calloc and PR_Free calls which leads me to believe something
funky may be happening with memory allocation/freeing as a result of this patch.

Updated

15 years ago
Priority: -- → P1
Target Milestone: --- → 3.7.1
(Assignee)

Comment 3

15 years ago
Created attachment 110774 [details] [diff] [review]
rev 2


That's what you get for asking for a patch late on Friday afternoon :)

There was something very obviously wrong with the last patch that I didn't see
before.  This patch implements the same idea, but (hopefully) correctly.  I
don't have a test case though.
Attachment #110623 - Attachment is obsolete: true

Comment 4

15 years ago
Comment on attachment 110774 [details] [diff] [review]
rev 2

r=wtc.

It seems clearer to rename the 'foundIt' parameter
'newInstance' (and reverse the true/false sense) or
'alreadyInCollection'.

Javi, could you test this patch?

Bob, could you review this patch, too?
Attachment #110774 - Flags: superreview?(relyea)
Attachment #110774 - Flags: review+

Comment 5

15 years ago
the latest patch preserves the trust bits without throwing up all those
Assertions from within the memory allocator module.

Comment 6

15 years ago
Comment on attachment 110774 [details] [diff] [review]
rev 2

It looks OK, though I would like to make sure it doesn't break smart cards
before we officially bless it.
Attachment #110774 - Flags: superreview?(relyea) → superreview+

Comment 7

15 years ago
I just tested Ian's patch with a smart card with Bob
looking over my shoulder.

Ian, please go ahead and check in your patch on the
trunk.  After it passes QA on all platforms, we will
check it in on the NSS_3_7_BRANCH.

Comment 8

15 years ago
Patch rev 2 checked into the tip and NSS_3_7_BRANCH of NSS.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.