On 09/05/2016 03:12 AM, Andreas Karlsson wrote:
> On 08/30/2016 08:42 AM, Heikki Linnakangas wrote:
>> 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.
>
> Sure, I have attached a patch where I try to use it.
Thanks! Unfortunately the callback mechanism is a bit more complicated
to use than that. The list of registered callbacks is global, so the
callback gets called for every ResourceOwner that's closed, not just the
one that was active when you registered it. Also, unregistering the
callback from within the callback is not safe. I fixed those things in
the (first) attached patch.
On 09/05/2016 03:23 AM, Tom Lane wrote:
> Judging by the number of people who have popped up recently with their
> own OpenSSL 1.1 patches, I think there is going to be a lot of demand for
> back-patching some sort of 1.1 support into our back branches. All this
> talk of refactoring does not sound very back-patchable. Should we be
> thinking of what we can extract that is back-patchable?
Yes, I think you're right.
The minimum set of changes is the first of the attached patches. It
includes Andreas' first patch, with the configure changes and other
changes needed to compile, and a working version of the resourceowner
callback mechanism to make sure we don't leak OpenSSL handles in pgcrypto.
Maybe we could get away without the resourceowner mechanism, and just
accept the minor memory leaks on the rare error case (out-of-memory
might be the only situation where that happen). Or #ifdef it so that we
keep the old embed-in-palloced-struct approach for OpenSSL versions
older than 1.1. But on the whole, I'd prefer to keep the code the same
in all branches.
The second patch attached here includes Andreas' second and third
patches, to silence deprecation warnings. That's not strictly required,
but seems safe enough that I think we should back-patch that too. It
needs one additional #ifdef version check in generate_dh_params(), if we
want it to still work with OpenSSL 0.9.7, but that's easy. I'm assuming
we want to still support it in back-branches, even though we just
dropped it from master.
- Heikki
