You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Joe Orton <jo...@redhat.com> on 2008/02/25 22:45:44 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 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
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