|From:||Michael Paquier <michael(at)paquier(dot)xyz>|
|To:||Heikki Linnakangas <hlinnaka(at)iki(dot)fi>|
|Cc:||Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>|
|Subject:||Re: Incorrect allocation handling for cryptohash functions with OpenSSL|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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
- 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.
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?
|Next Message||Amit Kapila||2020-12-19 06:40:30||Re: Single transaction in the tablesync worker?|
|Previous Message||Michael Paquier||2020-12-19 05:00:15||Re: Commit fest manager for 2021-01|