Closed
Bug 1229070
Opened 9 years ago
Closed 9 years ago
Incorrect comment in ssl3_HandleCertificateStatus
Categories
(NSS :: Libraries, defect, P2)
NSS
Libraries
Tracking
(firefox45 affected)
RESOLVED
FIXED
3.22
Tracking | Status | |
---|---|---|
firefox45 | --- | affected |
People
(Reporter: ekr, Assigned: wtc)
Details
Attachments
(2 files, 1 obsolete file)
2.74 KB,
patch
|
ekr
:
review+
wtc
:
checked-in+
|
Details | Diff | Splinter Review |
550 bytes,
patch
|
ekr
:
review+
wtc
:
checked-in+
|
Details | Diff | Splinter Review |
At the top of that function,
it says:
" This is always called before ssl3_HandleCertificate, even if the Certificate message is sent first."
However, looking at the code in ssl3_HandleHandshakeMessage, that
does not appear to be true, rather they appear to be called directly
at the point where the messages are seen:
https://dxr.mozilla.org/nss/source/nss/lib/ssl/ssl3con.c#11590
case certificate:
rv = ssl3_HandleCertificate(ss, b, length);
break;
case certificate_status:
rv = ssl3_HandleCertificateStatus(ss, b, length);
break;
Per discussion with WTC on IRC, this appears to be an incorrect comment from previous code. This bug is to just fix the comment.
Reporter | ||
Comment 1•9 years ago
|
||
The original code did have this property:
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=360420&attachment=616669
Assignee | ||
Comment 2•9 years ago
|
||
Reporter | ||
Comment 3•9 years ago
|
||
Comment on attachment 8693657 [details] [diff] [review]
Fix incorrect comments. Don't assign a variable in a function call.
Review of attachment 8693657 [details] [diff] [review]:
-----------------------------------------------------------------
::: lib/ssl/ssl3con.c
@@ +6543,5 @@
> ss->ssl3.hs.preliminaryInfo |= ssl_preinfo_cipher_suite;
> PORT_Assert(ss->ssl3.hs.suite_def);
> if (!ss->ssl3.hs.suite_def) {
> + errCode = SEC_ERROR_LIBRARY_FAILURE;
> + PORT_SetError(errCode);
We do PORT_SetError(SEC_ERROR_LIBRARY_FAILURE) here and *also* set
errCode = SEC_ERROR_LIBRARY_FAILURE.
then we jump to loser:
loser:
errCode = ssl_MapLowLevelError(errCode);
return SECFailure;
ssl_MapLowLevelError(x) does PORT_GetError() and then depending on that return value, does PORT_SetError(x) and then returns either x or the original error. SEC_ERROR_LIBRARY_FAILURE is one of the errors it maps, but since we are also passing this in as the high level error code, I believe the result is:
1. Re PORT_SetError(SEC_ERROR_LIBRARY_FAILURE).
2. Return SEC_ERROR_LIBRARY_FAILURE. It is then assigned to errCode, which we ignore.
It seems like we might want to:
1. Remove the PORT_SetError here.
2. Remove the assignment to errCode in loser.
Thoughts?
Attachment #8693657 -
Flags: review?(ekr)
Assignee | ||
Comment 4•9 years ago
|
||
Comment on attachment 8693657 [details] [diff] [review]
Fix incorrect comments. Don't assign a variable in a function call.
Review of attachment 8693657 [details] [diff] [review]:
-----------------------------------------------------------------
::: lib/ssl/ssl3con.c
@@ +6543,5 @@
> ss->ssl3.hs.preliminaryInfo |= ssl_preinfo_cipher_suite;
> PORT_Assert(ss->ssl3.hs.suite_def);
> if (!ss->ssl3.hs.suite_def) {
> + errCode = SEC_ERROR_LIBRARY_FAILURE;
> + PORT_SetError(errCode);
I agree that the assignment to errCode in |loser| should be removed.
As for the PORT_SetError() call, it ensures that |oldErr| is not a
stale error code from a previously failed NSS function. So it is
necessary.
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8693657 -
Attachment is obsolete: true
Attachment #8693797 -
Flags: review?(ekr)
Reporter | ||
Comment 6•9 years ago
|
||
(In reply to Wan-Teh Chang from comment #4)
> Comment on attachment 8693657 [details] [diff] [review]
> Fix incorrect comments. Don't assign a variable in a function call.
>
> Review of attachment 8693657 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: lib/ssl/ssl3con.c
> @@ +6543,5 @@
> > ss->ssl3.hs.preliminaryInfo |= ssl_preinfo_cipher_suite;
> > PORT_Assert(ss->ssl3.hs.suite_def);
> > if (!ss->ssl3.hs.suite_def) {
> > + errCode = SEC_ERROR_LIBRARY_FAILURE;
> > + PORT_SetError(errCode);
>
> I agree that the assignment to errCode in |loser| should be removed.
>
> As for the PORT_SetError() call, it ensures that |oldErr| is not a
> stale error code from a previously failed NSS function. So it is
> necessary.
Thank you for explaining.
Reporter | ||
Comment 7•9 years ago
|
||
Comment on attachment 8693797 [details] [diff] [review]
Fix incorrect comments. Don't assign a variable in a function call. Remove an ignored assignment.
Review of attachment 8693797 [details] [diff] [review]:
-----------------------------------------------------------------
LGTM
Attachment #8693797 -
Flags: review?(ekr) → review+
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8693797 [details] [diff] [review]
Fix incorrect comments. Don't assign a variable in a function call. Remove an ignored assignment.
Review of attachment 8693797 [details] [diff] [review]:
-----------------------------------------------------------------
https://hg.mozilla.org/projects/nss/rev/ddfaae48f249
Attachment #8693797 -
Flags: checked-in+
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8693809 -
Flags: review?(ekr)
Reporter | ||
Comment 10•9 years ago
|
||
Comment on attachment 8693809 [details] [diff] [review]
Remove an ignored assignment in dtls_HandleHelloVerifyRequest.
Review of attachment 8693809 [details] [diff] [review]:
-----------------------------------------------------------------
LGTM
Attachment #8693809 -
Flags: review?(ekr) → review+
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8693809 [details] [diff] [review]
Remove an ignored assignment in dtls_HandleHelloVerifyRequest.
Review of attachment 8693809 [details] [diff] [review]:
-----------------------------------------------------------------
https://hg.mozilla.org/projects/nss/rev/41a8009129f1
Attachment #8693809 -
Flags: checked-in+
Assignee | ||
Updated•9 years ago
|
Severity: normal → trivial
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → 3.21.1
Updated•9 years ago
|
Target Milestone: 3.21.1 → 3.22
Assignee | ||
Comment 12•9 years ago
|
||
Kai: the version number in lib/nss/nss.h is 3.21.1. Is that wrong?
Comment 13•9 years ago
|
||
(In reply to Wan-Teh Chang from comment #12)
> Kai: the version number in lib/nss/nss.h is 3.21.1. Is that wrong?
Yes, it's wrong. File nss.def contains new APIs marked with version 3.22.
Thanks for making me aware that our trunk version numbers are still at 3.21.1. I'll bump the version numbers in the header files.
You need to log in
before you can comment on or make changes to this bug.
Description
•