httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: svn commit: r630307 - in /httpd/httpd/trunk/modules/ssl: ssl_private.h ssl_scache.c ssl_scache_dbm.c ssl_scache_dc.c ssl_scache_memcache.c ssl_scache_shmcb.c
Date Sat, 23 Feb 2008 10:40:26 GMT


On 02/22/2008 08:58 PM, jorton@apache.org wrote:
> Author: jorton
> Date: Fri Feb 22 11:58:39 2008
> New Revision: 630307
> 
> URL: http://svn.apache.org/viewvc?rev=630307&view=rev
> Log:
> Move SSL session data deserialization up out of the session cache
> storage providers; includes a significant change to the shmcb storage
> structure:
> 
> * modules/ssl/ssl_private.h (modssl_sesscache_provider): Change
>   retrieve function to take dest/destlen output buffer, to take a
>   constant id paramater, and to return a BOOL.
> 
> * modules/ssl/ssl_scache.c (ssl_scache_retrieve): Update accordingly,
>   perform SSL deserialization here.
> 
> * modules/ssl/ssl_scache_dc.c (ssl_scache_dc_retrieve),
>   modules/ssl/ssl_scache_dbm.c (ssl_scache_dbm_retrieve),
>   modules/ssl/ssl_scache_memcache.c (ssl_scache_mc_retrieve):
>   Update accordingly.
> 
> * modules/ssl/ssl_scache_shmcb.c: Store the whole ID in the cache
>   before the data, so that each index can be compared against the
>   requested ID without deserializing the data.  This requires approx
>   20% extra storage per session in the common case, though should
>   reduce CPU overhead in some retrieval paths.
>   (SHMCBIndex): Replace s_id2 field with id_len.
>   (shmcb_cyclic_memcmp): New function.
>   (ssl_scache_shmcb_init): Change the heuristics to allow for increase
>   in per-session storage requirement.
>   (ssl_scache_shmcb_retrieve): Drop requirement on ID length.
>   (shmcb_subcache_store): Store the ID in the cyclic buffer.
>   (shmcb_subcache_retrieve, shmcb_subcache_remove): Compare against
>   the stored ID rather than deserializing the data.
>   (ssl_scache_shmcb_retrieve, ssl_scache_shmcb_store): Update
>   accordingly.
> 
> Modified:
>     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=630307&r1=630306&r2=630307&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/ssl/ssl_scache_dbm.c (original)
> +++ httpd/httpd/trunk/modules/ssl/ssl_scache_dbm.c Fri Feb 22 11:58:39 2008
> @@ -188,16 +188,15 @@
>      return TRUE;
>  }
>  
> -static SSL_SESSION *ssl_scache_dbm_retrieve(server_rec *s, UCHAR *id, int idlen,
> -                                            apr_pool_t *p)
> +static BOOL ssl_scache_dbm_retrieve(server_rec *s, const UCHAR *id, int idlen,
> +                                    unsigned char *dest, unsigned int *destlen,
> +                                    apr_pool_t *p)
>  {
>      SSLModConfigRec *mc = myModConfig(s);
>      apr_dbm_t *dbm;
>      apr_datum_t dbmkey;
>      apr_datum_t dbmval;
> -    SSL_SESSION *sess = NULL;
> -    MODSSL_D2I_SSL_SESSION_CONST unsigned char *ucpData;
> -    int nData;
> +    unsigned int nData;
>      time_t expiry;
>      time_t now;
>      apr_status_t rc;
> @@ -221,32 +220,30 @@
>                       "(fetch)",
>                       mc->szSessionCacheDataFile);
>          ssl_mutex_off(s);
> -        return NULL;
> +        return FALSE;
>      }
>      rc = apr_dbm_fetch(dbm, dbmkey, &dbmval);
>      if (rc != APR_SUCCESS) {
>          apr_dbm_close(dbm);
>          ssl_mutex_off(s);
> -        return NULL;
> +        return FALSE;
>      }
>      if (dbmval.dptr == NULL || dbmval.dsize <= sizeof(time_t)) {
>          apr_dbm_close(dbm);
>          ssl_mutex_off(s);
> -        return NULL;
> +        return FALSE;
>      }
>  
>      /* parse resulting data */
>      nData = dbmval.dsize-sizeof(time_t);
> -    ucpData = malloc(nData);
> -    if (ucpData == NULL) {
> +    if (nData > *destlen) {
>          apr_dbm_close(dbm);
>          ssl_mutex_off(s);
> -        return NULL;
> -    }
> -    /* Cast needed, ucpData may be const */
> -    memcpy((unsigned char *)ucpData,
> -           (char *)dbmval.dptr + sizeof(time_t), nData);
> +        return FALSE;
> +    }    
> +
>      memcpy(&expiry, dbmval.dptr, sizeof(time_t));
> +    memcpy(dest, (char *)dbmval.dptr + sizeof(time_t), nData);

Shouldn't we do

*destlen = nData;

here?


