On Mon, Nov 08, 2010 at 06:56:16AM -0600, Hyrum K. Wright wrote:
> On Sun, Nov 7, 2010 at 11:58 AM, Stefan Sperling <s...@elego.de> wrote:
> > [[[
> > * subversion/libsvn_fs_util/caching.c
> > (svn_fs__get_global_membuffer_cache): Fix a possible error leak.
> > ]]]
> >
> > Index: subversion/libsvn_fs_util/caching.c
> > ===================================================================
> > --- subversion/libsvn_fs_util/caching.c (revision 1032308)
> > +++ subversion/libsvn_fs_util/caching.c (working copy)
> > @@ -62,6 +62,7 @@ svn_fs__get_global_membuffer_cache(void)
> > /* auto-allocate cache*/
> > apr_allocator_t *allocator = NULL;
> > apr_pool_t *pool = NULL;
> > + svn_error_t *err;
> >
> > if (apr_allocator_create(&allocator))
> > return NULL;
> > @@ -75,12 +76,17 @@ svn_fs__get_global_membuffer_cache(void)
> > apr_allocator_max_free_set(allocator, 1);
> > pool = svn_pool_create_ex(NULL, allocator);
> >
> > - svn_cache__membuffer_cache_create
> > - (&cache,
> > - (apr_size_t)cache_size,
> > - (apr_size_t)(cache_size / 16),
> > - ! svn_fs_get_cache_config()->single_threaded,
> > - pool);
>
> If we're choosing to ignore the error above, you can just wrap the
> entire call in an svn_error_clear(). No need to introduce and check a
> temp variable.
Ah, nice trick. Thanks.
> > + err = svn_cache__membuffer_cache_create
> > + (&cache,
> > + (apr_size_t)cache_size,
> > + (apr_size_t)(cache_size / 16),
> > + ! svn_fs_get_cache_config()->single_threaded,
> > + pool);
> > + if (err)
> > + {
> > + svn_error_clear(err);
> > + return NULL;
> > + }
>
> Does this early return actually change functionality? If so, this
> patch is about more than just fixing an error leak...
It doesn't change functionality.
Cache is NULL in this case, and we return it next:
>
> > }
> >
> > return cache;
> >
I'm not sure though if svn_cache__membuffer_cache_create() guarantees
that cache remains NULL in case of error. Maybe this should be
documented as such to allow callers to ignore errors this way.
New patch below, also fixing a space-before-paren and another
similar error leak.
[[[
* subversion/libsvn_fs_util/caching.c
(svn_fs__get_global_membuffer_cache,
svn_fs__get_global_file_handle_cache): Fix a possible error leak.
]]]
Index: subversion/libsvn_fs_util/caching.c
===================================================================
--- subversion/libsvn_fs_util/caching.c (revision 1032333)
+++ subversion/libsvn_fs_util/caching.c (working copy)
@@ -75,12 +75,12 @@ svn_fs__get_global_membuffer_cache(void)
apr_allocator_max_free_set(allocator, 1);
pool = svn_pool_create_ex(NULL, allocator);
- svn_cache__membuffer_cache_create
- (&cache,
- (apr_size_t)cache_size,
- (apr_size_t)(cache_size / 16),
- ! svn_fs_get_cache_config()->single_threaded,
- pool);
+ svn_error_clear(svn_cache__membuffer_cache_create(
+ &cache,
+ (apr_size_t)cache_size,
+ (apr_size_t)(cache_size / 16),
+ ! svn_fs_get_cache_config()->single_threaded,
+ pool));
}
return cache;
@@ -96,10 +96,10 @@ svn_fs__get_global_file_handle_cache(void)
static svn_file_handle_cache_t *cache = NULL;
if (!cache)
- svn_file_handle_cache__create_cache(&cache,
- cache_settings.file_handle_count,
- !cache_settings.single_threaded,
- svn_pool_create(NULL));
+ svn_error_clear(svn_file_handle_cache__create_cache(
+ &cache, cache_settings.file_handle_count,
+ !cache_settings.single_threaded,
+ svn_pool_create(NULL)));
return cache;
}
