Re: Incorrect allocation handling for cryptohash functions with OpenSSL

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

> ... And I wanted to keep track of the type of cryptohash directly in
> the shared structure. ;)

You could also define a shared header, with the rest of the struct being
implementation-specific:

typedef struct pg_cryptohash_ctx
{
pg_cryptohash_type type;

/* implementation-specific data follows */
} pg_cryptohash_ctx;

- Heikki