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

Re: [PATCH] error leak on performance branch

From: Stefan Sperling <stsp_at_elego.de>
Date: Mon, 8 Nov 2010 17:33:56 +0100

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 <stsp_at_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;
 }
Received on 2010-11-08 17:34:40 CET

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