Closed Bug 1229070 Opened 6 years ago Closed 6 years ago

Incorrect comment in ssl3_HandleCertificateStatus

Categories

(NSS :: Libraries, defect, P2)

defect

Tracking

(firefox45 affected)

RESOLVED FIXED
Tracking Status
firefox45 --- affected

People

(Reporter: ekr, Assigned: wtc)

Details

Attachments

(2 files, 1 obsolete file)

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.
Assignee: nobody → wtc
Status: NEW → ASSIGNED
Attachment #8693657 - Flags: review?(ekr)
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)
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.
(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.
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+
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+
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+
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+
Severity: normal → trivial
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → 3.21.1
Target Milestone: 3.21.1 → 3.22
Kai: the version number in lib/nss/nss.h is 3.21.1. Is that wrong?
(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.