Re: Incorrect allocation handling for cryptohash functions with OpenSSL

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. 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.

> BTW, looking at pg_cryptohash_ctx and pg_cryptohash_state, why do we need
> two structs? They're both allocated and controlled by the cryptohash
> implementation. It would seem simpler to have just one.

Depending on the implementation, the data to track can be completely
different, and this split allows to know about the resowner dependency
only in the OpenSSL part of cryptohashes, without having to include
this knowledge in neither cryptohash.h nor in the fallback
implementation that can just use palloc() in the backend.
--
Michael