[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

The (hopefully) final BDB-4.4 fix

From: Branko Čibej <brane_at_xbc.nu>
Date: 2006-03-12 04:42:38 CET

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

This is an archived mail posted to the Subversion Dev mailing list.