Index: subversion/libsvn_fs_base/bdb/env.c =================================================================== --- subversion/libsvn_fs_base/bdb/env.c (revision 24581) +++ subversion/libsvn_fs_base/bdb/env.c (working copy) @@ -355,12 +355,12 @@ static volatile svn_atomic_t bdb_cache_state; static svn_error_t * -bdb_init_cb(void) +bdb_init_cb(apr_pool_t *pool) { #if APR_HAS_THREADS apr_status_t apr_err; #endif - bdb_cache_pool = svn_pool_create(NULL); + bdb_cache_pool = svn_pool_create(pool); bdb_cache = apr_hash_make(bdb_cache_pool); #if APR_HAS_THREADS apr_err = apr_thread_mutex_create(&bdb_cache_lock, @@ -380,9 +380,9 @@ } svn_error_t * -svn_fs_bdb__init(void) +svn_fs_bdb__init(apr_pool_t* pool) { - SVN_ERR(svn_atomic__init_once(&bdb_cache_state, bdb_init_cb)); + SVN_ERR(svn_atomic__init_once(&bdb_cache_state, bdb_init_cb, pool)); return SVN_NO_ERROR; } Index: subversion/libsvn_fs_base/bdb/env.h =================================================================== --- subversion/libsvn_fs_base/bdb/env.h (revision 24581) +++ subversion/libsvn_fs_base/bdb/env.h (working copy) @@ -102,7 +102,7 @@ /* Iniitalize the BDB back-end's private stuff. */ -svn_error_t *svn_fs_bdb__init(void); +svn_error_t *svn_fs_bdb__init(apr_pool_t* pool); /* Allocate the Berkeley DB descriptor BDB and open the environment. Index: subversion/libsvn_fs_base/fs.c =================================================================== --- subversion/libsvn_fs_base/fs.c (revision 24581) +++ subversion/libsvn_fs_base/fs.c (working copy) @@ -483,7 +483,6 @@ /* Creating a new filesystem */ static fs_vtable_t fs_vtable = { - base_serialized_init, svn_fs_base__youngest_rev, svn_fs_base__revision_prop, svn_fs_base__revision_proplist, @@ -623,7 +622,8 @@ static svn_error_t * -base_create(svn_fs_t *fs, const char *path, apr_pool_t *pool) +base_create(svn_fs_t *fs, const char *path, apr_pool_t *pool, + apr_pool_t *common_pool) { int format = SVN_FS_BASE__FORMAT_NUMBER; @@ -646,7 +646,7 @@ if (svn_err) goto error; SVN_ERR(svn_fs_merge_info__create_index(path, pool)); - return SVN_NO_ERROR; + return base_serialized_init(fs, common_pool, pool); error: svn_error_clear(cleanup_fs(fs)); @@ -679,7 +679,8 @@ } static svn_error_t * -base_open(svn_fs_t *fs, const char *path, apr_pool_t *pool) +base_open(svn_fs_t *fs, const char *path, apr_pool_t *pool, + apr_pool_t *common_pool) { int format; @@ -706,9 +707,8 @@ /* Now we've got a format number no matter what. */ ((base_fs_data_t *) fs->fsap_data)->format = format; SVN_ERR(check_format(format)); + return base_serialized_init(fs, common_pool, pool); - return SVN_NO_ERROR; - error: svn_error_clear(cleanup_fs(fs)); return svn_err; @@ -750,7 +750,8 @@ } static svn_error_t * -base_open_for_recovery(svn_fs_t *fs, const char *path, apr_pool_t *pool) +base_open_for_recovery(svn_fs_t *fs, const char *path, apr_pool_t *pool, + apr_pool_t *common_pool) { /* Just stash the path in the fs pointer - it's all we really need. */ fs->path = apr_pstrdup(fs->pool, path); @@ -1196,7 +1197,7 @@ svn_error_t * svn_fs_base__init(const svn_version_t *loader_version, - fs_library_vtable_t **vtable) + fs_library_vtable_t **vtable, apr_pool_t* common_pool) { static const svn_version_checklist_t checklist[] = { @@ -1213,7 +1214,7 @@ loader_version->major); SVN_ERR(svn_ver_check_list(base_version(), checklist)); SVN_ERR(check_bdb_version()); - SVN_ERR(svn_fs_bdb__init()); + SVN_ERR(svn_fs_bdb__init(common_pool)); *vtable = &library_vtable; return SVN_NO_ERROR; Index: subversion/include/svn_fs.h =================================================================== --- subversion/include/svn_fs.h (revision 24581) +++ subversion/include/svn_fs.h (working copy) @@ -94,7 +94,10 @@ * effort to bootstrap a mutex for protecting data common to FS * objects; however, there is a small window of failure. Also, a * small amount of data will be leaked if the Subversion FS library is - * dynamically unloaded. + * dynamically unloaded, and using the bdb FS can potentially segfault + * or invoke other undefined behavior if this function is not called + * with an appropriate pool (such as the pool the module was loaded into) + * when loaded dynamically. * * If this function is called multiple times before the pool passed to * the first call is destroyed or cleared, the later calls will have Index: subversion/include/private/svn_atomic.h =================================================================== --- subversion/include/private/svn_atomic.h (revision 24581) +++ subversion/include/private/svn_atomic.h (working copy) @@ -100,13 +100,14 @@ * * @a global_status must be a pointer to a global, zero-initialized * #svn_atomic_t. @a init_func is a pointer to the function that performs - * the actual initialization. + * the actual initialization, and @a pool is passed on to the init_func + * for its use. * * @since New in 1.5. */ svn_error_t * svn_atomic__init_once(volatile svn_atomic_t *global_status, - svn_error_t *(*init_func)(void)); + svn_error_t *(*init_func)(apr_pool_t*), apr_pool_t* pool); #ifdef __cplusplus } Index: subversion/libsvn_fs/fs-loader.h =================================================================== --- subversion/libsvn_fs/fs-loader.h (revision 24581) +++ subversion/libsvn_fs/fs-loader.h (working copy) @@ -66,14 +66,19 @@ this statement, now that the minor version has increased. */ const svn_version_t *(*get_version)(void); - svn_error_t *(*create)(svn_fs_t *fs, const char *path, apr_pool_t *pool); - svn_error_t *(*open)(svn_fs_t *fs, const char *path, apr_pool_t *pool); + /* The open/create/open_for_recovery functions are serialized so that they + may use the common_pool parameter to allocate fs-global objects such as + the bdb env cache. */ + svn_error_t *(*create)(svn_fs_t *fs, const char *path, apr_pool_t *pool, + apr_pool_t *common_pool); + svn_error_t *(*open)(svn_fs_t *fs, const char *path, apr_pool_t *pool, + apr_pool_t *common_pool); /* open_for_recovery() is like open(), but used to fill in an fs pointer - that will be passed to recover() after the filesystem's serialized_init() - is called. We assume that the open() method might not be immediately - appropriate for recovery. */ + that will be passed to recover(). We assume that the open() method + might not be immediately appropriate for recovery. */ svn_error_t *(*open_for_recovery)(svn_fs_t *fs, const char *path, - apr_pool_t *pool); + apr_pool_t *pool, + apr_pool_t *common_pool); svn_error_t *(*delete_fs)(const char *path, apr_pool_t *pool); svn_error_t *(*hotcopy)(const char *src_path, const char *dest_path, svn_boolean_t clean, apr_pool_t *pool); @@ -101,19 +106,28 @@ library vtable. The LOADER_VERSION parameter must remain first in the list, and the function must use the C calling convention on all platforms, so that the init functions can safely read the version - parameter. + parameter. The COMMON_POOL parameter must be a pool with a greater + lifetime than the fs module so that fs global state can be kept + in it and cleaned up on termination before the fs module is unloaded. ### need to force this to be __cdecl on Windows... how?? */ typedef svn_error_t *(*fs_init_func_t)(const svn_version_t *loader_version, - fs_library_vtable_t **vtable); + fs_library_vtable_t **vtable, + apr_pool_t* common_pool); /* Here are the declarations for the FS module init functions. If we are using DSO loading, they won't actually be linked into - libsvn_fs. */ + libsvn_fs. Note that these private functions have been revved + in order to add a common_pool parameter that may be + used for fs module scoped variables such as the bdb cache. This + will be the same common_pool that is passed to the create and open + functions. */ svn_error_t *svn_fs_base__init(const svn_version_t *loader_version, - fs_library_vtable_t **vtable); + fs_library_vtable_t **vtable, + apr_pool_t* common_pool); svn_error_t *svn_fs_fs__init(const svn_version_t *loader_version, - fs_library_vtable_t **vtable); + fs_library_vtable_t **vtable, + apr_pool_t* common_pool); @@ -121,17 +135,6 @@ typedef struct fs_vtable_t { - /* The FS loader library invokes serialized_init after a create or - open call, with the new FS object as its first parameter. Calls - to serialized_init are globally serialized, so the FS module - function has exclusive access to COMMON_POOL. The same - COMMON_POOL will be passed for every FS object created during the - lifetime of the pool passed to svn_fs_initialize(), or during the - lifetime of the process if svn_fs_initialize() is not invoked. - Temporary allocations can be made in POOL. */ - svn_error_t *(*serialized_init)(svn_fs_t *fs, apr_pool_t *common_pool, - apr_pool_t *pool); - svn_error_t *(*youngest_rev)(svn_revnum_t *youngest_p, svn_fs_t *fs, apr_pool_t *pool); svn_error_t *(*revision_prop)(svn_string_t **value_p, svn_fs_t *fs, Index: subversion/libsvn_fs/fs-loader.c =================================================================== --- subversion/libsvn_fs/fs-loader.c (revision 24581) +++ subversion/libsvn_fs/fs-loader.c (working copy) @@ -43,8 +43,7 @@ #define FS_TYPE_FILENAME "fs-type" /* A pool common to all FS objects. See the documentation on the - serialized_init function in fs-loader.h and for - svn_fs_initialize(). */ + open/create functions in fs-loader.h and for svn_fs_initialize(). */ static apr_pool_t *common_pool; #if APR_HAS_THREADS static apr_thread_mutex_t *common_pool_lock; @@ -131,7 +130,33 @@ _("Failed to load module for FS type '%s'"), fst->fs_type); - SVN_ERR(initfunc(my_version, vtable)); + { + apr_status_t status; + + /* Per our API compatibility rules, we cannot ensure that + svn_fs_initialize is called by the application. If not, we + cannot create the common pool and lock in a thread-safe fashion, + nor can we clean up the common pool if libsvn_fs is dynamically + unloaded. This function makes a best effort by creating the + common pool as a child of the global pool; the window of failure + due to thread collision is small. */ + if (!common_pool) + SVN_ERR(svn_fs_initialize(NULL)); + + /* Invoke the FS module's initfunc function with the common + pool protected by a lock. */ +#if APR_HAS_THREADS + status = apr_thread_mutex_lock(common_pool_lock); + if (status) + return svn_error_wrap_apr(status, _("Can't grab FS mutex")); +#endif + SVN_ERR(initfunc(my_version, vtable, common_pool)); +#if APR_HAS_THREADS + status = apr_thread_mutex_unlock(common_pool_lock); + if (status) + return svn_error_wrap_apr(status, _("Can't ungrab FS mutex")); +#endif + } fs_version = (*vtable)->get_version(); if (!svn_ver_equal(my_version, fs_version)) return svn_error_createf(SVN_ERR_VERSION_MISMATCH, NULL, @@ -264,37 +289,27 @@ } static svn_error_t * -serialized_init(svn_fs_t *fs, apr_pool_t *pool) +acquire_fs_mutex(void) { - svn_error_t *err; #if APR_HAS_THREADS apr_status_t status; -#endif - - /* Per our API compatibility rules, we cannot ensure that - svn_fs_initialize is called by the application. If not, we - cannot create the common pool and lock in a thread-safe fashion, - nor can we clean up the common pool if libsvn_fs is dynamically - unloaded. This function makes a best effort by creating the - common pool as a child of the global pool; the window of failure - due to thread collision is small. */ - if (!common_pool) - SVN_ERR(svn_fs_initialize(NULL)); - - /* Invoke the FS module's serialized_init function with the common - pool protected by a lock. */ -#if APR_HAS_THREADS status = apr_thread_mutex_lock(common_pool_lock); if (status) return svn_error_wrap_apr(status, _("Can't grab FS mutex")); #endif - err = fs->vtable->serialized_init(fs, common_pool, pool); + return SVN_NO_ERROR; +} + +static svn_error_t * +release_fs_mutex(void) +{ #if APR_HAS_THREADS + apr_status_t status; status = apr_thread_mutex_unlock(common_pool_lock); - if (status && !err) + if (status) return svn_error_wrap_apr(status, _("Can't ungrab FS mutex")); #endif - return err; + return SVN_NO_ERROR; } /* A default warning handling function. */ @@ -336,6 +351,8 @@ svn_fs_create(svn_fs_t **fs_p, const char *path, apr_hash_t *fs_config, apr_pool_t *pool) { + svn_error_t *err; + svn_error_t *err2; fs_library_vtable_t *vtable; const char *fs_type = NULL; @@ -352,20 +369,36 @@ /* Perform the actual creation. */ *fs_p = svn_fs_new(fs_config, pool); - SVN_ERR(vtable->create(*fs_p, path, pool)); - return serialized_init(*fs_p, pool); + SVN_ERR(acquire_fs_mutex()); + err = vtable->create(*fs_p, path, pool, common_pool); + err2 = release_fs_mutex(); + if (err2) + { + svn_error_clear(err); + return err2; + } + return err; } svn_error_t * svn_fs_open(svn_fs_t **fs_p, const char *path, apr_hash_t *fs_config, apr_pool_t *pool) { + svn_error_t *err; + svn_error_t *err2; fs_library_vtable_t *vtable; SVN_ERR(fs_library_vtable(&vtable, path, pool)); *fs_p = svn_fs_new(fs_config, pool); - SVN_ERR(vtable->open(*fs_p, path, pool)); - return serialized_init(*fs_p, pool); + SVN_ERR(acquire_fs_mutex()); + err = vtable->open(*fs_p, path, pool, common_pool); + err2 = release_fs_mutex(); + if (err2) + { + svn_error_clear(err); + return err2; + } + return err; } const char * @@ -403,14 +436,24 @@ svn_cancel_func_t cancel_func, void *cancel_baton, apr_pool_t *pool) { + svn_error_t *err; + svn_error_t *err2; fs_library_vtable_t *vtable; svn_fs_t *fs; SVN_ERR(fs_library_vtable(&vtable, path, pool)); fs = svn_fs_new(NULL, pool); - SVN_ERR(vtable->open_for_recovery(fs, path, pool)); - SVN_ERR(serialized_init(fs, pool)); - return vtable->recover(fs, cancel_func, cancel_baton, pool); + SVN_ERR(acquire_fs_mutex()); + err = vtable->open_for_recovery(fs, path, pool, common_pool); + err2 = release_fs_mutex(); + if (err2) + { + svn_error_clear(err); + return err2; + } + if (! err) + err = vtable->recover(fs, cancel_func, cancel_baton, pool); + return err; } @@ -419,6 +462,8 @@ svn_error_t * svn_fs_create_berkeley(svn_fs_t *fs, const char *path) { + svn_error_t *err; + svn_error_t *err2; fs_library_vtable_t *vtable; SVN_ERR(get_library_vtable(&vtable, SVN_FS_TYPE_BDB, fs->pool)); @@ -428,18 +473,34 @@ SVN_ERR(write_fs_type(path, SVN_FS_TYPE_BDB, fs->pool)); /* Perform the actual creation. */ - SVN_ERR(vtable->create(fs, path, fs->pool)); - return serialized_init(fs, fs->pool); + SVN_ERR(acquire_fs_mutex()); + err = vtable->create(fs, path, fs->pool, common_pool); + err2 = release_fs_mutex(); + if (err2) + { + svn_error_clear(err); + return err2; + } + return err; } svn_error_t * svn_fs_open_berkeley(svn_fs_t *fs, const char *path) { + svn_error_t *err; + svn_error_t *err2; fs_library_vtable_t *vtable; SVN_ERR(fs_library_vtable(&vtable, path, fs->pool)); - SVN_ERR(vtable->open(fs, path, fs->pool)); - return serialized_init(fs, fs->pool); + SVN_ERR(acquire_fs_mutex()); + err = vtable->open(fs, path, fs->pool, common_pool); + err2 = release_fs_mutex(); + if (err2) + { + svn_error_clear(err); + return err2; + } + return err; } const char * Index: subversion/libsvn_subr/atomic.c =================================================================== --- subversion/libsvn_subr/atomic.c (revision 24581) +++ subversion/libsvn_subr/atomic.c (working copy) @@ -26,7 +26,7 @@ svn_error_t* svn_atomic__init_once(volatile svn_atomic_t *global_status, - svn_error_t *(*init_func)(void)) + svn_error_t *(*init_func)(apr_pool_t*), apr_pool_t* pool) { /* We have to call init_func exactly once. Because APR doesn't have statically-initialized mutexes, we implement a poor @@ -37,7 +37,7 @@ if (status == SVN_ATOMIC_UNINITIALIZED) { - svn_error_t *err = init_func(); + svn_error_t *err = init_func(pool); if (err) { #ifdef APR_HAS_THREADS Index: subversion/libsvn_fs_fs/fs.c =================================================================== --- subversion/libsvn_fs_fs/fs.c (revision 24581) +++ subversion/libsvn_fs_fs/fs.c (working copy) @@ -140,7 +140,6 @@ /* The vtable associated with a specific open filesystem. */ static fs_vtable_t fs_vtable = { - fs_serialized_init, svn_fs_fs__youngest_rev, svn_fs_fs__revision_prop, svn_fs_fs__revision_proplist, @@ -166,9 +165,11 @@ /* This implements the fs_library_vtable_t.create() API. Create a new fsfs-backed Subversion filesystem at path PATH and link it into - *FS. Perform temporary allocations in POOL. */ + *FS. Perform temporary allocations in POOL, and fs-global allocations + in COMMON_POOL. */ static svn_error_t * -fs_create(svn_fs_t *fs, const char *path, apr_pool_t *pool) +fs_create(svn_fs_t *fs, const char *path, apr_pool_t *pool, + apr_pool_t *common_pool) { fs_fs_data_t *ffd; @@ -179,8 +180,7 @@ fs->fsap_data = ffd; SVN_ERR(svn_fs_fs__create(fs, path, pool)); - - return SVN_NO_ERROR; + return fs_serialized_init(fs, common_pool, pool); } @@ -190,9 +190,10 @@ /* This implements the fs_library_vtable_t.open() API. Open an FSFS Subversion filesystem located at PATH, set *FS to point to the correct vtable for the filesystem. Use POOL for any temporary - allocations. */ + allocations, and COMMON_POOL for fs-global allocations. */ static svn_error_t * -fs_open(svn_fs_t *fs, const char *path, apr_pool_t *pool) +fs_open(svn_fs_t *fs, const char *path, apr_pool_t *pool, + apr_pool_t *common_pool) { fs_fs_data_t *ffd; @@ -201,8 +202,7 @@ fs->fsap_data = ffd; SVN_ERR(svn_fs_fs__open(fs, path, pool)); - - return SVN_NO_ERROR; + return fs_serialized_init(fs, common_pool, pool); } @@ -211,7 +211,7 @@ static svn_error_t * fs_open_for_recovery(svn_fs_t *fs, const char *path, - apr_pool_t *pool) + apr_pool_t *pool, apr_pool_t *common_pool) { /* Recovery for FSFS is currently limited to recreating the current file from the latest revision. */ @@ -229,7 +229,7 @@ "0 1 1\n", pool)); /* Now open the filesystem properly by calling the vtable method directly. */ - return fs_open(fs, path, pool); + return fs_open(fs, path, pool, common_pool); } @@ -308,7 +308,7 @@ svn_error_t * svn_fs_fs__init(const svn_version_t *loader_version, - fs_library_vtable_t **vtable) + fs_library_vtable_t **vtable, apr_pool_t* common_pool) { static const svn_version_checklist_t checklist[] = {