>  
>      apr_dbm_close(dbm);
>      ssl_mutex_off(s);
>
> 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=630307&r1=630306&r2=630307&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/ssl/ssl_scache_dc.c (original)
> +++ httpd/httpd/trunk/modules/ssl/ssl_scache_dc.c Fri Feb 22 11:58:39 2008
> @@ -116,32 +116,25 @@
>      return TRUE;
>  }
>  
> -static SSL_SESSION *ssl_scache_dc_retrieve(server_rec *s, UCHAR *id, int idlen, apr_pool_t
*p)
> +static BOOL ssl_scache_dc_retrieve(server_rec *s, const UCHAR *id, int idlen,
> +                                   unsigned char *dest, unsigned int *destlen,
> +                                   apr_pool_t *p)
>  {
> -    unsigned char der[SSL_SESSION_MAX_DER];
> -    unsigned int der_len;
> -    SSL_SESSION *pSession;
> -    MODSSL_D2I_SSL_SESSION_CONST unsigned char *pder = der;
> +    unsigned int data_len;
>      SSLModConfigRec *mc = myModConfig(s);
>      DC_CTX *ctx = mc->tSessionCacheDataTable;
>  
>      /* Retrieve any corresponding session from the distributed cache context */
> -    if (!DC_CTX_get_session(ctx, id, idlen, der, SSL_SESSION_MAX_DER,
> -                            &der_len)) {
> +    if (!DC_CTX_get_session(ctx, id, idlen, dest, *destlen, &data_len)) {
>          ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, "distributed scache 'get_session'
MISS");
> -        return NULL;
> +        return FALSE;
>      }
> -    if (der_len > SSL_SESSION_MAX_DER) {
> +    if (data_len > *destlen) {
>          ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, "distributed scache 'get_session'
OVERFLOW");
> -        return NULL;
> -    }
> -    pSession = d2i_SSL_SESSION(NULL, &pder, der_len);
> -    if (!pSession) {
> -        ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, "distributed scache 'get_session'
CORRUPT");
> -        return NULL;
> +        return FALSE;
>      }
>      ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, "distributed scache 'get_session' HIT");
> -    return pSession;

Shouldn't we do

*destlen = data_len;

here?


> +    return TRUE;
>  }
>  
>  static void ssl_scache_dc_remove(server_rec *s, UCHAR *id, int idlen, apr_pool_t *p)
> 

> Modified: httpd/httpd/trunk/modules/ssl/ssl_scache_shmcb.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_scache_shmcb.c?rev=630307&r1=630306&r2=630307&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/ssl/ssl_scache_shmcb.c (original)
> +++ httpd/httpd/trunk/modules/ssl/ssl_scache_shmcb.c Fri Feb 22 11:58:39 2008

> @@ -669,39 +722,31 @@
>      while (loop < subcache->idx_used) {
>          SHMCBIndex *idx = SHMCB_INDEX(subcache, pos);
>  
> -        /* Only consider 'idx' if;
> -         * (a) the s_id2 byte matches
> -         * (b) the "removed" flag isn't set.
> -         */
> -        if ((idx->s_id2 == id[1]) && !idx->removed) {
> -            SSL_SESSION *pSession;
> -            unsigned char *s_id;
> -            unsigned int s_idlen;
> -            unsigned char tempasn[SSL_SESSION_MAX_DER];
> -            MODSSL_D2I_SSL_SESSION_CONST unsigned char *ptr = tempasn;
> -
> +        /* Only consider 'idx' if the id matches, and the "removed"
> +         * flag isn't set; check the data length too to avoid a buffer
> +         * overflow in case of corruption, which should be impossible,
> +         * but it's cheap to be safe. */
> +        if (idx->id_len == idlen && (idx->data_used - idx->id_len)
< *destlen
> +            && shmcb_cyclic_memcmp(header->subcache_data_size,
> +                                   SHMCB_DATA(header, subcache),
> +                                   idx->data_pos, id, idx->id_len) == 0) {

Where do you check for the removed flag?

> @@ -730,38 +775,20 @@
>      pos = subcache->idx_pos;
>      while (!to_return && (loop < subcache->idx_used)) {
>          SHMCBIndex *idx = SHMCB_INDEX(subcache, pos);
> -        /* Only consider 'idx' if the s_id2 byte matches and it's not already
> -         * removed - easiest way to avoid costly ASN decodings. */
> -        if ((idx->s_id2 == id[1]) && !idx->removed) {
> -            SSL_SESSION *pSession;
> -            unsigned char *s_id;
> -            unsigned int s_idlen;
> -            unsigned char tempasn[SSL_SESSION_MAX_DER];
> -            MODSSL_D2I_SSL_SESSION_CONST unsigned char *ptr = tempasn;
>  
> +        /* Only consider 'idx' if the id matches, and the "removed"
> +         * flag isn't set. */
> +        if (idx->id_len == idlen
> +            && shmcb_cyclic_memcmp(header->subcache_data_size,
> +                                   SHMCB_DATA(header, subcache),
> +                                   idx->data_pos, id, idx->id_len) == 0) {

Where do you check for the removed flag?

Regards

RĂ¼diger


Mime
View raw message