You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Jim Blandy <ji...@zwingli.cygnus.com> on 2001/02/27 23:42:45 UTC
Re: CVS update: subversion/subversion/libsvn_fs id.c
This is unnecessary. apr_pcalloc behaves this way automatically
whenever its pool argument is zero.
sussman@tigris.org writes:
>
> User: sussman
> Date: 01/02/27 14:58:08
>
> Modified: subversion/libsvn_fs id.c
> Log:
> (svn_fs_parse_id): bugfix: make this func use malloc if no pool is
> given, as described in its documentation.
>
> Revision Changes Path
> 1.16 +8 -1 subversion/subversion/libsvn_fs/id.c
>
> Index: id.c
> ===================================================================
> RCS file: /cvs/subversion/subversion/libsvn_fs/id.c,v
> retrieving revision 1.15
> retrieving revision 1.16
> diff -u -r1.15 -r1.16
> --- id.c 2001/02/12 00:26:14 1.15
> +++ id.c 2001/02/27 22:58:08 1.16
> @@ -191,7 +191,14 @@
>
> /* Allocate the ID array. Note that if pool is zero, apr_palloc
> just calls malloc, which meets our promised interface. */
> - id = apr_palloc (pool, sizeof (*id) * (id_len + 1));
> + if (pool)
> + id = apr_palloc (pool, sizeof (*id) * (id_len + 1));
> + else
> + {
> + id = malloc (sizeof (*id) * (id_len + 1));
> + if (! id)
> + abort(); /* couldn't malloc */
> + }
>
> {
> int i = 0;
>
>
>
>
Re: CVS update: subversion/subversion/libsvn_fs id.c
Posted by Jim Blandy <ji...@zwingli.cygnus.com>.
By gum, you're right. I had the call to malloc there in id.c
originally, but Greg S. removed it in revision 1.8. apr_palloc must
have changed since then.
Frankly, I don't blame them. The only advantage of malloc over
apr_palloc is that you can free the block, but every piece of code
either:
1) doesn't assume that pool is zero, and thus can't call free, or
2) does assume that pool is zero, and thus might as well call malloc
directly and avoid confusion.
Ben Collins-Sussman <su...@newton.ch.collab.net> writes:
> Uhhhhh, nope. apr_pcalloc was segfaulting on both me and Mike
> (fs-test #5). Adding the malloc fixed this.
>
> Maybe it's time to debug apr. :)
>
> Jim Blandy <ji...@zwingli.cygnus.com> writes:
>
> > This is unnecessary. apr_pcalloc behaves this way automatically
> > whenever its pool argument is zero.
> >
> > sussman@tigris.org writes:
> >
> > >
> > > User: sussman
> > > Date: 01/02/27 14:58:08
> > >
> > > Modified: subversion/libsvn_fs id.c
> > > Log:
> > > (svn_fs_parse_id): bugfix: make this func use malloc if no pool is
> > > given, as described in its documentation.
> > >
> > > Revision Changes Path
> > > 1.16 +8 -1 subversion/subversion/libsvn_fs/id.c
> > >
> > > Index: id.c
> > > ===================================================================
> > > RCS file: /cvs/subversion/subversion/libsvn_fs/id.c,v
> > > retrieving revision 1.15
> > > retrieving revision 1.16
> > > diff -u -r1.15 -r1.16
> > > --- id.c 2001/02/12 00:26:14 1.15
> > > +++ id.c 2001/02/27 22:58:08 1.16
> > > @@ -191,7 +191,14 @@
> > >
> > > /* Allocate the ID array. Note that if pool is zero, apr_palloc
> > > just calls malloc, which meets our promised interface. */
> > > - id = apr_palloc (pool, sizeof (*id) * (id_len + 1));
> > > + if (pool)
> > > + id = apr_palloc (pool, sizeof (*id) * (id_len + 1));
> > > + else
> > > + {
> > > + id = malloc (sizeof (*id) * (id_len + 1));
> > > + if (! id)
> > > + abort(); /* couldn't malloc */
> > > + }
> > >
> > > {
> > > int i = 0;
> > >
> > >
> > >
> > >
>
Re: CVS update: subversion/subversion/libsvn_fs id.c
Posted by Jim Blandy <ji...@zwingli.cygnus.com>.
Greg Stein <gs...@lyra.org> writes:
> Another alternative is to stash a pool into app_private field of the DBT
> whenever you create it. Hmm... I wonder if that will be set properly for a
> key pulled out of the database.
Yes, exactly. The DBT's passed to the key comparison function aren't
necessarily DBT's you've created.
> I recognize that compare_nodes_keys() isn't leaking memory, but
> (accidentally) passing NULL to svn_fs_parse_id() from some other call point
> could introduce leakage.
Yeah. It would be possible to create an internal function which
actually does the parsing and can handle either a pool or zero, and
make the public function svn_fs_parse_id check its argument, and then
call that. nodes-table.c could call the internal function directly.
If you want to clean that up, go ahead.
Re: CVS update: subversion/subversion/libsvn_fs id.c
Posted by Greg Stein <gs...@lyra.org>.
Another alternative is to stash a pool into app_private field of the DBT
whenever you create it. Hmm... I wonder if that will be set properly for a
key pulled out of the database.
I recognize that compare_nodes_keys() isn't leaking memory, but
(accidentally) passing NULL to svn_fs_parse_id() from some other call point
could introduce leakage.
Oy...
Cheers,
-g
On Wed, Feb 28, 2001 at 10:27:05AM -0500, Jim Blandy wrote:
>
> Greg Stein <gs...@lyra.org> writes:
> > So the question becomes: wtf was NULL passed? I'm wary of anything that is
> > using malloc rather than a pool. IMO, we should always use a pool and never
> > fall back to malloc. Consider: who is calling free() on that thing?
>
> Don't fret --- we are of one mind on that issue.
>
> The documentation for svn_fs_parse_id reads:
>
> Allocate the parsed ID in POOL. If POOL is zero, malloc the ID; we
> need this in certain cases where we can't pass in a pool, but it's
> generally best to use a pool whenever possible. */
>
> I've added the following comment to nodes-table.c, above
> compare_nodes_keys, which is the only place where we use this:
>
> NOTE WELL: this function and its helpers use `malloc' to get space
> for the parsed node revision ID's. In general, we try to use pools
> for everything in Subversion, but in this case it's not practical.
> Berkeley DB doesn't provide any way to pass a baton through to the
> btree comparison function. Even if it did, since Berkeley DB needs
> to invoke the comparison function at pretty arbitrary times, you'd
> have to pass the baton to almost every Berkeley DB operation. You
> could stuff a pool pointer in a global variable, but then you'd
> have to make sure the pool was up to date before every Berkeley DB
> operation; you'd surely forget, leading to crashes... Using malloc
> is more maintainable. Since the comparison function isn't allowed
> to signal an error anyway, the need for pools isn't quite as urgent
> as in other code, but we still need to take care. */
--
Greg Stein, http://www.lyra.org/
Re: CVS update: subversion/subversion/libsvn_fs id.c
Posted by Jim Blandy <ji...@zwingli.cygnus.com>.
Greg Stein <gs...@lyra.org> writes:
> So the question becomes: wtf was NULL passed? I'm wary of anything that is
> using malloc rather than a pool. IMO, we should always use a pool and never
> fall back to malloc. Consider: who is calling free() on that thing?
Don't fret --- we are of one mind on that issue.
The documentation for svn_fs_parse_id reads:
Allocate the parsed ID in POOL. If POOL is zero, malloc the ID; we
need this in certain cases where we can't pass in a pool, but it's
generally best to use a pool whenever possible. */
I've added the following comment to nodes-table.c, above
compare_nodes_keys, which is the only place where we use this:
NOTE WELL: this function and its helpers use `malloc' to get space
for the parsed node revision ID's. In general, we try to use pools
for everything in Subversion, but in this case it's not practical.
Berkeley DB doesn't provide any way to pass a baton through to the
btree comparison function. Even if it did, since Berkeley DB needs
to invoke the comparison function at pretty arbitrary times, you'd
have to pass the baton to almost every Berkeley DB operation. You
could stuff a pool pointer in a global variable, but then you'd
have to make sure the pool was up to date before every Berkeley DB
operation; you'd surely forget, leading to crashes... Using malloc
is more maintainable. Since the comparison function isn't allowed
to signal an error anyway, the need for pools isn't quite as urgent
as in other code, but we still need to take care. */
Re: CVS update: subversion/subversion/libsvn_fs id.c
Posted by Greg Stein <gs...@lyra.org>.
Originally, you could pass NULL to get a malloc. Jim had written that
if/else just like Ben did, and I went and "simplified" it. Then APR had a
mindset change and said "NULL pools ar Bad" and removed that capability. So
now we segfault if we apss NULL.
So the question becomes: wtf was NULL passed? I'm wary of anything that is
using malloc rather than a pool. IMO, we should always use a pool and never
fall back to malloc. Consider: who is calling free() on that thing?
Cheers,
-g
p.s. note the comment must be changed
On Tue, Feb 27, 2001 at 05:52:59PM -0600, Ben Collins-Sussman wrote:
>
> Uhhhhh, nope. apr_pcalloc was segfaulting on both me and Mike
> (fs-test #5). Adding the malloc fixed this.
>
> Maybe it's time to debug apr. :)
>
> Jim Blandy <ji...@zwingli.cygnus.com> writes:
>
> > This is unnecessary. apr_pcalloc behaves this way automatically
> > whenever its pool argument is zero.
> >
> > sussman@tigris.org writes:
> >
> > >
> > > User: sussman
> > > Date: 01/02/27 14:58:08
> > >
> > > Modified: subversion/libsvn_fs id.c
> > > Log:
> > > (svn_fs_parse_id): bugfix: make this func use malloc if no pool is
> > > given, as described in its documentation.
> > >
> > > Revision Changes Path
> > > 1.16 +8 -1 subversion/subversion/libsvn_fs/id.c
> > >
> > > Index: id.c
> > > ===================================================================
> > > RCS file: /cvs/subversion/subversion/libsvn_fs/id.c,v
> > > retrieving revision 1.15
> > > retrieving revision 1.16
> > > diff -u -r1.15 -r1.16
> > > --- id.c 2001/02/12 00:26:14 1.15
> > > +++ id.c 2001/02/27 22:58:08 1.16
> > > @@ -191,7 +191,14 @@
> > >
> > > /* Allocate the ID array. Note that if pool is zero, apr_palloc
> > > just calls malloc, which meets our promised interface. */
> > > - id = apr_palloc (pool, sizeof (*id) * (id_len + 1));
> > > + if (pool)
> > > + id = apr_palloc (pool, sizeof (*id) * (id_len + 1));
> > > + else
> > > + {
> > > + id = malloc (sizeof (*id) * (id_len + 1));
> > > + if (! id)
> > > + abort(); /* couldn't malloc */
> > > + }
> > >
> > > {
> > > int i = 0;
> > >
> > >
> > >
> > >
--
Greg Stein, http://www.lyra.org/
Re: CVS update: subversion/subversion/libsvn_fs id.c
Posted by Ben Collins-Sussman <su...@newton.ch.collab.net>.
Uhhhhh, nope. apr_pcalloc was segfaulting on both me and Mike
(fs-test #5). Adding the malloc fixed this.
Maybe it's time to debug apr. :)
Jim Blandy <ji...@zwingli.cygnus.com> writes:
> This is unnecessary. apr_pcalloc behaves this way automatically
> whenever its pool argument is zero.
>
> sussman@tigris.org writes:
>
> >
> > User: sussman
> > Date: 01/02/27 14:58:08
> >
> > Modified: subversion/libsvn_fs id.c
> > Log:
> > (svn_fs_parse_id): bugfix: make this func use malloc if no pool is
> > given, as described in its documentation.
> >
> > Revision Changes Path
> > 1.16 +8 -1 subversion/subversion/libsvn_fs/id.c
> >
> > Index: id.c
> > ===================================================================
> > RCS file: /cvs/subversion/subversion/libsvn_fs/id.c,v
> > retrieving revision 1.15
> > retrieving revision 1.16
> > diff -u -r1.15 -r1.16
> > --- id.c 2001/02/12 00:26:14 1.15
> > +++ id.c 2001/02/27 22:58:08 1.16
> > @@ -191,7 +191,14 @@
> >
> > /* Allocate the ID array. Note that if pool is zero, apr_palloc
> > just calls malloc, which meets our promised interface. */
> > - id = apr_palloc (pool, sizeof (*id) * (id_len + 1));
> > + if (pool)
> > + id = apr_palloc (pool, sizeof (*id) * (id_len + 1));
> > + else
> > + {
> > + id = malloc (sizeof (*id) * (id_len + 1));
> > + if (! id)
> > + abort(); /* couldn't malloc */
> > + }
> >
> > {
> > int i = 0;
> >
> >
> >
> >