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/23 11:40:26 UTC
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
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
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
Posted by Joe Orton <jo...@redhat.com>.
On Sat, Feb 23, 2008 at 11:40:26AM +0100, Ruediger Pluem wrote:
> 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
...
>> memcpy(&expiry, dbmval.dptr, sizeof(time_t));
>> + memcpy(dest, (char *)dbmval.dptr + sizeof(time_t), nData);
>
> Shouldn't we do
>
> *destlen = nData;
>
> here?
Fixed both of those cases in r630787.
>> ==============================================================================
>> --- 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
...
>> + /* 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?
And both of those cases in r630786.
Thanks a lot for the careful review!
joe