On Sat, Jan 4, 2014 at 9:47 PM, Bert Huijben <b...@qqmail.nl> wrote: > You only know this for the success path. The fs api should hide these > details.. >
With all due respect, the fault is svn_client_mtcc_commit destroying mtcc->pool before destroying mtcc itself (via its owning pool). Hence, the caller has no control over pool lifetimes and any allocation in sibling pools, e.g. scratch_pool, is potentially invalid. There is no way we can document these guarantees for public APIs and really > assume that all existing and future users follow all these new requirements > for a new Fs layer cache. > > We had similar caches in the past, but hid the ugly details by installing > cleanup handlers... > > We should really look at this well before 1.9, as adding such requirements > is a breaking change. And the commit API already existed since well before > 1.0 without these pool requirements. Strict dual pools are only common > since 1.7, but especially generated bindings (like swig) have strange pool > handling much longer. > I leave it to the author of the mucc code to simply go back to standard pool handling by destroying the mtcc with its parent pool instead of partly destroying it in some API function. -- Stefan^2. ------------------------------ > From: stef...@apache.org > Sent: 4-1-2014 15:05 > To: commits@subversion.apache.org > Subject: svn commit: r1555350 > -/subversion/trunk/subversion/libsvn_client/mtcc.c > > Author: stefan2 > Date: Sat Jan 4 14:04:36 2014 > New Revision: 1555350 > > URL: http://svn.apache.org/r1555350 > Log: > Fix segfault in svnmucc. > > * subversion/libsvn_client/mtcc.c > (svn_client_mtcc_commit): We explicitly destroy the MTCC pool and > have no knowledge about its relation to > SCRATCH_POOL. Thus, we can't use the > for anything non-trivial. > > Modified: > subversion/trunk/subversion/libsvn_client/mtcc.c > > Modified: subversion/trunk/subversion/libsvn_client/mtcc.c > URL: > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/mtcc.c?rev=1555350&r1=1555349&r2=1555350&view=diff > > ============================================================================== > --- subversion/trunk/subversion/libsvn_client/mtcc.c (original) > +++ subversion/trunk/subversion/libsvn_client/mtcc.c Sat Jan 4 14:04:36 > 2014 > @@ -1337,12 +1337,15 @@ svn_client_mtcc_commit(apr_hash_t *revpr > "is not a directory"), > session_url); > > + /* Beware that the editor object must not live longer than the MTCC. > + Otherwise, txn objects etc. in EDITOR may live longer than their > + respective FS objects. So, we can't use SCRATCH_POOL here. */ > SVN_ERR(svn_ra_get_commit_editor3(mtcc->ra_session, &editor, > &edit_baton, > commit_revprops, > commit_callback, commit_baton, > NULL /* lock_tokens */, > FALSE /* keep_locks */, > - scratch_pool)); > + mtcc->pool)); > > err = editor->open_root(edit_baton, mtcc->base_revision, scratch_pool, > &root_baton); > > > >
