You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2008/02/25 21:49:55 UTC
Re: svn commit: r630974 - in /httpd/httpd/trunk/modules/ssl: ssl_engine_config.c
ssl_private.h ssl_scache.c ssl_scache_dbm.c ssl_scache_dc.c ssl_scache_memcache.c
ssl_scache_shmcb.c
On 02/25/2008 09:09 PM, jorton@apache.org wrote:
> Author: jorton
> Date: Mon Feb 25 12:09:38 2008
> New Revision: 630974
>
> URL: http://svn.apache.org/viewvc?rev=630974&view=rev
> Log:
> Session cache interface redesign, Part 4:
>
> Move provider-specific configuration handling down into the provider
> code. Eliminate all use of SSLModConfigRec within provider code.
>
> * modules/ssl/ssl_private.h (modssl_sesscache_provider): Add 'create'
> function which creates and configures the cache provider, before
> initialisation. Change 'init' function to take the context pointer
> as an input parameter, and reorder to be first.
>
> * modules/ssl/ssl_scache.c (ssl_scache_init): Adjust accordingly.
>
> * modules/ssl/ssl_scache_memcache.c (struct context): Add servers
> field.
> (ssl_scache_mc_create): New function.
> (ssl_scache_mc_init): Use servers from context not SSLModConfigRec.
>
> * modules/ssl/ssl_scache_dbm.c (struct context): Define.
> (ssl_scache_dbm_create): New function.
> (ssl_scache_dbm_init, ssl_scache_dbm_kill): Adjust to use filename
> and pool from context.
> (ssl_scache_dbm_store, ssl_scache_dbm_retrieve,
> ssl_scache_dbm_status): Use filename from context. Use context pool
> for temp storage of the DBM object, and clear before use.
> (ssl_scache_dbm_expire): Remove static tLast; use last_expiry from
> context. Use context pool for temp storage and clear before use.
>
> * modules/ssl/ssl_scache_dc.c (struct context): Add target field.
> (ssl_scache_dc_init, ssl_scache_dc_status): Use target from context.
>
> * modules/ssl/ssl_scache_shmcb.c (struct context): Add data_file,
> shm_size fields.
> (ssl_scache_shmcb_create): New function; moved argument parsing
> logic from ssl_cmd_SSLSessionCache
> (ssl_scache_shmcb_init, ssl_scache_shmcb_status): Use config from
> context.
>
> * modules/ssl/ssl_engine_config.c (ssl_config_global_create): Remove
> handling of old provider-specific fields.
> (ssl_cmd_SSLSessionCache): Call provider ->create function to parse
> the argument and create provider-specific context structure.
>
> Modified:
> httpd/httpd/trunk/modules/ssl/ssl_engine_config.c
> httpd/httpd/trunk/modules/ssl/ssl_private.h
> httpd/httpd/trunk/modules/ssl/ssl_scache.c
> httpd/httpd/trunk/modules/ssl/ssl_scache_dbm.c
> httpd/httpd/trunk/modules/ssl/ssl_scache_dc.c
> httpd/httpd/trunk/modules/ssl/ssl_scache_memcache.c
> httpd/httpd/trunk/modules/ssl/ssl_scache_shmcb.c
>
>
> Modified: httpd/httpd/trunk/modules/ssl/ssl_scache_dbm.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_scache_dbm.c?rev=630974&r1=630973&r2=630974&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/ssl/ssl_scache_dbm.c (original)
> +++ httpd/httpd/trunk/modules/ssl/ssl_scache_dbm.c Mon Feb 25 12:09:38 2008
> @@ -26,19 +26,43 @@
>
> #include "ssl_private.h"
>
> -static void ssl_scache_dbm_expire(void *context, server_rec *s);
> +/* Use of the context structure must be thread-safe after the initial
> + * create/init; callers must hold the mutex. */
> +struct context {
> + const char *data_file;
> + /* Pool must only be used with the mutex held. */
> + apr_pool_t *pool;
> + time_t last_expiry;
> +};
> +
> +static void ssl_scache_dbm_expire(struct context *ctx, server_rec *s);
>
> static void ssl_scache_dbm_remove(void *context, server_rec *s, UCHAR *id, int idlen,
> apr_pool_t *p);
>
> -static apr_status_t ssl_scache_dbm_init(server_rec *s, void **context, apr_pool_t *p)
> +static const char *ssl_scache_dbm_create(void **context, const char *arg,
> + apr_pool_t *tmp, apr_pool_t *p)
> +{
> + struct context *ctx;
> +
> + *context = ctx = apr_pcalloc(p, sizeof *ctx);
Hm. Don't we need to set ctx->pool to p or create a subpool of p and
assign it to ctx->pool?
> +
> + ctx->data_file = ap_server_root_relative(p, arg);
> + if (!ctx->data_file) {
> + return apr_psprintf(tmp, "Invalid cache file path %s", arg);
> + }
> +
> + return NULL;
> +}
> +
> +static apr_status_t ssl_scache_dbm_init(void *context, server_rec *s, apr_pool_t *p)
> {
> - SSLModConfigRec *mc = myModConfig(s);
> + struct context *ctx = context;
> apr_dbm_t *dbm;
> apr_status_t rv;
>
> /* for the DBM we need the data file */
> - if (mc->szSessionCacheDataFile == NULL) {
> + if (ctx->data_file == NULL) {
> ap_log_error(APLOG_MARK, APLOG_ERR, 0, s,
> "SSLSessionCache required");
> return APR_EINVAL;
> @@ -274,12 +291,12 @@
>
> /* and delete it from the DBM file */
> ssl_mutex_on(s);
Any particular reason why we do not clear ctx->pool here?
> - if ((rv = apr_dbm_open(&dbm, mc->szSessionCacheDataFile,
> + if ((rv = apr_dbm_open(&dbm, ctx->data_file,
> APR_DBM_RWCREATE, SSL_DBM_FILE_MODE, p)) != APR_SUCCESS) {
> ap_log_error(APLOG_MARK, APLOG_ERR, rv, s,
> "Cannot open SSLSessionCache DBM file `%s' for writing "
> "(delete)",
> - mc->szSessionCacheDataFile);
> + ctx->data_file);
> ssl_mutex_off(s);
> return;
> }
> Modified: httpd/httpd/trunk/modules/ssl/ssl_scache_dc.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_scache_dc.c?rev=630974&r1=630973&r2=630974&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/ssl/ssl_scache_dc.c (original)
> +++ httpd/httpd/trunk/modules/ssl/ssl_scache_dc.c Mon Feb 25 12:09:38 2008
> @@ -49,22 +49,28 @@
> */
>
> struct context {
> + /* Configured target server: */
> + const char *target;
> + /* distcache client context: */
> DC_CTX *dc;
> };
>
> -static apr_status_t ssl_scache_dc_init(server_rec *s, void **context, apr_pool_t *p)
> +static const char *ssl_scache_dc_create(void **context, const char *arg,
> + apr_pool_t *tmp, apr_pool_t *p)
> {
> - DC_CTX *dc;
> - SSLModConfigRec *mc = myModConfig(s);
> struct context *ctx;
>
> - /*
> - * Create a session context
> - */
> - if (mc->szSessionCacheDataFile == NULL) {
> - ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, "SSLSessionCache required");
> - return APR_EINVAL;
> - }
Why don't we check any if arg == NULL as a replacement for the if statement above?
> + ctx = *context = apr_palloc(p, sizeof *ctx);
> +
> + ctx->target = apr_pstrdup(p, arg);
> +
> + return NULL;
> +}
> +
> +static apr_status_t ssl_scache_dc_init(void *context, server_rec *s, apr_pool_t *p)
> +{
> + struct context *ctx = ctx;
> +
> #if 0
> /* If a "persistent connection" mode of operation is preferred, you *must*
> * also use the PIDCHECK flag to ensure fork()'d processes don't interlace
> Modified: httpd/httpd/trunk/modules/ssl/ssl_scache_memcache.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_scache_memcache.c?rev=630974&r1=630973&r2=630974&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/ssl/ssl_scache_memcache.c (original)
> +++ httpd/httpd/trunk/modules/ssl/ssl_scache_memcache.c Mon Feb 25 12:09:38 2008
> @@ -68,10 +68,23 @@
> #endif
>
> struct context {
> + const char *servers;
> apr_memcache_t *mc;
> };
>
> -static apr_status_t ssl_scache_mc_init(server_rec *s, void **context, apr_pool_t *p)
> +static const char *ssl_scache_mc_create(void **context, const char *arg,
> + apr_pool_t *tmp, apr_pool_t *p)
> +{
> + struct context *ctx;
> +
> + *context = ctx = apr_palloc(p, sizeof *ctx);
> +
> + ctx->servers = apr_pstrdup(p, arg);
Why don't we check any if arg == NULL as a replacement for the if statement below
checking mc->szSessionCacheDataFile == NULL
> +
> + return NULL;
> +}
> +
> +static apr_status_t ssl_scache_mc_init(void *context, server_rec *s, apr_pool_t *p)
> {
> apr_status_t rv;
> int thread_limit = 0;
> @@ -79,18 +92,12 @@
> char *cache_config;
> char *split;
> char *tok;
> - SSLModConfigRec *mc = myModConfig(s);
> - struct context *ctx = apr_palloc(p, sizeof *ctx);
> + struct context *ctx = context;
>
> ap_mpm_query(AP_MPMQ_HARD_LIMIT_THREADS, &thread_limit);
>
> - if (mc->szSessionCacheDataFile == NULL) {
> - ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, "SSLSessionCache required");
> - return APR_EINVAL;
> - }
> -
> /* Find all the servers in the first run to get a total count */
> - cache_config = apr_pstrdup(p, mc->szSessionCacheDataFile);
> + cache_config = apr_pstrdup(p, ctx->servers);
> split = apr_strtok(cache_config, ",", &tok);
> while (split) {
> nservers++;
Regards
Rüdiger
Re: svn commit: r630974 - in /httpd/httpd/trunk/modules/ssl: ssl_engine_config.c
ssl_private.h ssl_scache.c ssl_scache_dbm.c ssl_scache_dc.c ssl_scache_memcache.c
ssl_scache_shmcb.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 02/25/2008 11:40 PM, Ruediger Pluem wrote:
>
>
> On 02/25/2008 11:22 PM, Ruediger Pluem wrote:
>>
>>
>> On 02/25/2008 10:45 PM, Joe Orton wrote:
>>> On Mon, Feb 25, 2008 at 09:49:55PM +0100, Ruediger Pluem wrote:
>>>> On 02/25/2008 09:09 PM, jorton@apache.org wrote:
>>>>> Author: jorton
>>>>> Date: Mon Feb 25 12:09:38 2008
>>>>> New Revision: 630974
>>
>>>>> - if (mc->szSessionCacheDataFile == NULL) {
>>>>> - ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, "SSLSessionCache
>>>>> required");
>>>>> - return APR_EINVAL;
>>>>> - }
>>>> Why don't we check any if arg == NULL as a replacement for the if
>>>> statement above?
>>>
>>> Sorry, I should have mentioned this in the changelog message. In
>>> both cases of this, the code will always be passed a non-NULL arg
>>> parameter
>>
>> Hm, but you are supplying sep + 1 which is not NULL but can point to
>> '\0'.
>> Shouldn't we check for this?
>>
>>> unless a pstrdup fails (which by policy will be ignored anyway). I
>>> think these checks may have just been a copy'n'paste legacy from the
>>> shmcb code, which has genuine config argument parsing failure cases.
>>>
>>> Again, thanks for the detailed review!
>
> Running the perl test suit I saw some regressions with core dumps:
>
> t/modules/status.t 1 1 100.00% 1
> t/security/CVE-2006-5752.t 2 2 100.00% 1-2
> t/security/CVE-2007-6388.t 2 2 100.00% 1-2
>
> There might be something rotten in the mod_ssl status hook
> now.
Ah. The following patch fixes this:
Index: modules/ssl/ssl_scache.c
===================================================================
--- modules/ssl/ssl_scache.c (Revision 631020)
+++ modules/ssl/ssl_scache.c (Arbeitskopie)
@@ -143,7 +143,7 @@
{
SSLModConfigRec *mc = myModConfig(r->server);
- if (mc == NULL || flags & AP_STATUS_SHORT)
+ if (mc == NULL || flags & AP_STATUS_SHORT || mc->sesscache == NULL)
return OK;
ap_rputs("<hr>\n", r);
Regards
Rüdiger
Re: svn commit: r630974 - in /httpd/httpd/trunk/modules/ssl: ssl_engine_config.c
ssl_private.h ssl_scache.c ssl_scache_dbm.c ssl_scache_dc.c ssl_scache_memcache.c
ssl_scache_shmcb.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 02/25/2008 11:22 PM, Ruediger Pluem wrote:
>
>
> On 02/25/2008 10:45 PM, Joe Orton wrote:
>> On Mon, Feb 25, 2008 at 09:49:55PM +0100, Ruediger Pluem wrote:
>>> On 02/25/2008 09:09 PM, jorton@apache.org wrote:
>>>> Author: jorton
>>>> Date: Mon Feb 25 12:09:38 2008
>>>> New Revision: 630974
>
>>>> - if (mc->szSessionCacheDataFile == NULL) {
>>>> - ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, "SSLSessionCache
>>>> required");
>>>> - return APR_EINVAL;
>>>> - }
>>> Why don't we check any if arg == NULL as a replacement for the if
>>> statement above?
>>
>> Sorry, I should have mentioned this in the changelog message. In both
>> cases of this, the code will always be passed a non-NULL arg parameter
>
> Hm, but you are supplying sep + 1 which is not NULL but can point to '\0'.
> Shouldn't we check for this?
>
>> unless a pstrdup fails (which by policy will be ignored anyway). I
>> think these checks may have just been a copy'n'paste legacy from the
>> shmcb code, which has genuine config argument parsing failure cases.
>>
>> Again, thanks for the detailed review!
Running the perl test suit I saw some regressions with core dumps:
t/modules/status.t 1 1 100.00% 1
t/security/CVE-2006-5752.t 2 2 100.00% 1-2
t/security/CVE-2007-6388.t 2 2 100.00% 1-2
There might be something rotten in the mod_ssl status hook
now.
Regards
Rüdiger
Re: svn commit: r630974 - in /httpd/httpd/trunk/modules/ssl: ssl_engine_config.c
ssl_private.h ssl_scache.c ssl_scache_dbm.c ssl_scache_dc.c ssl_scache_memcache.c
ssl_scache_shmcb.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 02/25/2008 10:45 PM, Joe Orton wrote:
> On Mon, Feb 25, 2008 at 09:49:55PM +0100, Ruediger Pluem wrote:
>> On 02/25/2008 09:09 PM, jorton@apache.org wrote:
>>> Author: jorton
>>> Date: Mon Feb 25 12:09:38 2008
>>> New Revision: 630974
>>> - if (mc->szSessionCacheDataFile == NULL) {
>>> - ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, "SSLSessionCache required");
>>> - return APR_EINVAL;
>>> - }
>> Why don't we check any if arg == NULL as a replacement for the if statement above?
>
> Sorry, I should have mentioned this in the changelog message. In both
> cases of this, the code will always be passed a non-NULL arg parameter
Hm, but you are supplying sep + 1 which is not NULL but can point to '\0'.
Shouldn't we check for this?
> unless a pstrdup fails (which by policy will be ignored anyway). I
> think these checks may have just been a copy'n'paste legacy from the
> shmcb code, which has genuine config argument parsing failure cases.
>
> Again, thanks for the detailed review!
You are welcome.
Regards
Rüdiger
Re: svn commit: r630974 - in /httpd/httpd/trunk/modules/ssl:
ssl_engine_config.c ssl_private.h ssl_scache.c ssl_scache_dbm.c
ssl_scache_dc.c ssl_scache_memcache.c ssl_scache_shmcb.c
Posted by Joe Orton <jo...@redhat.com>.
On Mon, Feb 25, 2008 at 09:49:55PM +0100, Ruediger Pluem wrote:
> On 02/25/2008 09:09 PM, jorton@apache.org wrote:
>> Author: jorton
>> Date: Mon Feb 25 12:09:38 2008
>> New Revision: 630974
>>
>> URL: http://svn.apache.org/viewvc?rev=630974&view=rev
>> Log:
>> Session cache interface redesign, Part 4:
...
>> --- httpd/httpd/trunk/modules/ssl/ssl_scache_dbm.c (original)
>> +++ httpd/httpd/trunk/modules/ssl/ssl_scache_dbm.c Mon Feb 25 12:09:38 2008
...
>> +static const char *ssl_scache_dbm_create(void **context, const char *arg,
>> + apr_pool_t *tmp, apr_pool_t *p)
>> +{
>> + struct context *ctx;
>> +
>> + *context = ctx = apr_pcalloc(p, sizeof *ctx);
>
> Hm. Don't we need to set ctx->pool to p or create a subpool of p and
> assign it to ctx->pool?
Yes indeed, I already found this out with a little trip through gdb.
Fixed in r630990.
>> @@ -274,12 +291,12 @@
>> /* and delete it from the DBM file */
>> ssl_mutex_on(s);
>
> Any particular reason why we do not clear ctx->pool here?
I missed that, and it wasn't even using ctx->pool later; thanks!
...
>> --- httpd/httpd/trunk/modules/ssl/ssl_scache_dc.c (original)
>> +++ httpd/httpd/trunk/modules/ssl/ssl_scache_dc.c Mon Feb 25 12:09:38 2008
>> @@ -49,22 +49,28 @@
>> */
>> struct context {
>> + /* Configured target server: */
>> + const char *target;
>> + /* distcache client context: */
>> DC_CTX *dc;
>> };
>> -static apr_status_t ssl_scache_dc_init(server_rec *s, void **context, apr_pool_t *p)
>> +static const char *ssl_scache_dc_create(void **context, const char *arg,
>> + apr_pool_t *tmp, apr_pool_t *p)
>> {
>> - DC_CTX *dc;
>> - SSLModConfigRec *mc = myModConfig(s);
>> struct context *ctx;
>> - /*
>> - * Create a session context
>> - */
>> - if (mc->szSessionCacheDataFile == NULL) {
>> - ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, "SSLSessionCache required");
>> - return APR_EINVAL;
>> - }
>
> Why don't we check any if arg == NULL as a replacement for the if statement above?
Sorry, I should have mentioned this in the changelog message. In both
cases of this, the code will always be passed a non-NULL arg parameter
unless a pstrdup fails (which by policy will be ignored anyway). I
think these checks may have just been a copy'n'paste legacy from the
shmcb code, which has genuine config argument parsing failure cases.
Again, thanks for the detailed review!
Regards,
joe