Re: Incorrect allocation handling for cryptohash functions with OpenSSL

On Fri, Dec 18, 2020 at 01:04:27PM +0200, Heikki Linnakangas 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.

Okay.

> 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);

I'd rather keep the init and update routines to return an error code
directly. This is more consistent with OpenSSL (note that libnss does
not return error codes for the init, update and final but it is
possible to grab for errors then react on that). And we have even in
our tree code paths a-la-pgcrypto that have callbacks for each phase
with some processing in-between. HMAC also gets a bit cleaner by
keeping this flexibility IMO.
--
Michael