On Feb 7, 2008 2:55 PM, Eric Gillespie <epg_at_google.com> wrote:
> "David Glasser" <glasser_at_davidglasser.net> writes:
>
> > On Feb 7, 2008 2:30 PM, Eric Gillespie <epg_at_google.com> wrote:
> > > "David Glasser" <glasser_at_davidglasser.net> writes:
> > >
> > > > 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.
> > > > ]]]
> > >
> > > Looks like it, but why allocate it twice? First you changed the
> > > pool from which you allocate rev_str, then you pstrdup it when
> > > adding to the cache.
> >
> > Well, unless we want to move the cache-clearing code to before we
> > actually know whether or not we're adding a new value, we really have
> > to allocate it twice, I think... am I missing an easy way out here?
>
> Ah, you're right. The only other thing I could think of is this
> (edited from your patch, not likely to apply), but it's no better:
>
> @@ -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);
> + /* Just wiped out rev_str, have to make it again. */
> + rev_str = apr_psprintf(ffd->rev_root_id_cache_pool, "%ld", rev);
>
> }
> apr_hash_set(ffd->rev_root_id_cache, rev_str, APR_HASH_KEY_STRING,
Yup. In fact my original code and your patch here both essentially
have a mem leak: if you call this routine a million times on the same
<4096 entries, you'll keep reallocating strings in the cache pool
which never gets cleared.
--dave
--
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
Received on 2008-02-08 00:22:17 CET