Re: Error leak in libsvn_fs_base/bdb/env.c

On 1/11/07, Branko Èibej <brane@xbc.nu> wrote:
> Looks O.K. ... but I'd be careful here. It's been a while since I wrote
> that code, but I do recall strange things happening if I wasn't careful
> about the pending errors list.

Ok, I think I now have a better understanding of what's going on. It's
a combination of three things:

a) The apr_threadkey_* APIs do not support destructors on Windows, so
the cleanup_error_info callback, which is supposed to be the last
resort for cleaning up errors, never gets called

b) bdb_close() leaks the pending errors if bdb->env->close() returns DB_RECOVERY

c) svn_fs_bdb__close() is also supposed to clean everything up, but it
does so before calling bdb_close()

I think the simplest fix would be to move the cleanup code in
svn_fs_bdb__close() near the end of the function, after the call to
bdb_close(). This is also the only way we can avoid leaking a
bdb_error_info_t structure, which would be calloc()ed when
db->env->close() invokes our error callback.

Index: subversion/libsvn_fs_base/bdb/env.c
===================================================================
--- subversion/libsvn_fs_base/bdb/env.c (revision 22970)
+++ subversion/libsvn_fs_base/bdb/env.c (working copy)
@@ -515,19 +515,6 @@
       cleanup_env_baton(). */
    bdb_baton->bdb = NULL;

- /* Note that we only bother with this cleanup if the pool is non-NULL, to
- guard against potential races between this and the cleanup_env cleanup
- callback. It's not clear if that can actually happen, but better safe
- than sorry. */
- if (0 == --bdb_baton->error_info->refcount && bdb->pool)
- {
- svn_error_clear(bdb_baton->error_info->pending_errors);
-#if APR_HAS_THREADS
- free(bdb_baton->error_info);
- apr_threadkey_private_set(NULL, bdb->error_info);
-#endif
- }
-
    acquire_cache_mutex();
    if (--bdb->refcount != 0)
      {
@@ -550,6 +537,19 @@
        err = bdb_close(bdb);
        release_cache_mutex();
      }
+
+ /* Note that we only bother with this cleanup if the pool is non-NULL, to
+ guard against potential races between this and the cleanup_env cleanup
+ callback. It's not clear if that can actually happen, but better safe
+ than sorry. */
+ if (0 == --bdb_baton->error_info->refcount && bdb->pool)
+ {
+ svn_error_clear(bdb_baton->error_info->pending_errors);
+#if APR_HAS_THREADS
+ free(bdb_baton->error_info);
+#endif
+ }
+
    return err;
  }

-- 
Vlad