Re: Incorrect allocation handling for cryptohash functions with OpenSSL

On Fri, Dec 18, 2020 at 6:04 AM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> 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);

TBH, I think there's no point in return an error here at all, because
it's totally non-specific. You have no idea what failed, just that
something failed. Blech. If we want to check that ctx is non-NULL, we
should do that with an Assert(). Complicating the code with error
checks that have to be added in multiple places that are far removed
from where the actual problem was detected stinks.

--
Robert Haas
EDB: http://www.enterprisedb.com