Re: Incorrect allocation handling for cryptohash functions with OpenSSL

On Fri, Dec 18, 2020 at 11:51:55AM +0200, Heikki Linnakangas wrote:
> On 18/12/2020 11:35, Heikki Linnakangas wrote:
> > 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.
>
> Something like this. Passes regression tests, but otherwise untested.

Interesting. I have looked at that with a fresh mind, thanks for the
idea. This reduces the number of allocations to one making the error
handling a no-brainer, at the cost of hiding the cryptohash type
directly to the caller. I originally thought that this would be
useful as I recall reading cases in the OpenSSL code doing checks on
hash type used, but perhaps that's just some over-engineered thoughts
from my side. I have found a couple of small-ish issues, please see
my comments below.

+ /*
+ * FIXME: this always allocates enough space for the largest hash.
+ * A smaller allocation would be enough for md5, sha224 and sha256.
+ */
I am not sure that this is worth complicating more, and we are not
talking about a lot of memory (sha512 requires 208 bytes, sha224/256
104 bytes, md5 96 bytes with a quick measurement). This makes free()
equally more simple. So just allocating the amount of memory based on
the max size in the union looks fine to me. I would add a memset(0)
after this allocation though.

-#define ALLOC(size) palloc(size)
+#define ALLOC(size) MemoryContextAllocExtended(TopMemoryContext, size, MCXT_ALLOC_NO_OOM)
As the only allocation in TopMemoryContext is for the context, it
would be fine to not use MCXT_ALLOC_NO_OOM here, and fail so as
callers in the backend don't need to worry about create() returning
NULL.

- state->evpctx = EVP_MD_CTX_create();
+ ctx->evpctx = EVP_MD_CTX_create();

- if (state->evpctx == NULL)
+ if (ctx->evpctx == NULL)
{
If EVP_MD_CTX_create() fails, you would leak memory with the context
allocated in TopMemoryContext. So this requires a free of the context
before the elog(ERROR).

+ /*
+ * Make sure that the resource owner has space to remember this
+ * reference. This can error out with "out of memory", so do this
+ * before any other allocation to avoid leaking.
+ */
#ifndef FRONTEND
ResourceOwnerEnlargeCryptoHash(CurrentResourceOwner);
#endif
Right. Good point.

- /* OpenSSL internals return 1 on success, 0 on failure */
+ /* openssl internals return 1 on success, 0 on failure */
It seems to me that this was not wanted.

At the same time, I have taken care of your comment from upthread to
return a failure if the caller passes NULL for the context, and
adjusted some comments. What do you think of the attached?
--
Michael