68097 - ssl API inconsistent use of SECStatus vs int

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

There are many functions in ssl.h that are declared to return
type int, which actually return type SECStatus.

Similarly, in ssl.h there are typedefs for pointers to functions
that return int  which should be pointers to functions that
return SECStatus.

I propose to change ssl.h to use SECStatus instead of int in
the declarations of all functions and typedefs that really
should use SECStatus.

This will eliminate numerous warnings.  For some of our customers
who use ssl.h, it may also create some new warnings.  Customer
code that includes callback functions that were defined to return
int (to agree with the typedef in ssl.h) will generate a warning
that it doesn't agree with the typedef.  This will be apparent
in places where the address of the customer's function is passed 
to an SSL function that registers the callback.  Customers who
don't like those warnings can trivially eliminate them.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Proposed patch to SSL library and test programs

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

I invite review of the attached patch.

Comment 3

[Robert Relyea]

Now is a good time to clean these up. All the functions you've changed look like
they are appropriate.

I approve the changes.

bob

Comment 4

[John G. Myers]

SSL_SecurityStatus(), SSL_AuthCertificateHook(), etc. will return SECFailure 
without setting an error code.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

John,  the functions you mentioned only return SECFailure when
an underlying function that they've called returns an error.
In every case, it is the responsibility of the underlying function
to set the error code.

So, you have no objections to the conversion from int to SECStatus?

Comment 6

[John G. Myers]

The functions I've mentioned return SECFailure when ssl_FindSocket() (and 
PR_GetIdentitiesLayer) returns NULL.  NULL returns from these functions are not 
errors and do not set the error code.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

You're right.  ssl_FindSocket() should set an error when 
PR_GetIdentitiesLayer() returns NULL.  I'll make another
bug out of that.  Failure to set an error code
is a bug regardless of the return value type.

This bug is about the types for function return values.  
You apparently have no objection to the change proposed 
in this bug.  

Comment 8

[John G. Myers]

Correct, I have no objection.

I would like further auditing to make sure that PR_WOULD_BLOCK_ERROR is set 
every time a function returns SECWouldBlock.  I would be happier if the 
functions returned a PRStatus instead of a SECStatus.  It is error prone to have 
two different ways of signalling the same condition.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

PR_WOULD_BLOCK_ERROR and SecWouldBlock are not strictly 
synonymous.
There are places where it is appropriate to map one to the
other, and places where it is not.  
I agree that it should be done in all places where appropriate.

The patch attached (above) has been checked in.

Comment 10

[John G. Myers]

How are PR_WOULD_BLOCK_ERROR and SecWouldBlock different?  When would mapping 
them be inappropriate?