Re: Incorrect allocation handling for cryptohash functions with OpenSSL

On 18/12/2020 12:55, Heikki Linnakangas wrote:
> On 18/12/2020 12:10, Michael Paquier wrote:
>> On Fri, Dec 18, 2020 at 11:35:14AM +0200, Heikki Linnakangas wrote:
>>> pg_cryptohash_create() is now susceptible to leaking memory in
>>> TopMemoryContext, if the allocations fail. I think the attached should fix
>>> it (but I haven't tested it at all).
>>
>> Yeah, you are right here.  If the second allocation fails the first
>> one would leak.  I don't think that your suggested fix is completely
>> right though because it ignores that the callers of
>> pg_cryptohash_create() in the backend expect an error all the time, so
>> it could crash.
> 
> Ah, right.
> 
>> Perhaps we should just bite the bullet and switch the
>> OpenSSL and fallback implementations to use allocation APIs that never
>> cause an error, and always return NULL?  That would have the advantage
>> to be more consistent with the frontend that relies in malloc(), at
>> the cost of requiring more changes for the backend code where the
>> _create() call would need to handle the NULL case properly.  The
>> backend calls are already aware of errors so that would not be
>> invasive except for the addition of some elog(ERROR) or similar, and
>> we could change the fallback implementation to use palloc_extended()
>> with MCXT_ALLOC_NO_OOM.
> 
> -1. On the contrary, I think we should reduce the number of checks
> needed in the callers, and prefer throwing errors, if possible. It's too
> easy to forget the check, and it makes the code more verbose, too.
> 
> In fact, it might be better if pg_cryptohash_init() and
> pg_cryptohash_update() didn't return errors either. If an error happens,
> they could just set a flag in the pg_cryptohash_ctx, and
> pg_cryptohash_final() function would return the error. That way, you
> would only need to check for error return in the call to
> pg_cryptohash_final().

BTW, it's a bit weird that the pg_cryptohash_init/update/final() 
functions return success, if the ctx argument is NULL. It would seem 
more sensible for them to return an error. That way, if a caller forgets 
to check for NULL result from pg_cryptohash_create(), but correctly 
checks the result of those other functions, it would catch the error. In 
fact, if we documented that pg_cryptohash_create() can return NULL, and 
pg_cryptohash_final() always returns error on NULL argument, then it 
would be sufficient for the callers to only check the return value of 
pg_cryptohash_final(). So the usage pattern would be:

ctx = pg_cryptohash_create(PG_MD5);
pg_cryptohash_inti(ctx);
pg_update(ctx, data, size);
pg_update(ctx, moredata, size);
if (pg_cryptohash_final(ctx, &hash) < 0)
     elog(ERROR, "md5 calculation failed");
pg_cryptohash_free(ctx);

- Heikki