Fix "safe" scan-build issues

Fix "safe" scan-build issues

[ekr-rietveld, 2016-03-13 23:42:31]

MT, these are the changes that I think are "safe" in the sense that they don't
seem to implicate serious bugs.

[mt, 2016-03-14 00:09:08]

Patch Set 1 LGTM; most of these extra changes would just make things a tiny bit
cleaner, your call.

https://codereview.appspot.com/283630043/diff/1/cmd/strsclnt/strsclnt.c
File cmd/strsclnt/strsclnt.c (right):

https://codereview.appspot.com/283630043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:330: typedef int startFn(void *a, void *b, int c);
s/int/SECStatus

I noticed a few of these; seemed like a simple change to make.

https://codereview.appspot.com/283630043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:550: int
s/int/SECStatus

https://codereview.appspot.com/283630043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:711: int
s/int/SECStatus

https://codereview.appspot.com/283630043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:723: int                 rv 		= SECSuccess;
s/int/SECStatus

https://codereview.appspot.com/283630043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:857: return SECSuccess;
I think that you want to return rv here instead.

https://codereview.appspot.com/283630043/diff/1/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/283630043/diff/1/lib/ssl/ssl3con.c#newcode3285
lib/ssl/ssl3con.c:3285: return rv;
You could return SECSuccess here and then remove the initializer on rv on 3279.

https://codereview.appspot.com/283630043/diff/1/lib/ssl/ssl3con.c#newcode9136
lib/ssl/ssl3con.c:9136: 
If you added a PORT_Assert(!haveXmitBufLock); and
PORT_Assert(!haveSpecWriteLock); to this line (and to 9118 above), would that be
better?  It seems like the booleans are updated only to be consistent.  An
optimized build will recognize that they aren't used and avoid the extra set (I
hope).

https://codereview.appspot.com/283630043/diff/1/lib/ssl/ssl3con.c#newcode9182
lib/ssl/ssl3con.c:9182: goto loser; /* alert_loser */
Why not also alert_loser here?

https://codereview.appspot.com/283630043/diff/1/lib/ssl/ssl3con.c#newcode9196
lib/ssl/ssl3con.c:9196: goto alert_loser;
This is a substantive change, but a good one.

https://codereview.appspot.com/283630043/diff/1/lib/ssl/ssl3con.c#newcode11270
lib/ssl/ssl3con.c:11270: return rv;
return SECSuccess;

https://codereview.appspot.com/283630043/diff/1/lib/ssl/ssl3ext.c
File lib/ssl/ssl3ext.c (right):

https://codereview.appspot.com/283630043/diff/1/lib/ssl/ssl3ext.c#newcode1660
lib/ssl/ssl3ext.c:1660: }
Nice.  Can't imagine that it ever happened, but still.

[ekr-rietveld, 2016-03-14 00:20:13]

Should I land these in advance of fixing the more substantive ones?

https://codereview.appspot.com/283630043/diff/1/cmd/strsclnt/strsclnt.c
File cmd/strsclnt/strsclnt.c (right):

https://codereview.appspot.com/283630043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:857: return SECSuccess;
On 2016/03/14 00:09:07, mt wrote:
> I think that you want to return rv here instead.

good catch

https://codereview.appspot.com/283630043/diff/1/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/283630043/diff/1/lib/ssl/ssl3con.c#newcode9182
lib/ssl/ssl3con.c:9182: goto loser; /* alert_loser */
On 2016/03/14 00:09:07, mt wrote:
> Why not also alert_loser here?

We haven't verified that this doesn't have a v2 version # yet. Maybe we should
do that first and then check the state.

[ekr-rietveld, 2016-03-14 13:29:15]

https://codereview.appspot.com/283630043/diff/1/cmd/strsclnt/strsclnt.c
File cmd/strsclnt/strsclnt.c (right):

https://codereview.appspot.com/283630043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:330: typedef int startFn(void *a, void *b, int c);
On 2016/03/14 00:09:07, mt wrote:
> s/int/SECStatus
> 
> I noticed a few of these; seemed like a simple change to make.

Done.

https://codereview.appspot.com/283630043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:550: int
On 2016/03/14 00:09:07, mt wrote:
> s/int/SECStatus

Done.

https://codereview.appspot.com/283630043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:711: int
On 2016/03/14 00:09:07, mt wrote:
> s/int/SECStatus

Done.

https://codereview.appspot.com/283630043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:723: int                 rv 		= SECSuccess;
On 2016/03/14 00:09:07, mt wrote:
> s/int/SECStatus

Done.

https://codereview.appspot.com/283630043/diff/1/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/283630043/diff/1/lib/ssl/ssl3con.c#newcode3285
lib/ssl/ssl3con.c:3285: return rv;
On 2016/03/14 00:09:07, mt wrote:
> You could return SECSuccess here and then remove the initializer on rv on
3279.

Done.

https://codereview.appspot.com/283630043/diff/1/lib/ssl/ssl3con.c#newcode9136
lib/ssl/ssl3con.c:9136: 
On 2016/03/14 00:09:07, mt wrote:
> If you added a PORT_Assert(!haveXmitBufLock); and
> PORT_Assert(!haveSpecWriteLock); to this line (and to 9118 above), would that
be
> better?  It seems like the booleans are updated only to be consistent.  An
> optimized build will recognize that they aren't used and avoid the extra set
(I
> hope).

My sense is that it's better to have less code here.

[franziskus, 2016-03-15 13:32:18]

lgtm in general. But please have a look at the comments.

https://codereview.appspot.com/283630043/diff/20001/cmd/tstclnt/tstclnt.c
File cmd/tstclnt/tstclnt.c (right):

https://codereview.appspot.com/283630043/diff/20001/cmd/tstclnt/tstclnt.c#new...
cmd/tstclnt/tstclnt.c:1582: PR_Poll(pollset, 1, PR_INTERVAL_NO_TIMEOUT);
I don't think we should remove the assignment here but rather check the result.
At least something like if (CC == -1) { /* print and set error */ }

https://codereview.appspot.com/283630043/diff/20001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/283630043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9184: goto loser; /* alert_loser */
why not leaving desc in and goto alert_loser;?

[ekr-rietveld, 2016-03-15 13:35:22]

https://codereview.appspot.com/283630043/diff/20001/cmd/tstclnt/tstclnt.c
File cmd/tstclnt/tstclnt.c (right):

https://codereview.appspot.com/283630043/diff/20001/cmd/tstclnt/tstclnt.c#new...
cmd/tstclnt/tstclnt.c:1582: PR_Poll(pollset, 1, PR_INTERVAL_NO_TIMEOUT);
On 2016/03/15 13:32:18, franziskus wrote:
> I don't think we should remove the assignment here but rather check the
result.
> At least something like if (CC == -1) { /* print and set error */ }

OK. I figured this was harmless, but I can do that.

https://codereview.appspot.com/283630043/diff/20001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/283630043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9184: goto loser; /* alert_loser */
On 2016/03/15 13:32:18, franziskus wrote:
> why not leaving desc in and goto alert_loser;?

That was my comment to Tim as well. I will clean this when I rebase onto the new
code.