Re: OpenSSL 1.1 breaks configure and more

On 08/30/2016 03:26 AM, Andreas Karlsson wrote:
> On 08/26/2016 11:31 AM, Heikki Linnakangas wrote:
>> On 07/05/2016 04:46 PM, Andreas Karlsson wrote:
>>> @@ -280,8 +287,9 @@ px_find_digest(const char *name, PX_MD **res)
>>> digest = px_alloc(sizeof(*digest));
>>> digest->algo = md;
>>>
>>> - EVP_MD_CTX_init(&digest->ctx);
>>> - if (EVP_DigestInit_ex(&digest->ctx, digest->algo, NULL) == 0)
>>> + digest->ctx = EVP_MD_CTX_create();
>>> + EVP_MD_CTX_init(digest->ctx);
>>> + if (EVP_DigestInit_ex(digest->ctx, digest->algo, NULL) == 0)
>>> return -1;
>>>
>>> h = px_alloc(sizeof(*h));
>>
>> Now that we're calling EVP_MD_CTX_create((), which allocates memory, are
>> we risking memory leaks? It has always been part of the contract that
>> you have to call px_md_free(), for any context returned by
>> px_find_digest(), but I wonder just how careful we have been about that.
>> Before this, you would probably get away with it without leaking, if the
>> digest implementation didn't allocate any extra memory or other resources.
>>
>> At least pg_digest and try_unix_std functions call px_find_digest(), and
>> then do more palloc()s which could elog() if you run out of memory,
>> leaking th digest struct. Highly unlikely, but I think it would be
>> fairly straightforward to reorder those calls to eliminate the risk, so
>> we probably should.
>
> Since px_find_digest() calls palloc() later in the function there is a
> slim possibility of memory leaks.

Yep. That palloc() would be easy to move to before the EVP_MD_CTX_new()
call. And some of the calls to px_find_digest() could likewise be
rearranged. But there are some more complicated callers. pgp_encrypt(),
for example, builds a pipeline of multiple "mbuf" filters, and one of
those filters uses px_find_digest().

> How do we generally handle that things
> not allocated with palloc() may leak when something calls elog()?

There's the ResourceOwner mechanism, see src/backend/utils/resowner/.
That would be the proper way to do this. Call
RegisterResourceReleaseCallback() when the context is allocated, and
have the callback free it. One pitfall to watch out for is that
RegisterResourceReleaseCallback() itself calls palloc(), and can error
out, so you have to do things in such an order that you don't leak in
that case either.

Want to take a stab at that?

Another approach is put each allocated context in a list or array in a
global variable, and to register a callback to be called at
end-of-(sub)transaction, which closes all the contexts. But the resource
owner mechanism is probably easier.

There's also PG_TRY-CATCH, that you could maybe use in the callers of
px_find_digest(), to make sure they call px_free_digest() even on error.
But that also seems difficult to use with the pgp_encrypt() pipeline.

> I have attached new versions of the patches which are rebased on master,
> with slightly improves error handling in px_find_digest(), and handles
> the deprecation of ASN1_STRING_data().

Thanks!

PS. I just remembered that I've wanted to refactor the pgcrypto calls
for symmetric encryption to use the newer EVP API for some time, and
even posted a patch for that
(https://www.postgresql.org/message-id/561274F1.1030000@iki.fi) I
dropped the ball back then, but I think I'll go ahead and do that now,
once we get these other OpenSSL changes in.

- Heikki