726315 - Followup from bug 542832

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Kai Engert (:KaiE:)]

In bug 542832 comment 93 Wan-Teh proposed the following cleanup/corrections.

::: security/nss/lib/ssl/sslsock.c
@@ +1959,5 @@
> +
> +    if (ss->version >= SSL_LIBRARY_VERSION_3_0 &&
> +	ss->ssl3.hs.restartTarget != NULL) {
> +	/* Read and write will block until the asynchronous callback completes
> +	 * (e.g. until SSL_AuthCertificateComplete is called), so don't tell

e.g. => i.e.

@@ +1963,5 @@
> +	 * (e.g. until SSL_AuthCertificateComplete is called), so don't tell
> +	 * the caller to poll the socket unless there is pending write data.
> +	 */
> +	if (ss->lastWriteBlocked && ss->pendingBuf.len != 0) {
> +	    new_flags &= (PR_POLL_WRITE | PR_POLL_EXCEPT);

It seems clearer to just clear the PR_POLL_READ flag.

The problem with PR_POLL_EXCEPT is worth a comment in the
source code.

If PR_POLL_EXCEPT is a problem, should we also clear the
PR_POLL_EXCEPT flag when there is pending write data?
I guess it's not necessary because PR_XXX will send the
pending write data, which will fail with a real error
(not PR_WOULD_BLOCK_ERROR).

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Kai Engert (:kaie) from comment #88)
> With the latest checkins, we get a new build warning.
> 
> tstclnt.c: In function ‘ownAuthCertificate’:
> tstclnt.c:328:5: warning: too many arguments for format [-Wformat-extra-args]
> 
> code:
>     FPRINTF(stderr, "using asynchronous certificate validation\n", progName);
> 
> As the parameter is obviously "too much", I've checked in an update to
> remove the parameter.
> 
> cvs commit: Examining .
> Checking in tstclnt.c;
> /cvsroot/mozilla/security/nss/cmd/tstclnt/tstclnt.c,v  <--  tstclnt.c
> new revision: 1.66; previous revision: 1.65
> done

Kai's fix is fine for now. The better fix to to fix the format string and re-add the removed parameter so that this message is in the same format as other messages output by tstclnt. (Also, IIRC, there may be one other instance where we are forgetting to add the program name to an output message in tstclnt, based on recent readings of tinderbox logs.)

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Kai Engert (:kaie) from comment #0)
> > +	 * (e.g. until SSL_AuthCertificateComplete is called), so don't tell
> 
> e.g. => i.e.

Right now, SSL_AuthCertificateComplete is the only possibility, but in the future there may be others (e.g. async client cert handling should be made to work, using the same mechanism).

> > +	if (ss->lastWriteBlocked && ss->pendingBuf.len != 0) {
> > +	    new_flags &= (PR_POLL_WRITE | PR_POLL_EXCEPT);
> 
> It seems clearer to just clear the PR_POLL_READ flag.

I don't care much either way. I did it this way to make it clearer that ONLY write can be returned. (Imagining some crazy world where there is some new possibility between read and write.)

> The problem with PR_POLL_EXCEPT is worth a comment in the
> source code.
> 
> If PR_POLL_EXCEPT is a problem, should we also clear the
> PR_POLL_EXCEPT flag when there is pending write data?
> I guess it's not necessary because PR_XXX will send the
> pending write data, which will fail with a real error
> (not PR_WOULD_BLOCK_ERROR).

That is my understanding. I will add a comment.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Address Wan-Teh's review comments

I did not address (AFAICT) the documentation on how to convert an application from synchronous to asynchronous cert validation. I think it is good enough to make the potential user aware that it is tricky; the actual solutions may vary depending on the application. We can improve this documentation in a future release if necessary.

I also did not change tstclnt to integrate its bad cert hook directly into its auth cert hook, because I want tstclnt to help demonstrate the differences between the synchronous and asynchronous mechanisms.

When we check this NSS release into mozilla-*, we will have to update Gecko to use the new name for the NPN extension.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:bsmith) from comment #3)
> When we check this NSS release into mozilla-*, we will have to update Gecko
> to use the new name for the NPN extension.

It turns out we don't need to do that, because Gecko doesn't call SSL_HandshakeNegotiatedExtension for this extension.

Comment 5

[Kai Engert (:KaiE:)]

Comment on attachment 597540 [details] [diff] [review]
Address Wan-Teh's review comments

no functional changes in the library

the only functional change is in the debug printf in tstclnt
(which doesn't affect mozilla code)

r=kaie

I did not review the text. I will check this in and release it. We can fix typos in comments later, should there be any.

Comment 6

[Kai Engert (:KaiE:)]

Checking in cmd/tstclnt/tstclnt.c;
/cvsroot/mozilla/security/nss/cmd/tstclnt/tstclnt.c,v  <--  tstclnt.c
new revision: 1.69; previous revision: 1.68
done
Checking in lib/ssl/ssl.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v  <--  ssl.h
new revision: 1.49; previous revision: 1.48
done
Checking in lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.163; previous revision: 1.162
done
Checking in lib/ssl/ssl3ext.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ext.c,v  <--  ssl3ext.c
new revision: 1.21; previous revision: 1.20
done
Checking in lib/ssl/sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.94; previous revision: 1.93
done
Checking in lib/ssl/sslsecur.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsecur.c,v  <--  sslsecur.c
new revision: 1.57; previous revision: 1.56
done
Checking in lib/ssl/sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.82; previous revision: 1.81
done
Checking in lib/ssl/sslt.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslt.h,v  <--  sslt.h
new revision: 1.18; previous revision: 1.17
done