If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

function 'SEC_FindCrlByName' in crl.c does not set 'pkcs11ID' field in the 'CERTSignedCrl' it returns

RESOLVED FIXED in 3.4

Status

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

People

(Reporter: Rangan Sen, Assigned: Wan-Teh Chang)

Tracking

x86
Windows NT
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

624 bytes, patch
Robert Relyea
: review+
Details | Diff | Splinter Review
(Reporter)

Description

16 years ago
This makes subsequent calls to 'SEC_DeletePermCRL' [using the 'CERTSignedCrl'
returned by 'SEC_FindCrlByName'] fail.

This blocks PSM bug# 130979
(Reporter)

Comment 1

16 years ago
Created attachment 74131 [details] [diff] [review]
Suggested Patch

Bob, can you please review...
(Reporter)

Updated

16 years ago
Blocks: 130979

Comment 2

16 years ago
Good catch, We must be getting CRLs some other way when delete *was* working for
CRLs. 

It looks like we have a similar problem in PK11_LookupCrls.
Also it looks like SEC_DeletePermCRL is not returning an erro code on failure.
I'll supply patches for these problems as well.

bob

Comment 3

16 years ago
Rangen, how are you freeing the CRLHeadNodes from PK11_LookupCrls?

bob
(Reporter)

Comment 4

16 years ago
By calling PORT_FreeArena(head->arena, PR_FALSE);
(Assignee)

Comment 5

16 years ago
Comment on attachment 74131 [details] [diff] [review]
Suggested Patch

>+	if(crlHandle != CK_INVALID_HANDLE)
>+	 crl->pkcs11ID = crlHandle;

Can we simply say
	crl->pkcs11ID = crlHandle;
?

Do we not want to override the old value of crl->pkcs11ID
if crlHandle is invalid?

Comment 6

16 years ago
it should assert, or completely fail. assert is probably better, since
PK11_FindCrlByName does not return the DERCrl unless the crlHandle is valid.

bob

Comment 7

16 years ago
OK, that's what I was afraid of.

It looks like you can't call SEC_DeletePermCert on any of these CRLs. If I setup
the slot and handle, then we will leak references to the slot whenever we call
this function.

bob
(Reporter)

Comment 8

16 years ago
OK, I'll update the patch to assert ....
(Assignee)

Comment 9

16 years ago
We should not use assertions to check for invalid input
arguments.  Invalid input arguments should be reported
with a failure status and the appropriate error code.

We should only use assertions to detect internal errors
or when there is no way to report an error (e.g, in a
function that returns void).

Comment 10

16 years ago
It's not an input value, it's generated by a function which we call withint this
function to retrieve the crl.
We should either assert or fail outright, though it would be an internal error
if we ever hit this condition.
(Reporter)

Comment 11

16 years ago
Created attachment 74361 [details] [diff] [review]
updated patch

I agree ... if PK11_FindCrlByName returns non-null, and crlhandle is still
invalid, it is an internal error - and it would make sense putting an assert
right there...

Comment 12

16 years ago
Comment on attachment 74361 [details] [diff] [review]
updated patch

Patch looks good rangen r=relyea
Attachment #74361 - Flags: review+
(Assignee)

Updated

16 years ago
Attachment #74131 - Attachment is obsolete: true
(Assignee)

Comment 13

16 years ago
I checked in Rangan's patch.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Priority: -- → P1
Resolution: --- → FIXED
Target Milestone: --- → 3.4
(Assignee)

Updated

16 years ago
Blocks: 131513
(Reporter)

Comment 14

16 years ago
Thanks .... small correction, Bob - its Rangan, not Rangen :)
You need to log in before you can comment on or make changes to this bug.