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
