Check NULL pointers or not...

[Dr Paul Dale]

I’d prefer option 1 or the middle ground.  I’ve lost count of the number of times I’ve seen programs crashing in the crypto library which required mammoth debugging efforts to irrefutably demonstrate that the caller is at fault rather than the crypto library :(

Option 1 would be preferable from this point of view but it can cause a performance hit — most of the time it wouldn’t matter but when it does it would be a big deal.  The middle ground doesn’t entail any performance loss in production code (it does in debug but that shouldn’t be relevant).


Pauli
-- 
Dr Paul Dale | Distinguished Architect | Cryptographic Foundations 
Phone +61 7 3031 7217
Oracle Australia




> On Wed, Nov 27, 2019 at 10:38:41AM +0100, Richard Levitte wrote:
>> Depending on who you're asking, you get different responses.
>> 
>> The topic is: should we check pointers coming from applications before
>> using them or not?
>> 
>> There are two positions on this:
>> 
>> 1. Yes we should so we don't crash processes unnecessarily, and we
>>   should return an error if pointers are NULL (unless that's valid
>>   input, such as for the freeing functions).  There is a generic
>>   error reason code for it, ERR_R_PASSED_NULL_PARAMETER.
>> 
>> 2. No we should not, as we don't cater for user programming errors.
>>   Also, it allows us to catch our own errors early.
>> 
>> For a while, our pendulum was going with option 2, but lately, we seem
>> to go back to option 1.
>> 
>> Both options have their pros and cons:
>> 
>> 1. cons: there's the very real possibility that users don't check for
>>         errors as they should, and things go wrong further on,
>>         sometimes in ways where the actual cause is very hard to
>>         figure out.
>>   pros: programs will not spuriously crash because we didn't catch an
>>         internal corner case in our tests.
>> 
>> 2. cons: programs may crash, sometimes due to our own mistakes,
>>         sometimes due to user errors.
>>   pros: some very subtle bugs are found, either quicker or at all.
>>         An actual case is PR 7630 [1], which was uncovered because
>>         there was a crash due to a NULL being used (this was
>>         ultimately due to users not checking errors).
>> 
>> As a middle ground, and perhaps a way to satify both camps, we could
>> use ossl_assert() in our checks for input NULL, as follows:
>> 
>>    if (!ossl_assert(ptr != NULL)) {
>>        ERR_raise(ERR_LIB_WHATEVER, ERR_R_PASSED_NULL_PARAMETER);
>>        return 0;
>>    }
>> 
>> This means that in a debug build, there may be crashes due to pointers
>> being NULL, while in a release build, the caller gets an error.
>> 
>> Thoughts?