[PATCH] abort_trail shouldn't destroy its pool

[Yoshiki Hayashi <yo...@xemacs.org>]

I was bitten by this bug when I wrote test code for
svn_fs_abort_txn().  Only obvious part is implemented in my
workspace.  I'll implement mutable node deletion and submit
a patch when I'm sure I understand it correctly.

trail->pool is not a subpool private to a trail.  It should
not be destroyed even txn_body failed.  It can't be a
subpool either, since functions like txn_body_begin_txn
assumes trail->pool's life time is longer than trail itself.

In fs-test.c, global pool is passed to svn_fs_open_txn.
When invalid txn_id is given, txn_body_open_txn returns an
error.  Then, svn_fs__retry_txn find there has been an error
and calls abort_trail().  Since trail->pool is the global
pool, abort_trail() destroys global pool.  If you try to
access it, you'll get core dump.

* trail.c (abort_trail): Do not destroy trail->pool.

Index: trail.c
===================================================================
RCS file: /cvs/subversion/subversion/libsvn_fs/trail.c,v
retrieving revision 1.6
diff -u -r1.6 trail.c
--- trail.c	2001/02/21 22:03:59	1.6
+++ trail.c	2001/02/27 09:30:48
@@ -69,8 +69,6 @@
   SVN_ERR (DB_WRAP (fs, "aborting Berkeley DB transaction",
                     txn_abort (trail->db_txn)));
  
-  apr_pool_destroy (trail->pool);
-
   return SVN_NO_ERROR;
 }
 


-- 
Yoshiki Hayashi

Re: [PATCH] abort_trail shouldn't destroy its pool

[Jim Blandy <ji...@zwingli.cygnus.com>]

Yoshiki Hayashi <yo...@xemacs.org> writes:
> trail->pool is not a subpool private to a trail.  It should
> not be destroyed even txn_body failed.  It can't be a
> subpool either, since functions like txn_body_begin_txn
> assumes trail->pool's life time is longer than trail itself.

Thanks for catching this bug!

In fact, trail->pool is supposed to be a subpool of the pool passed to
svn_fs__retry_txn.  If the transaction aborts, then the subpool is
destroyed, undoing all the allocation that was made in the transaction
attempt.  If the transaction succeeds, the subpool is simply left
alone; since nobody has any references to it any more, and it will be
freed when its parent pool is freed, the objects it contains are
effectively merged into the parent pool --- exactly the effect we want
for a successful transaction.

I've committed a fix.  Let me know if I've missed something.