[PATCH] Fix for database locking crashes (was Re: bogofilter/bogoutil concurrency crash?)

[David Relson]

At 10:35 PM 4/6/03, Jim Correia wrote:

>On Monday, April 7, 2003, at 12:16  AM, David Relson wrote:
>
>>Just post the patch to the mailing list.  I'll get it and can test it.
>
>Ok. Here it is. As you'll see since we were peeking at disposed memory 
>this can actually fail on any system. (Maybe the malloc allocator on Mac 
>OS X makes it more likely by scribbling on the freed block right away.)
>
>I'm not completely familiar with the code, but it looks like the right 
>fix. It passes all 'make check' tests now, and also fixes the crash 
>previously described.
>
>(I don't usually use the CLI to generate diffs/patches - I hope this comes 
>out at the other end in tact.)

Jim,

You did a fine job generating the patch.  It applied properly on my machine 
and had no bad effects on the regression test, a.k.a. "make 
check".  However, the changes are not optimal.

The sequence "db_close(handle,??); handle = NULL; goto open_err" is 
inefficient.  open_err: calls dbh_free(handle) and returns NULL.  As you 
point out, the call to dbh_free() is not needed because db_close() has 
already called dbh_free().  The code after the call to db_close() should be 
"return NULL" (which is simpler).

Also, you added a "goto open_error".  That exits the "waiting for the lock" 
loop, which is _not_ what we want.

The attached patch has the critical "handle = NULL" and added test for 
handle != NULL before using it.  Give it a try and let me know how well it 
works (compared to your patch).

David

-------------- next part --------------
Index: datastore_db.c
===================================================================
RCS file: /cvsroot/bogofilter/bogofilter/src/datastore_db.c,v
retrieving revision 1.17
diff -u -r1.17 datastore_db.c
--- datastore_db.c	28 Mar 2003 15:15:58 -0000	1.17
+++ datastore_db.c	7 Apr 2003 04:09:28 -0000
@@ -183,7 +183,7 @@
 	    handle->dbp->err (handle->dbp, ret, "%s (db) get_byteswapped: %s",
 		    progname, db_file);
 	    db_close(handle, false);
-	    goto open_err;
+	    return NULL;		/* handle already freed, ok to return */
 	}
 
 	ret = handle->dbp->fd(handle->dbp, &handle->fd);
@@ -191,7 +191,7 @@
 	    handle->dbp->err (handle->dbp, ret, "%s (db) fd: %s",
 		    progname, db_file);
 	    db_close(handle, false);
-	    goto open_err;
+	    return NULL;		/* handle already freed, ok to return */
 	}
 
 	if (db_lock(handle->fd, F_SETLK,
@@ -200,6 +200,7 @@
 	    int e = errno;
 	    handle->fd = -1;
 	    db_close(handle, true);
+	    handle = NULL;	/* db_close freed it, we don't want to use it anymore */
 	    errno = e;
 	    /* do not bother to retry if the problem wasn't EAGAIN */
 	    if (e != EAGAIN && e != EACCES) return NULL;
@@ -209,7 +210,7 @@
 	}
     }
 
-    if (handle -> fd >= 0) {
+    if (handle && (handle -> fd >= 0)) {
 	handle->locked = true;
 	return (void *)handle;
     }