You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Christophe Jaillet <ch...@wanadoo.fr> on 2018/06/30 12:08:16 UTC
Re: svn commit: r1682074 - in /httpd/httpd/branches/2.4.x: ./ STATUS
modules/ssl/ssl_engine_init.c
Le 27/05/2015 à 18:33, wrowe@apache.org a écrit :
> Author: wrowe
> Date: Wed May 27 16:33:10 2015
> New Revision: 1682074
>
> URL: http://svn.apache.org/r1682074
> Log:
> mod_ssl: fix small memory leak in ssl_init_server_certs when ECDH is used.
> SSL_CTX_set_tmp_ecdh increases reference count, so we have to call
> EC_KEY_free, otherwise eckey will not be freed.
>
> Backports: r1666363
> Author: jkaluza
> Reviewed by: rjung, ylavic, wrowe
>
>
> Modified:
> httpd/httpd/branches/2.4.x/ (props changed)
> httpd/httpd/branches/2.4.x/STATUS
> httpd/httpd/branches/2.4.x/modules/ssl/ssl_engine_init.c
>
> Modified: httpd/httpd/branches/2.4.x/STATUS
> URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/STATUS?rev=1682074&r1=1682073&r2=1682074&view=diff
> ==============================================================================
> --- httpd/httpd/branches/2.4.x/STATUS (original)
> +++ httpd/httpd/branches/2.4.x/STATUS Wed May 27 16:33:10 2015
> @@ -105,13 +105,6 @@ RELEASE SHOWSTOPPERS:
> PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
> [ start all new proposals below, under PATCHES PROPOSED. ]
>
> - *) mod_ssl: fix small memory leak in ssl_init_server_certs when ECDH is used.
> - SSL_CTX_set_tmp_ecdh increases reference count, so we have to call
> - EC_KEY_free, otherwise eckey will not be freed.
> - trunk patch: http://svn.apache.org/r1666363
> - 2.4.x patch: http://people.apache.org/~rjung/patches/httpd-2.4.x-free-eckey.patch
> - +1: rjung, ylavic, wrowe
> -
>
> PATCHES PROPOSED TO BACKPORT FROM TRUNK:
> [ New proposals should be added at the end of the list ]
>
> Modified: httpd/httpd/branches/2.4.x/modules/ssl/ssl_engine_init.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/ssl/ssl_engine_init.c?rev=1682074&r1=1682073&r2=1682074&view=diff
> ==============================================================================
> --- httpd/httpd/branches/2.4.x/modules/ssl/ssl_engine_init.c (original)
> +++ httpd/httpd/branches/2.4.x/modules/ssl/ssl_engine_init.c Wed May 27 16:33:10 2015
> @@ -960,7 +960,7 @@ static apr_status_t ssl_init_server_cert
> #ifdef HAVE_ECC
> EC_GROUP *ecparams;
> int nid;
> - EC_KEY *eckey;
> + EC_KEY *eckey = NULL;
> #endif
> #ifndef HAVE_SSL_CONF_CMD
> SSL *ssl;
> @@ -1133,6 +1133,7 @@ static apr_status_t ssl_init_server_cert
> EC_KEY_new_by_curve_name(NID_X9_62_prime256v1));
> #endif
> }
> + EC_KEY_free(eckey);
> #endif
>
> return APR_SUCCESS;
>
>
>
Hi,
This backport looks incomplete.
In the original patch (r1666363) we have:
@@ -1151,10 +1151,11 @@
#if defined(SSL_CTX_set_ecdh_auto)
SSL_CTX_set_ecdh_auto(mctx->ssl_ctx, 1);
#else
- SSL_CTX_set_tmp_ecdh(mctx->ssl_ctx,
-
EC_KEY_new_by_curve_name(NID_X9_62_prime256v1));
+ eckey = EC_KEY_new_by_curve_name(NID_X9_62_prime256v1);
+ SSL_CTX_set_tmp_ecdh(mctx->ssl_ctx, eckey);
which is not in the backported code. (neither in the .patch file, nor in
the backport itself)
Was it intentional or should the missing part be also backported?
My feeling is that it should be merged.
CJ
AW: svn commit: r1682074 - in /httpd/httpd/branches/2.4.x: ./
STATUS modules/ssl/ssl_engine_init.c
Posted by Plüm,
Rüdiger,
Vodafone Group <ru...@vodafone.com>.
> -----Ursprüngliche Nachricht-----
> Von: Christophe Jaillet <ch...@wanadoo.fr>
> Gesendet: Samstag, 30. Juni 2018 14:08
> An: dev@httpd.apache.org
> Betreff: Re: svn commit: r1682074 - in /httpd/httpd/branches/2.4.x: ./
> STATUS modules/ssl/ssl_engine_init.c
>
> Le 27/05/2015 à 18:33, wrowe@apache.org a écrit :
> > Author: wrowe
> > Date: Wed May 27 16:33:10 2015
> > New Revision: 1682074
> >
> > URL: http://svn.apache.org/r1682074
> > Log:
> > mod_ssl: fix small memory leak in ssl_init_server_certs when ECDH is
> used.
> > SSL_CTX_set_tmp_ecdh increases reference count, so we have to call
> > EC_KEY_free, otherwise eckey will not be freed.
> >
> > Backports: r1666363
> > Author: jkaluza
> > Reviewed by: rjung, ylavic, wrowe
> >
>
> Hi,
>
> This backport looks incomplete.
>
> In the original patch (r1666363) we have:
> @@ -1151,10 +1151,11 @@
> #if defined(SSL_CTX_set_ecdh_auto)
> SSL_CTX_set_ecdh_auto(mctx->ssl_ctx, 1);
> #else
> - SSL_CTX_set_tmp_ecdh(mctx->ssl_ctx,
> -
> EC_KEY_new_by_curve_name(NID_X9_62_prime256v1));
> + eckey = EC_KEY_new_by_curve_name(NID_X9_62_prime256v1);
> + SSL_CTX_set_tmp_ecdh(mctx->ssl_ctx, eckey);
> which is not in the backported code. (neither in the .patch file, nor in
> the backport itself)
>
> Was it intentional or should the missing part be also backported?
> My feeling is that it should be merged.
I agree that this part should be merged as well.
Regards
Rüdiger