[erlang-patches] Fix SSL ETS table element leak and error cleanup

[unknown]

Hi list,
The SSL library maintains an internal table of CA certificates
(ssl_otp_cacertificate_db). This is supposed to be cleaned up when the
last connection using a certificate closes, however there's two problems
in R16B02 (and in the current master branch on github):

* When CA certificates are provided as binary blobs, rather than by
filename (ie, #ssl_options.cacerts is set, but #ssl_options.cacertfile
is not) the cleanup never occurs due to an incorrect pattern match in
tls_connection:handle_trusted_certs_db/1. This causes the table to grow
unchecked because each connection adds a new entry.

* When the process exits abnormally, tls_connection:terminate/1 is never
called because the trap_exit process flag is not set and so similarly
the table (and everything else cleaned in terminate/1, for that matter)
is not cleaned up. This doesn't affect "normal" termination caused by
the connection closing because terminate/1 is called explicitly from
handle_sync_event/4, rather that relying on gen_fsm's automatic calling.

Fixes for both are here:

git fetch git://github.com/bernardd/otp ssl_cert_cache_fix
https://github.com/bernardd/otp/compare/erlang:master...ssl_cert_cache_fix

Credit goes to my colleague Nick Marino for doing the initial legwork to
track this down.

Cheers,

Bernard

________________________________

This e-mail and any attachments are confidential. If it is not intended for you, please notify the sender, and please erase and ignore the contents.

[unknown]

Thank you for your contribution!

You could, in addition to the email and proper links, also create a
pullrequest for the patch.
That way it will be processed into our system.

I will create a pullrequest for this.
Thank you!



On 2013-12-10 05:44, Bernard Duggan wrote:
> Hi list,
> The SSL library maintains an internal table of CA certificates
> (ssl_otp_cacertificate_db). This is supposed to be cleaned up when the
> last connection using a certificate closes, however there's two problems
> in R16B02 (and in the current master branch on github):
>
> * When CA certificates are provided as binary blobs, rather than by
> filename (ie, #ssl_options.cacerts is set, but #ssl_options.cacertfile
> is not) the cleanup never occurs due to an incorrect pattern match in
> tls_connection:handle_trusted_certs_db/1. This causes the table to grow
> unchecked because each connection adds a new entry.
>
> * When the process exits abnormally, tls_connection:terminate/1 is never
> called because the trap_exit process flag is not set and so similarly
> the table (and everything else cleaned in terminate/1, for that matter)
> is not cleaned up. This doesn't affect "normal" termination caused by
> the connection closing because terminate/1 is called explicitly from
> handle_sync_event/4, rather that relying on gen_fsm's automatic calling.
>
> Fixes for both are here:
>
> git fetch git://github.com/bernardd/otp ssl_cert_cache_fix
> https://github.com/bernardd/otp/compare/erlang:master...ssl_cert_cache_fix
>
>
> Credit goes to my colleague Nick Marino for doing the initial legwork to
> track this down.
>
> Cheers,
>
> Bernard
>
> ________________________________
>
> This e-mail and any attachments are confidential. If it is not
> intended for you, please notify the sender, and please erase and
> ignore the contents.
> _______________________________________________
> erlang-patches mailing list
> erlang-patches
> http://erlang.org/mailman/listinfo/erlang-patches

--
/Henrik Nord Erlang/OTP

[unknown]

Hi!

Patch solves two problems, one (trap exit) is already solved in R16B03.

The other part of the patch we do want, but it should be ammended nice
with a test case.



On 2013-12-10 05:44, Bernard Duggan wrote:
> Hi list,
> The SSL library maintains an internal table of CA certificates
> (ssl_otp_cacertificate_db). This is supposed to be cleaned up when the
> last connection using a certificate closes, however there's two problems
> in R16B02 (and in the current master branch on github):
>
> * When CA certificates are provided as binary blobs, rather than by
> filename (ie, #ssl_options.cacerts is set, but #ssl_options.cacertfile
> is not) the cleanup never occurs due to an incorrect pattern match in
> tls_connection:handle_trusted_certs_db/1. This causes the table to grow
> unchecked because each connection adds a new entry.
>
> * When the process exits abnormally, tls_connection:terminate/1 is never
> called because the trap_exit process flag is not set and so similarly
> the table (and everything else cleaned in terminate/1, for that matter)
> is not cleaned up. This doesn't affect "normal" termination caused by
> the connection closing because terminate/1 is called explicitly from
> handle_sync_event/4, rather that relying on gen_fsm's automatic calling.
>
> Fixes for both are here:
>
> git fetch git://github.com/bernardd/otp ssl_cert_cache_fix
> https://github.com/bernardd/otp/compare/erlang:master...ssl_cert_cache_fix
>
>
> Credit goes to my colleague Nick Marino for doing the initial legwork to
> track this down.
>
> Cheers,
>
> Bernard
>
> ________________________________
>
> This e-mail and any attachments are confidential. If it is not
> intended for you, please notify the sender, and please erase and
> ignore the contents.
> _______________________________________________
> erlang-patches mailing list
> erlang-patches
> http://erlang.org/mailman/listinfo/erlang-patches

