So, here's the patch that should fix the BDB-related
FMR-during-apr_terminate bug on trunk. I followed Philip's suggestion to
use pool cleanups in the cache pool to implement Garrett's and Justin's
suggestion to invalidate BDB environment batons during apr_terminate
(thanks, guys!), and I think this worked out pretty well -- except that
anyone trying to follow the tangled maze of pool cleanups is likely to
get a headache pretty quickly.
Anyway, this passes all tests (local and svn://) on my box, and I'll run
another set of stress tests before committing this.
Unfortunately, I can't easily reproduce Philip's test case on Windows
(or if I can, the FMR doesn't show up in Purify for some reason).
Philip, could you possibly try your last test (I think it was checkout
via 'svnserve -X' under valgrind) with this patch?
[[[
Prevent accessing freed memory through BDB environment batons by
invalidating the baton during pool cleanup of the environment
descriptor cache.
Suggested by: rooneg
jerenkrantz
philip
* subversion/libsvn_fs_base/bdb/env.h (svn_fs_bdb_t): Add a 'valid' flag
to this baton.
* subversion/libsvn_fs_base/bdb/env.c (invalidate_env_baton): New.
(svn_fs_bdb__open): Register it in the environment descriptor's pool.
Set the baton's 'valid' flag, and create the baton while the
cache mutex is locked.
(svn_fs_bdb__close): Run the invalidate_env_baton cleanup when the
cache mutex is locked.
(cleanup_env_baton, svn_fs_bdb__get_panic, svn_fs_bdb__set_panic):
Don't touch the environment descriptor if the baton is invalid.
]]]
Index: subversion/libsvn_fs_base/bdb/env.c
===================================================================
--- subversion/libsvn_fs_base/bdb/env.c (revision 18846)
+++ subversion/libsvn_fs_base/bdb/env.c (working copy)
@@ -214,6 +214,7 @@
bdb_baton.env = bdb->env;
bdb_baton.bdb = bdb;
bdb_baton.error_info = get_error_info(bdb);
+ bdb_baton.valid = TRUE;
SVN_BDB_ERR(&bdb_baton, db_err);
}
return SVN_NO_ERROR;
@@ -509,6 +510,7 @@
return err;
}
+static apr_status_t invalidate_env_baton(void *data);
svn_error_t *
svn_fs_bdb__close(bdb_env_baton_t *bdb_baton)
@@ -516,6 +518,11 @@
svn_error_t *err = SVN_NO_ERROR;
bdb_env_t *bdb = bdb_baton->bdb;
+ /* Don't do anything if the BDB baton has been invalidated; if it
+ has been, then the environment is already closed. */
+ if (!bdb_baton->valid)
+ return SVN_NO_ERROR;
+
assert(bdb_baton->env == bdb_baton->bdb->env);
/* Neutralize bdb_baton's pool cleanup to prevent double-close. See
@@ -532,6 +539,7 @@
}
acquire_cache_mutex();
+ apr_pool_cleanup_run(bdb->pool, bdb_baton, invalidate_env_baton);
if (--bdb->refcount != 0)
{
@@ -586,13 +594,32 @@
{
bdb_env_baton_t *bdb_baton = data;
- if (bdb_baton->bdb)
+ if (bdb_baton->valid && bdb_baton->bdb)
svn_error_clear(svn_fs_bdb__close(bdb_baton));
return APR_SUCCESS;
}
+/* Pool cleanup that invalidates the environment baton.
+ This pool cleanup is registered in the environment descriptor's pool.
+ The cleanup can be run in one of two cases:
+ - explicitly during svn_fs_bdb__close; this will happen in the context
+ of the baton owner's thread, and with the global cach mutex locked.
+ This will happen either after an explicit call to svn_fs_bdb__close,
+ or during cleanup of the baton's pool. In both cases, the baton
+ is considered dead afterwards.
+ - implicitly during apr_terminate, when the cache is destroyed; this
+ is guaranteed to happen in a single-threaded context.
+*/
+static apr_status_t
+invalidate_env_baton(void *data)
+{
+ bdb_env_baton_t *bdb_baton = data;
+ bdb_baton->valid = FALSE;
+ return APR_SUCCESS;
+}
+
svn_error_t *
svn_fs_bdb__open(bdb_env_baton_t **bdb_batonp, const char *path,
u_int32_t flags, int mode,
@@ -674,18 +701,21 @@
++bdb->refcount;
}
- release_cache_mutex();
-
if (!err)
{
*bdb_batonp = apr_palloc(pool, sizeof **bdb_batonp);
(*bdb_batonp)->env = bdb->env;
(*bdb_batonp)->bdb = bdb;
(*bdb_batonp)->error_info = get_error_info(bdb);
+ (*bdb_batonp)->valid = TRUE;
++(*bdb_batonp)->error_info->refcount;
apr_pool_cleanup_register(pool, *bdb_batonp, cleanup_env_baton,
apr_pool_cleanup_null);
+ apr_pool_cleanup_register(bdb->pool, *bdb_batonp, invalidate_env_baton,
+ apr_pool_cleanup_null);
}
+
+ release_cache_mutex();
return err;
}
@@ -693,6 +723,11 @@
svn_boolean_t
svn_fs_bdb__get_panic(bdb_env_baton_t *bdb_baton)
{
+ /* An invalid baton is equivalent to a panicked environment; in both
+ cases, database cleanups should be skipped. */
+ if (!bdb_baton->valid)
+ return TRUE;
+
assert(bdb_baton->env == bdb_baton->bdb->env);
return !!svn__atomic_read(&bdb_baton->bdb->panic);
}
@@ -700,6 +735,9 @@
void
svn_fs_bdb__set_panic(bdb_env_baton_t *bdb_baton)
{
+ if (!bdb_baton->valid)
+ return;
+
assert(bdb_baton->env == bdb_baton->bdb->env);
svn__atomic_set(&bdb_baton->bdb->panic, TRUE);
}
Index: subversion/libsvn_fs_base/bdb/env.h
===================================================================
--- subversion/libsvn_fs_base/bdb/env.h (revision 18846)
+++ subversion/libsvn_fs_base/bdb/env.h (working copy)
@@ -73,7 +73,7 @@
typedef struct
{
/* The Berkeley DB environment. This pointer must be identical to
- the one in bdb_env_t. */
+ the one in the bdb_env_t. */
DB_ENV *env;
/* The (opaque) cached environemnt descriptor. */
@@ -82,6 +82,19 @@
/* The error info related to this baton. */
bdb_error_info_t *error_info;
+ /* This flag determines if the environment baton points to a valid
+ environment descriptor. It is used for inetrnal bookkeeping and
+ pool lifetime disambiguation, and need not be checked by the user
+ of the baton.
+
+ This flag will become FALSE when the baton is closed (either because
+ of an explicit call to svn_fs_bdb__close, or during pool cleanup);
+ in that case the baton can not be used again.
+
+ The flag may also becone FALSE during global pool cleanup in
+ apr_terminate, if the baton's pool happens to survive longer than
+ the environment descriptor cache. */
+ svn_boolean_t valid;
} bdb_env_baton_t;
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Mar 12 04:42:57 2006