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