[PATCH] ssl_scache_dbm

[Bill Stoddard <bi...@wstoddard.com>]

This patch corrects a problem where we are using apr_dbm calls incorrectly.  WIBNIF we
passed in request_rec rather than the server_rec on:

ssl_scache_dbm_retrieve(server_rec *s, UCHAR *id, int idlen)

This would allow us to close the dbm with a request pool cleanup which would remove some
cruftyness from this patch. Also looking for comments on the (in)appropriate use of
ssl_mutex calls to serialize access to the dbm.

Index: ssl_scache_dbm.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/ssl/ssl_scache_dbm.c,v
retrieving revision 1.9
diff -u -r1.9 ssl_scache_dbm.c
--- ssl_scache_dbm.c 2001/11/07 01:43:20 1.9
+++ ssl_scache_dbm.c 2001/11/07 02:43:43
@@ -219,7 +219,16 @@
     dbmkey.dsize = idlen;

     /* and fetch it from the DBM file */
+    /* XXX: Do we -really- need the ssl_mutex calls here? The apr_dbm calls
+     * should provide appropriate serialization...
+     */
     ssl_mutex_on(s);
+
+    /* XXX: Should we open the dbm against r->pool so the cleanup will
+     * have the right lifetime? Because we don't associate the dbm
+     * with a r->pool cleanup, we have to sprinkle apr_dbm_close()
+     * calls at every return point which just stinks.
+     */
     if (apr_dbm_open(&dbm, mc->szSessionCacheDataFile,
      APR_DBM_RWCREATE, SSL_DBM_FILE_MODE, mc->pPool) != APR_SUCCESS) {
         ssl_log(s, SSL_LOG_ERROR|SSL_ADD_ERRNO,
@@ -229,20 +238,23 @@
         return NULL;
     }
     rc = apr_dbm_fetch(dbm, dbmkey, &dbmval);
-    apr_dbm_close(dbm);
     ssl_mutex_off(s);
-
-    /* immediately return if not found */
-    if (rc != APR_SUCCESS)
+    if (rc != APR_SUCCESS) {
+        apr_dbm_close(dbm);
         return NULL;
-    if (dbmval.dptr == NULL || dbmval.dsize <= sizeof(time_t))
+    }
+    if (dbmval.dptr == NULL || dbmval.dsize <= sizeof(time_t)) {
+        apr_dbm_close(dbm);
         return NULL;
+    }

     /* parse resulting data */
     nData = dbmval.dsize-sizeof(time_t);
     ucpData = (UCHAR *)malloc(nData);
-    if (ucpData == NULL)
+    if (ucpData == NULL) {
+        apr_dbm_close(dbm);
         return NULL;
+    }
     memcpy(ucpData, (char *)dbmval.dptr+sizeof(time_t), nData);
     memcpy(&expiry, dbmval.dptr, sizeof(time_t));

@@ -250,12 +262,14 @@
     now = time(NULL);
     if (expiry <= now) {
         ssl_scache_dbm_remove(s, id, idlen);
+        apr_dbm_close(dbm);
         return NULL;
     }

     /* unstreamed SSL_SESSION */
     sess = d2i_SSL_SESSION(NULL, &ucpData, nData);

+    apr_dbm_close(dbm);
     return sess;
 }

Re: [PATCH] ssl_scache_dbm

["Roy T. Fielding" <fi...@ebuilt.com>]

On Tue, Nov 06, 2001 at 10:04:13PM -0500, Bill Stoddard wrote:
> This patch corrects a problem where we are using apr_dbm calls incorrectly.
> WIBNIF we
> passed in request_rec rather than the server_rec on:
> 
> ssl_scache_dbm_retrieve(server_rec *s, UCHAR *id, int idlen)
> 
> This would allow us to close the dbm with a request pool cleanup which would remove some
> cruftyness from this patch. Also looking for comments on the (in)appropriate use of
> ssl_mutex calls to serialize access to the dbm.

They are inappropriate.  In fact, the whole thing is inappropriate -- what
is the point in having a butt-slow cache?

+1 on the patch and removing the mutex, but I think a long-term solution
would be based on advisory locks around a shared-memory API (even if the
shared memory was simply backed by apr_dbm).

....Roy