394199 - Always set out param in pkix_CertSelector_Match_BasicConstraint

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Kai Engert (:KaiE:)]

The implementation of pkix_CertSelector_Match_BasicConstraint uses a boolean out parameter pResult.

The function does not init the out parameter.
It will set the out parameter only in some scenarios. 

Julien said:
"I think you found a bug. The output parameter pResult should be set in all cases where the function returns no error. The easiest way to fix it in this case is to set it to PR_TRUE at the beginning of the function, but the most logical is to set it in each place where there is a test that causes a validation pass status. "


I personally prefer the approach to set the value to TRUE initially, and keep the current mechanism to set to FALSE on each error exit.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

The function returns a success/failure indicator, and it has an output 
parameter.  It should not make ANY change to the output parameter unless
it returns success, and it should ALWAYS set the output parameter when it
returns success.

Comment 3

[Kai Engert (:KaiE:)]

(In reply to comment #2)
> The function returns a success/failure indicator, and it has an output 
> parameter.  It should not make ANY change to the output parameter unless
> it returns success, and it should ALWAYS set the output parameter when it
> returns success.


Nelson, when a function returns success, I agree, it should always set all output parameters.

But I disagree with your other request. I'm very surprised you request that a function shall never modify any out parameters in the failure scneario. I find that a very strict and uncommon proposal.

I think we should strive for simplicity.

A caller should never assume that a non-const pointer parameter shall be preserved in the failure case.

A function should be allowed to use the out parameters as variables while it does its job.

I'm used to complex functions that have many exit scenarios. I think it is very common to have a function initialize all output parameters at the beginning.

In my understanding it's the caller's responsibility to only make use of parameters on a successful return, and it should ignore any values on failure.

(Of course, it's a function's responsibility to clean up all allocated things assigned to out param pointers, but that's obvious.)

Nelson, please reconsider the patch.

Comment 4

[Julien Pierre]

Comment on attachment 278811 [details] [diff] [review]
Patch v1

r+, as discussed by e-mail earlier.

Comment 5

[Julien Pierre]

Kai, Nelson,

I think it's OK to change the output parameter regardless of success/failure. This is not an error code. The caller is only supposed to look at it if the function succeeded. If there is a failure and the caller is looking at the output parameter value, it is a bug in the caller anyway, and the behavior of the function WRT output parameters is undefined.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

I think it is reasonable for a caller to initialize a value in a variable,
then pass the address of that variable to a function, for use as an 
output parameter, and then after the function returns, if it has failed,
to expect the value in that variable to be as before.  This is quite common.

I'm willing to define this function as not following that principle, but 
the function's block comments need to call attention to it.  

Comment 7

[Kai Engert (:KaiE:)]

I'll add a comment saying "on failure the output parameters of this function are undefined".

Comment 8

[Julien Pierre]

Yes, that's fine. We can never have too much API documentation. I think in general when there is no explicit statement about output parameters for failures, it means they are undefined. Ie. the documentation need is more pressing when the output parameters actually mean something in failure cases than when they don't. But we don't want the callers to make any incorrect assumptions, so we should doc it.

Comment 9

[Kai Engert (:KaiE:)]

Attachment: Patch v1 plus comment

This is the patch I'll check in.

Comment 10

[Kai Engert (:KaiE:)]

fixed

/cvsroot/mozilla/security/nss/lib/libpkix/pkix/certsel/pkix_certselector.c,v  <--  pkix_certselector.c
new revision: 1.4; previous revision: 1.3