Closed
Bug 130983
Opened 23 years ago
Closed 23 years ago
function 'SEC_FindCrlByName' in crl.c does not set 'pkcs11ID' field in the 'CERTSignedCrl' it returns
Categories
(NSS :: Libraries, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
3.4
People
(Reporter: rangansen, Assigned: wtc)
References
Details
Attachments
(1 file, 1 obsolete file)
624 bytes,
patch
|
rrelyea
:
review+
|
Details | Diff | Splinter Review |
This makes subsequent calls to 'SEC_DeletePermCRL' [using the 'CERTSignedCrl'
returned by 'SEC_FindCrlByName'] fail.
This blocks PSM bug# 130979
Reporter | ||
Comment 1•23 years ago
|
||
Bob, can you please review...
Comment 2•23 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•23 years ago
|
||
Rangen, how are you freeing the CRLHeadNodes from PK11_LookupCrls?
bob
Reporter | ||
Comment 4•23 years ago
|
||
By calling PORT_FreeArena(head->arena, PR_FALSE);
Assignee | ||
Comment 5•23 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•23 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•23 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•23 years ago
|
||
OK, I'll update the patch to assert ....
Assignee | ||
Comment 9•23 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•23 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•23 years ago
|
||
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•23 years ago
|
||
Comment on attachment 74361 [details] [diff] [review]
updated patch
Patch looks good rangen r=relyea
Attachment #74361 -
Flags: review+
Assignee | ||
Updated•23 years ago
|
Attachment #74131 -
Attachment is obsolete: true
Assignee | ||
Comment 13•23 years ago
|
||
I checked in Rangan's patch.
Status: NEW → RESOLVED
Closed: 23 years ago
Priority: -- → P1
Resolution: --- → FIXED
Target Milestone: --- → 3.4
Reporter | ||
Comment 14•23 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.
Description
•