Add assert policy

[kroeckx]

Fixes: #49

[kroeckx]

So I have some comments about this:

• When do we want to use assertions? Do we want every function to check it's invariants? Pre? Post? Steps? Or don't we want to say when to add it?
• One thing that the document doesn't cover now is that with assert() the code that does the checks is removed in a release build, while the other 2 still execute the code and so have runtime overhead. Maybe that changes when to use which one.

[kroeckx]

A previous discussion about this starts at: https://github.com/openssl/web/pull/43#discussion_r168798577

[levitte]

So I have some comments about this:

• When do we want to use assertions? Do we want every function to check it's invariants? Pre? Post? Steps? Or don't we want to say when to add it?

I wouldn't want to specify that. I can imagine all sorts of situations where that will have to depend on what the code does.

• One thing that the document doesn't cover now is that with assert() the code that does the checks is removed in a release build, while the other 2 still execute the code and so have runtime overhead. Maybe that changes when to use which one.

Isn't that said in the table, where you do say that assert() isn't evaluated in release builds? You could, if you want, add a note that assert() compiles to nothing in release builds. A footnote, perhaps?

[kroeckx]

Isn't that said in the table, where you do say that assert() isn't evaluated in release builds?

That was changed in the fixup commit

[kroeckx]

I think it might be helpful if it contained more guidance. For instance, I think this code could potentially use an assert in the default case:

https://github.com/openssl/openssl/blob/5318c012885a5382eadbf95aa9c1d35664bca819/ssl/quic/quic_tls.c#L795-L809

Should that be just assert(0); before the return RAISE_INTERNAL_ERROR?

[levitte]

Part of an assertion is, at least to me, that it shows the actual condition you're making an assertion on. I that sense, assert(0) is quite meaningless.

[hlandau]

This proposal seems reasonable.

We should use assertions more than we currently do.

In the future we should probably:

• introduce an UNREACHABLE define of some kind for use instead of assert(0). Reads much better and we may want to make it enabled in release builds, since if the control flow should never reach that point there is no performance issue with leaving it enabled.
• replace use of assert with some name we come up with ourselves, so that assertion failure handling can be centralised and controlled in the same way that ossl_assert and OPENSSL_assert are. Taking suggestions for a name.

[t8m]

I would not add assert in the case referenced above - this is not an impossible condition and I think disallowing the code to continue - even in debug builds only - would be IMO wrong.

[hlandau]

It is worth noting that @t8m is right here, the above referenced QUIC_TLS code is not a case where use of an assert would be suitable.

Assertions are either for

• things that should not be possible, and the fact that they are possible implies a serious violation of the invariants under which a piece of code has been designed to operate
• calls made to an internal function which violate a precondition of that function's contract, which reflects a programming error on the part of the caller (which is itself internal)

Most likely, calls to external API functions should in general probably return an error rather than asserting even if the issue is objectively due to the caller violating a precondition and a product of caller error. However there are some cases where we have API behaviour which is effectively assert-like anyway in the case of caller error, for example where we do not do NULL checks on arguments; those cases could at least be asserted if error returns are not feasible.

The main risk of assertions is that people fail to understand when they should and should not be used. But that can be caught in code review and clarified in this policy.

[kroeckx]

I was under the impression that the code I pointed to fall under the first case, but I don't know enough about the code. But it's my understanding that you both agree that for cases that should be impossible to reach in a switch statement, using an assert() is something that could be added, because you can't ossl_assert() anymore, the condition was already checked.

It's unclear what @levitte wants to see instead. Is the problem with the 0? #49 had as example: assert("We forgot to handle a value" == NULL); Would you like to see something like that instead?

[levitte]

I would find something like assert(UNREACHABLE) acceptable, because that prints something sufficiently meaningful, at least to get an idea of what's going wrong. assert(0) is utter nonsense.

So yeah, that's correct, 0 is meaningless in my view, I want something sufficiently expressive.

[t8m]

In case of this particular switch() we do not really know if some other error cannot be returned by the TLS handshake layer. It very well might be. It would be an internal error for the connection. It would not be a reason to crash the app even in the debug mode.

This definitely does not belong to the first case.

[hlandau]

I would find something like assert(UNREACHABLE) acceptable, because that prints something sufficiently meaningful, at least to get an idea of what's going wrong. assert(0) is utter nonsense.

So yeah, that's correct, 0 is meaningless in my view, I want something sufficiently expressive.

I think we should just have a dedicated UNREACHABLE; macro.

[kroeckx]

In case of this particular switch() we do not really know if some other error cannot be returned by the TLS handshake layer. It very well might be. It would be an internal error for the connection. It would not be a reason to crash the app even in the debug mode.

I'm not really interested in this case, just on how we want to use asserts.

There are cases where we don't expect to ever come, but we're not sure that either now or in the future we might reach that. For instance, with code changed we could at a later point reach that. We probably want to verify that it's doing the correct thing. With an assert in debug mode, we could realise that we might need to change code.

[t8m]

Yes, that is definitely a case for debug-only asserts.

[levitte]

Since we still have varying opinions on the subject of use, I suggest that we leave that vague for now, to be revisited when we have more experience trying to apply this policy. We could actually schedule a future re-evaluation (which I think we should for most policies)

[kroeckx]

So you suggest to leave it as is?

[levitte]

Yes, I do. I believe that we'll get to a better place if that gets developed organically and with experience, rather than trying to decide now (which is obviously not so easy, 'cause opinions vary). It will be frustrating, for sure, but I believe it will either way we go (if we decide something strict now, there is bound to be lots of frustrations over cases where the strict ruling just doesn't match the reality of the case).

[t8m]

Starting the vote for Add assert policy at commit 8cd764e

[t8m]

Vote: [+1]

[t8m]

@openssl/otc please vote here

[mattcaswell]

Vote: [+1]

[levitte]

Vote: [+1]

(voted by email too)

[beldmit]

Vote: [+1]

[paulidale]

Vote: [0]

[slontis]

Vote [+1]

[mspncp]

Vote [+1]

[kroeckx]

Voting +1

[t8m]

Vote is closed, the policy change is approved.

Topic: Add assert policy as of commit 8cd764e
Proposed by: Tomas Mraz
Issue link: https://github.com/openssl/technical-policies/pull/72
Public: yes
Opened: 2023-09-26
Closed: 2023-10-06
Accepted:  yes  (for: 8, against: 0, abstained: 1, not voted: 2)

  Dmitry     [+1 ]
  Matt       [+1]
  Pauli      [ 0]
  Tim        [  ]
  Hugo       [  ]
  Richard    [+1]
  Shane      [+1]
  Tomas      [+1]
  Kurt       [+1]
  Matthias   [+1]
  Nicola     [+1]

[t8m]

The assert policy was merged into the repository.

[hlandau]

Vote: [+1]

[t-j-h]

Vote [0]