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