--
/Henrik Nord Erlang/OTP

[unknown]

Hi Henrik,
I've trimmed the patch down to just the one remaining problem and (after much boggling over the SSL unit test system) added a unit test that checks the fix. I've rebased it to 'maint' and created a pull request. You can see the change at:

https://github.com/bernardd/otp/compare/erlang:maint...ssl_cert_cache_fix
https://github.com/bernardd/otp/compare/erlang:maint...ssl_cert_cache_fix.patch

Cheers,

Bernard
________________________________________
From: Henrik Nord [henrik]
Sent: Thursday, 6 February 2014 3:27 AM
To: Bernie Duggan; erlang patches
Subject: Re: [erlang-patches] Fix SSL ETS table element leak and error cleanup

Hi!

Patch solves two problems, one (trap exit) is already solved in R16B03.

The other part of the patch we do want, but it should be ammended nice
with a test case.



On 2013-12-10 05:44, Bernard Duggan wrote:
> Hi list,
> The SSL library maintains an internal table of CA certificates
> (ssl_otp_cacertificate_db). This is supposed to be cleaned up when the
> last connection using a certificate closes, however there's two problems
> in R16B02 (and in the current master branch on github):
>
> * When CA certificates are provided as binary blobs, rather than by
> filename (ie, #ssl_options.cacerts is set, but #ssl_options.cacertfile
> is not) the cleanup never occurs due to an incorrect pattern match in
> tls_connection:handle_trusted_certs_db/1. This causes the table to grow
> unchecked because each connection adds a new entry.
>
> * When the process exits abnormally, tls_connection:terminate/1 is never
> called because the trap_exit process flag is not set and so similarly
> the table (and everything else cleaned in terminate/1, for that matter)
> is not cleaned up. This doesn't affect "normal" termination caused by
> the connection closing because terminate/1 is called explicitly from
> handle_sync_event/4, rather that relying on gen_fsm's automatic calling.
>
> Fixes for both are here:
>
> git fetch git://github.com/bernardd/otp ssl_cert_cache_fix
> https://github.com/bernardd/otp/compare/erlang:master...ssl_cert_cache_fix
>
>
> Credit goes to my colleague Nick Marino for doing the initial legwork to
> track this down.
>
> Cheers,
>
> Bernard
>
> ________________________________
>
> This e-mail and any attachments are confidential. If it is not
> intended for you, please notify the sender, and please erase and
> ignore the contents.
> _______________________________________________
> erlang-patches mailing list
> erlang-patches
> http://erlang.org/mailman/listinfo/erlang-patches

--
/Henrik Nord Erlang/OTP


________________________________

This e-mail and any attachments are confidential. If it is not intended for you, please notify the sender, and please erase and ignore the contents.

[unknown]

Thanks!

As we are already passed the codestop for 17.0 we will put this in the
backlog for 17.1.

Thank you for your contribution!

On 2014-03-31 13:38, Bernie Duggan wrote:
> Hi Henrik,
> I've trimmed the patch down to just the one remaining problem and (after much boggling over the SSL unit test system) added a unit test that checks the fix. I've rebased it to 'maint' and created a pull request. You can see the change at:
>
> https://github.com/bernardd/otp/compare/erlang:maint...ssl_cert_cache_fix
> https://github.com/bernardd/otp/compare/erlang:maint...ssl_cert_cache_fix.patch
>
> Cheers,
>
> Bernard
> ________________________________________
> From: Henrik Nord [henrik]
> Sent: Thursday, 6 February 2014 3:27 AM
> To: Bernie Duggan; erlang patches
> Subject: Re: [erlang-patches] Fix SSL ETS table element leak and error cleanup
>
> Hi!
>
> Patch solves two problems, one (trap exit) is already solved in R16B03.
>
> The other part of the patch we do want, but it should be ammended nice
> with a test case.
>
>
>
> On 2013-12-10 05:44, Bernard Duggan wrote:
>> Hi list,
>> The SSL library maintains an internal table of CA certificates
>> (ssl_otp_cacertificate_db). This is supposed to be cleaned up when the
>> last connection using a certificate closes, however there's two problems
>> in R16B02 (and in the current master branch on github):
>>
>> * When CA certificates are provided as binary blobs, rather than by
>> filename (ie, #ssl_options.cacerts is set, but #ssl_options.cacertfile
>> is not) the cleanup never occurs due to an incorrect pattern match in
>> tls_connection:handle_trusted_certs_db/1. This causes the table to grow
>> unchecked because each connection adds a new entry.
>>
>> * When the process exits abnormally, tls_connection:terminate/1 is never
>> called because the trap_exit process flag is not set and so similarly
>> the table (and everything else cleaned in terminate/1, for that matter)
>> is not cleaned up. This doesn't affect "normal" termination caused by
>> the connection closing because terminate/1 is called explicitly from
>> handle_sync_event/4, rather that relying on gen_fsm's automatic calling.
>>
>> Fixes for both are here:
>>
>> git fetch git://github.com/bernardd/otp ssl_cert_cache_fix
>> https://github.com/bernardd/otp/compare/erlang:master...ssl_cert_cache_fix
>>
>>
>> Credit goes to my colleague Nick Marino for doing the initial legwork to
>> track this down.
>>
>> Cheers,
>>
>> Bernard
>>
>> ________________________________
>>
>> This e-mail and any attachments are confidential. If it is not
>> intended for you, please notify the sender, and please erase and
>> ignore the contents.
>> _______________________________________________
>> erlang-patches mailing list
>> erlang-patches
>> http://erlang.org/mailman/listinfo/erlang-patches
> --
> /Henrik Nord Erlang/OTP
>
>
> ________________________________
>
> This e-mail and any attachments are confidential. If it is not intended for you, please notify the sender, and please erase and ignore the contents.

