Incorrect comment in ssl3_HandleCertificateStatus

RESOLVED FIXED in 3.22

Status

P2
trivial
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ekr, Assigned: wtc)

Tracking

trunk
3.22

Firefox Tracking Flags

(firefox45 affected)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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)

Comment 2

3 years ago
Created attachment 8693657 [details] [diff] [review]
Fix incorrect comments. Don't assign a variable in a function call.
Assignee: nobody → wtc
Status: NEW → ASSIGNED
Attachment #8693657 - Flags: review?(ekr)
(Reporter)

Comment 3

3 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

3 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

3 years ago
Created attachment 8693797 [details] [diff] [review]
Fix incorrect comments. Don't assign a variable in a function call. Remove an ignored assignment.
Attachment #8693657 - Attachment is obsolete: true
Attachment #8693797 - Flags: review?(ekr)
(Reporter)

Comment 6

3 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

3 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

3 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

3 years ago
Created attachment 8693809 [details] [diff] [review]
Remove an ignored assignment in dtls_HandleHelloVerifyRequest.
Attachment #8693809 - Flags: review?(ekr)
(Reporter)

Comment 10

3 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

3 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

3 years ago
Severity: normal → trivial
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → 3.21.1

Updated

3 years ago
Target Milestone: 3.21.1 → 3.22
(Assignee)

Comment 12

3 years ago
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.