Re: svn commit: r27098 - trunk/subversion/libsvn_fs_fs

On Feb 6, 2008 1:55 PM, Eric Gillespie <epg_at_google.com> wrote:
> glasser_at_tigris.org writes:
>
> > Modified: trunk/subversion/libsvn_fs_fs/fs_fs.c
> > URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_fs_fs/fs_fs.c?pathrev=27098&r1=27097&r2=27098
> > ==============================================================================
> > --- trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> > +++ trunk/subversion/libsvn_fs_fs/fs_fs.c Wed Oct 10 17:11:58 2007
> > @@ -1748,14 +1748,15 @@
> > apr_off_t root_offset;
> > svn_fs_id_t *root_id;
> > svn_error_t *err;
> > - unsigned int hid;
> > + const char *rev_str = apr_psprintf(ffd->rev_root_id_cache_pool, "%ld", rev);
>
> Allocate rev_str from ffd->rev_root_id_cache_pool...
>
>
> > svn_fs_id_t *cached_id;
> >
> > /* Calculate an index into the revroot id cache */
> > - hid = RRI_CACHE_ENTRIES_MASK(rev);
> > - cached_id = ffd->rev_root_id_cache[hid];
> > + cached_id = apr_hash_get(ffd->rev_root_id_cache,
> > + rev_str,
> > + APR_HASH_KEY_STRING);
> >
> > - if (cached_id && rev == svn_fs_fs__id_rev(cached_id))
> > + if (cached_id)
> > {
> > *root_id_p = svn_fs_fs__id_copy(cached_id, pool);
> > return SVN_NO_ERROR;
> > @@ -1779,14 +1780,16 @@
> >
> > SVN_ERR(svn_io_file_close(revision_file, pool));
> >
> > - /* Cache it (and a pool) */
> > - ffd->rev_root_id_cache[hid] = NULL;
> > - if (ffd->rev_root_id_cache_pool[hid])
> > - svn_pool_clear(ffd->rev_root_id_cache_pool[hid]);
> > - else
> > - ffd->rev_root_id_cache_pool[hid] = svn_pool_create(fs->pool);
> > - ffd->rev_root_id_cache[hid]
> > - = svn_fs_fs__id_copy(root_id, ffd->rev_root_id_cache_pool[hid]);
> > + /* Cache it */
> > + if (apr_hash_count(ffd->rev_root_id_cache) >= NUM_RRI_CACHE_ENTRIES)
> > + {
> > + /* In order to only use one pool for the whole cache, we need to
> > + * completely wipe it to expire entries! */
> > + svn_pool_clear(ffd->rev_root_id_cache_pool);
>
> Clear ffd->rev_root_id_cache_pool...
>
> > + ffd->rev_root_id_cache = apr_hash_make(ffd->rev_root_id_cache_pool);
> > + }
> > + apr_hash_set(ffd->rev_root_id_cache, rev_str, APR_HASH_KEY_STRING,
> > + svn_fs_fs__id_copy(root_id, ffd->rev_root_id_cache_pool));
>
> And use rev_str. Boom!
>
> Yeah, we just hit a svnadmin dump segfault on this one ;->

Does this fix it?

[[[
Followup to r27098: fix potential segfault by using pools correctly.

* subversion/libsvn_fs_fs/fs_fs.c
  (svn_fs_fs__rev_get_root): Keep a string out of a pool that might
   be cleared.
]]]

Index: /opt/svn/checkouts/trunk/subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- /opt/svn/checkouts/trunk/subversion/libsvn_fs_fs/fs_fs.c (revision 29154)
+++ /opt/svn/checkouts/trunk/subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -2043,7 +2043,7 @@ svn_fs_fs__rev_get_root(svn_fs_id_t **ro
   apr_off_t root_offset;
   svn_fs_id_t *root_id;
   svn_error_t *err;
- const char *rev_str = apr_psprintf(ffd->rev_root_id_cache_pool, "%ld", rev);
+ const char *rev_str = apr_psprintf(pool, "%ld", rev);
   svn_fs_id_t *cached_id;

   SVN_ERR(ensure_revision_exists(fs, rev, pool));
@@ -2077,7 +2077,7 @@ svn_fs_fs__rev_get_root(svn_fs_id_t **ro

   SVN_ERR(svn_io_file_close(revision_file, pool));

- /* Cache it */
+ /* Make sure our cache size doesn't grow without bounds. */
   if (apr_hash_count(ffd->rev_root_id_cache) >= NUM_RRI_CACHE_ENTRIES)
     {
       /* In order to only use one pool for the whole cache, we need to
@@ -2085,7 +2085,12 @@ svn_fs_fs__rev_get_root(svn_fs_id_t **ro
       svn_pool_clear(ffd->rev_root_id_cache_pool);
       ffd->rev_root_id_cache = apr_hash_make(ffd->rev_root_id_cache_pool);
     }
- apr_hash_set(ffd->rev_root_id_cache, rev_str, APR_HASH_KEY_STRING,
+
+ /* Cache the answer, copying both the key and value into the cache's
+ pool. */
+ apr_hash_set(ffd->rev_root_id_cache,
+ apr_pstrdup(ffd->rev_root_id_cache_pool, rev_str),
+ APR_HASH_KEY_STRING,
                svn_fs_fs__id_copy(root_id, ffd->rev_root_id_cache_pool));

   *root_id_p = root_id;

-- 
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org