Re: OpenSSL 1.1 breaks configure and more - Mailing list pgsql-hackers

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