--
/Henrik Nord Erlang/OTP

[unknown]

Hi!

Thank you very much for your effort, however I have already included the
reaming part of your patch in 17.0, writing a test case for it myself.
It happened to become prioritized and we had not heard anything from you
in a long time. I apologizes for the information not reaching the github
pull-request immediately, I thought it had (a miss in internal routines,
we will make sure it does not happen again). If you had based your
branch on master you would have seen it included, and I have thanked you
in the release note for your participation.

Regards Ingela Erlang/OTP team - Ericsson AB

On 03/31/2014 01:58 PM, Henrik Nord wrote:
> Thanks!
>
> As we are already passed the codestop for 17.0 we will put this in
the backlog for 17.1.
>
> Thank you for your contribution!
>
> On 2014-03-31 13:38, Bernie Duggan wrote:
>> Hi Henrik,
>> I've trimmed the patch down to just the one remaining problem and
(after much boggling over the SSL unit test system) added a unit test
that checks the fix. I've rebased it to 'maint' and created a pull
request. You can see the change at:
>>
>>
https://github.com/bernardd/otp/compare/erlang:maint...ssl_cert_cache_fix
>>
https://github.com/bernardd/otp/compare/erlang:maint...ssl_cert_cache_fix.patch
>>
>> Cheers,
>>
>> Bernard
>> ________________________________________
>> From: Henrik Nord [henrik]
>> Sent: Thursday, 6 February 2014 3:27 AM
>> To: Bernie Duggan; erlang patches
>> Subject: Re: [erlang-patches] Fix SSL ETS table element leak and
error cleanup
>>
>> Hi!
>>
>> Patch solves two problems, one (trap exit) is already solved in R16B03.
>>
>> The other part of the patch we do want, but it should be ammended nice
>> with a test case.
>>
>>
>>
>> On 2013-12-10 05:44, Bernard Duggan wrote:
>>> Hi list,
>>> The SSL library maintains an internal table of CA certificates
>>> (ssl_otp_cacertificate_db). This is supposed to be cleaned up when the
>>> last connection using a certificate closes, however there's two
problems
>>> in R16B02 (and in the current master branch on github):
>>>
>>> * When CA certificates are provided as binary blobs, rather than by
>>> filename (ie, #ssl_options.cacerts is set, but #ssl_options.cacertfile
>>> is not) the cleanup never occurs due to an incorrect pattern match in
>>> tls_connection:handle_trusted_certs_db/1. This causes the table to grow
>>> unchecked because each connection adds a new entry.
>>>
>>> * When the process exits abnormally, tls_connection:terminate/1 is
never
>>> called because the trap_exit process flag is not set and so similarly
>>> the table (and everything else cleaned in terminate/1, for that matter)
>>> is not cleaned up. This doesn't affect "normal" termination caused by
>>> the connection closing because terminate/1 is called explicitly from
>>> handle_sync_event/4, rather that relying on gen_fsm's automatic
calling.
>>>
>>> Fixes for both are here:
>>>
>>> git fetch git://github.com/bernardd/otp ssl_cert_cache_fix
>>>
https://github.com/bernardd/otp/compare/erlang:master...ssl_cert_cache_fix
>>>
>>>
>>> Credit goes to my colleague Nick Marino for doing the initial
legwork to
>>> track this down.
>>>
>>> Cheers,
>>>
>>> Bernard
>>>
>>> ________________________________
>>>
>>> This e-mail and any attachments are confidential. If it is not
>>> intended for you, please notify the sender, and please erase and
>>> ignore the contents.
>>> _______________________________________________
>>> erlang-patches mailing list
>>> erlang-patches
>>> http://erlang.org/mailman/listinfo/erlang-patches
>> --
>> /Henrik Nord Erlang/OTP
>>
>>
>> ________________________________
>>
>> This e-mail and any attachments are confidential. If it is not
intended for you, please notify the sender, and please erase and ignore
the contents.
>