svn_error_compose and (potential) memory leaks

[Branko Čibej <br...@xbc.nu>]

The latest changes in the error functions is nice, but I think I found a
potential problem. When we create a new error, we reuse the child's pool
or create a new one; and that's fine, becsue it means that
svn_error_clear only has to destroy the first pool it gets it hands on
-- which is exactly what it does.

Except when we call svn_error_compose to splice two errors together.
When that happens (I count six places right now), we get a chain of
errors that aren't all allocated from the same pool, which means that
svn_error_clear is bound to leak memory.

Any ideas how to fix this? I'd thought about splicing the pool
hierarchy, but that sounds like a bit of a cruddy hack.

-- 
Brane Čibej   <br...@xbc.nu>   http://www.xbc.nu/brane/


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn_error_compose and (potential) memory leaks

[Philip Martin <ph...@codematters.co.uk>]

Greg Hudson <gh...@MIT.EDU> writes:

>   In commit.c and copy.c, this means not reporting errors which might
> occur in svn_wc_adm_close() and remove_tmpfiles() when there is an error
> bumping revisions or committing.  (I'm not sure why we're not just
> relying on pool cleanup handlers, really; then we could just use SVN_ERR
> instead of going through all these hoops.)

Probably because the code was written before there was much (or any?)
use of pool cleanup handlers in Subversion.  As I recall, when I added
a pool cleanup handler to the wc diff code there was only one other
handler in the Subversion code.

-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn_error_compose and (potential) memory leaks

[Greg Hudson <gh...@MIT.EDU>]

On Tue, 2002-11-12 at 12:21, Branko Čibej wrote:
> Except when we call svn_error_compose to splice two errors together.
> When that happens (I count six places right now), we get a chain of
> errors that aren't all allocated from the same pool, which means that
> svn_error_clear is bound to leak memory.

I hadn't thought of this.  I can see two solutions:

  * Ditch svn_error_compose().  It's used in three places (three times
in libsvn_client/commit.c, twice in libsvn_client/copy.c, once in
clients/cmdline/util.c).  We could argue that error chains should never
be composed because errors should always flow from most specific to
least specific.

  In commit.c and copy.c, this means not reporting errors which might
occur in svn_wc_adm_close() and remove_tmpfiles() when there is an error
bumping revisions or committing.  (I'm not sure why we're not just
relying on pool cleanup handlers, really; then we could just use SVN_ERR
instead of going through all these hoops.)

  In clients/cmdline/util.c, in the unlikely case where we can't convert
the temporary filename from UTF-8 to the native charset, we could just
report the UTF-8 string for the temp file, or something.

  * Allocate each error in a separate pool.  (I'm not worried about the
inefficiency here; errors are uncommon and error chains are short.) 
Change the svn_error_clear() contract to only clear an error and its
descendents, since the pools of ancestors are no longer reachable. 
Provide an svn_error_clear_one() to clear a single error (so that you
can pass its child back to the caller); or just expect code to do:

  newerr = err->child;
  err->child = NULL;
  svn_error_clear(err);

  in that uncommon case.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org