You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bert Huijben <be...@qqmail.nl> on 2014/01/04 21:47:43 UTC
RE: svn commit: r1555350
-/subversion/trunk/subversion/libsvn_client/mtcc.c
You only know this for the success path. The fs api should hide these details..
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.
Bert
-----Original Message-----
From: "stefan2@apache.org" <st...@apache.org>
Sent: 4-1-2014 15:05
To: "commits@subversion.apache.org" <co...@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);
Re: svn commit: r1555350 -/subversion/trunk/subversion/libsvn_client/mtcc.c
Posted by Branko Čibej <br...@wandisco.com>.
On 05.01.2014 01:53, Stefan Fuhrmann wrote:
> Hi Bert,
>
> All your arguments would be valid and I would fully agree
> with them, if the FS layer was the root cause of the pool
> issue. However, IMO, that is not the case and r1555297
> merely accidentally caused free memory to be reused
> earlier than before.
>
> My understanding of mucc_commit is the following sequence:
>
> ra = open_session(mtcc->pool)
> editor = open_editor(ra, scratch_pool)
> destroy(mtcc->pool)
> ... // editor cannot be cleaned up safely anymore
> destroy(scratch_pool)
>
> This is clearly invalid (unless I'm completely misreading the code).
I agree that it looks clearly invalid; however, the API documentation is
unclear. It only says that the RA session may not be used during the
lifetime of the editor.
I would expect that "editor->close_edit()" (which you omit from that
sequence and which happens before the "destroy(mtcc->pool)" call) to in
fact kills the editor, and that destroying its containing pool is just a
minor detail. It's obviously a bad idea for the editor implementation to
assume anything about the RA session's pool after close_edit (or
abort_edit).
On the other hand, the mtcc code is decidedly weird when it comes to
pool lifetime handling, which becomes clear when the sequence above is
seen in the context of the surrounding API calls and pool creations:
execute(..., pool):
scratch_pool = create(pool)
mtcc = mtcc_create(pool, scratch_pool):
mtcc.pool = create(pool)
mtcc.ra = open_session(pool, scratch_pool)
mtcc_commit(mtcc, scratch_pool):
editor = open_editor(mtcc.ra, scratch_pool)
editor.close_edit()
destroy(mtcc.pool) ## Oops ...
destroy(scratch_pool)
This makes the lifetime of the editor object (as opposed to the edit
drive) overlap with the lifetime of the session. As I said, there's
nothing in the API docs saying explicitly that this is wrong, but at the
very least it's extremely unusual (and, FWIW, it has nothing to do with
the single- vs. dual-pool pattern).
While it may be true that we use this pattern elsewhere, there's
absolutely no call for repeating it here and perpetuating the broken
pool usage. I'll note that this kind of crud (and in new code, no less)
will make switching to Ev2 even more of a nightmare.
As an aside to all of the above: Why in blazes is svn_client_mtcc_* a
public API? It is nothing but a wrapper for the RA commit editor API. It
doesn't even hide the nasty details of getting file contents streams and
checksums from the API consumer. As such, it's nothing but a maintenance
burden. Yes, it hides the nasty details of Ev1 from the poor user, but
that goal would be much better served by working on making Ev2 public
(as I've already done in JavaHL). This svn_client_mtcc thing adds
absolutely no benefit compared to Ev2, as far as I can see.
(It also breaks our established API pattern by putting the API context
(the svn_client_mtcc_t) last in the parameter list, instead of first,
but given that the whole idea of making this a public API is silly,
that's just a minor nit.)
In other words, -1 to the whole svn_client_mtcc idea for the "technical"
reason that it's a total waste of time.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: svn commit: r1555350 -/subversion/trunk/subversion/libsvn_client/mtcc.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
Hi Bert,
All your arguments would be valid and I would fully agree
with them, if the FS layer was the root cause of the pool
issue. However, IMO, that is not the case and r1555297
merely accidentally caused free memory to be reused
earlier than before.
My understanding of mucc_commit is the following sequence:
ra = open_session(mtcc->pool)
editor = open_editor(ra, scratch_pool)
destroy(mtcc->pool)
... // editor cannot be cleaned up safely anymore
destroy(scratch_pool)
This is clearly invalid (unless I'm completely misreading the code).
I see three reasonable ways to fix this:
* don't destroy mtcc->pool but let it be cleaned up with mtcc,
i.e. the user has full control over its lifetime - as usual
* stick with the destructive commit() code and use a sub-pool
of scratch_pool create the editor and the clean it up *before*
the ra session becomes invalid
* allocate the editor in the same pool as the ra layer (current
code) which would not mean a extra memory consumption
in the "o.k." case.
-- Stefan^2.
On Sun, Jan 5, 2014 at 12:07 AM, Bert Huijben <be...@qqmail.nl> wrote:
> In the case of svnmucc the passed scratch pool is a subpool of the pool
> that the Ra session id created it (where the mtcc is created in). This is a
> perfectly valid case of using pools, just like the common iterpool pattern.
>
> If this shows a bug in the caching of the fs layer we should fix that bug
> instead of requiring the rest of our code to just use a single pool for
> everything. So your fs changes +- requires users to stop using iterpool and
> scratch pool everywhere.
>
> The fs layer should manage its own memory and not require API users to
> manage it for this library.
>
> The cache pattern of allocating pointers in an unrelated pool, pointing to
> data sorted centrally with different lifetime is broken. To make this work
> you need cleanup handlers both ways… or a separate allocation strategy
> (refcounts, global allocator?)
>
> (I'm not able to look at the svnmucc code in detail now, so there might be
> another bug that you fixed… but by reading your log message and change this
> looks like covering up for a design problem)
>
> We can't fix all code using our APIs until 2.0. Until then our existing
> API surface should cover for requirements changes.
>
> Bert
>
> Sent from Windows Mail
>
> *From:* Stefan Fuhrmann <st...@wandisco.com>
> *Sent:* Saturday, January 4, 2014 10:26 PM
> *To:* Bert Huijben <be...@qqmail.nl>
> *Cc:* Subversion Development <de...@subversion.apache.org>,
> commits@subversion.apache.org
>
> On Sat, Jan 4, 2014 at 9:47 PM, Bert Huijben <be...@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: stefan2@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);
>>
>>
>>
>>
>
Re: svn commit: r1555350 -/subversion/trunk/subversion/libsvn_client/mtcc.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
Hi Bert,
All your arguments would be valid and I would fully agree
with them, if the FS layer was the root cause of the pool
issue. However, IMO, that is not the case and r1555297
merely accidentally caused free memory to be reused
earlier than before.
My understanding of mucc_commit is the following sequence:
ra = open_session(mtcc->pool)
editor = open_editor(ra, scratch_pool)
destroy(mtcc->pool)
... // editor cannot be cleaned up safely anymore
destroy(scratch_pool)
This is clearly invalid (unless I'm completely misreading the code).
I see three reasonable ways to fix this:
* don't destroy mtcc->pool but let it be cleaned up with mtcc,
i.e. the user has full control over its lifetime - as usual
* stick with the destructive commit() code and use a sub-pool
of scratch_pool create the editor and the clean it up *before*
the ra session becomes invalid
* allocate the editor in the same pool as the ra layer (current
code) which would not mean a extra memory consumption
in the "o.k." case.
-- Stefan^2.
On Sun, Jan 5, 2014 at 12:07 AM, Bert Huijben <be...@qqmail.nl> wrote:
> In the case of svnmucc the passed scratch pool is a subpool of the pool
> that the Ra session id created it (where the mtcc is created in). This is a
> perfectly valid case of using pools, just like the common iterpool pattern.
>
> If this shows a bug in the caching of the fs layer we should fix that bug
> instead of requiring the rest of our code to just use a single pool for
> everything. So your fs changes +- requires users to stop using iterpool and
> scratch pool everywhere.
>
> The fs layer should manage its own memory and not require API users to
> manage it for this library.
>
> The cache pattern of allocating pointers in an unrelated pool, pointing to
> data sorted centrally with different lifetime is broken. To make this work
> you need cleanup handlers both ways… or a separate allocation strategy
> (refcounts, global allocator?)
>
> (I'm not able to look at the svnmucc code in detail now, so there might be
> another bug that you fixed… but by reading your log message and change this
> looks like covering up for a design problem)
>
> We can't fix all code using our APIs until 2.0. Until then our existing
> API surface should cover for requirements changes.
>
> Bert
>
> Sent from Windows Mail
>
> *From:* Stefan Fuhrmann <st...@wandisco.com>
> *Sent:* Saturday, January 4, 2014 10:26 PM
> *To:* Bert Huijben <be...@qqmail.nl>
> *Cc:* Subversion Development <de...@subversion.apache.org>,
> commits@subversion.apache.org
>
> On Sat, Jan 4, 2014 at 9:47 PM, Bert Huijben <be...@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: stefan2@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);
>>
>>
>>
>>
>
Re: svn commit: r1555350 -/subversion/trunk/subversion/libsvn_client/mtcc.c
Posted by Bert Huijben <be...@qqmail.nl>.
In the case of svnmucc the passed scratch pool is a subpool of the pool that the Ra session id created it (where the mtcc is created in). This is a perfectly valid case of using pools, just like the common iterpool pattern.
If this shows a bug in the caching of the fs layer we should fix that bug instead of requiring the rest of our code to just use a single pool for everything. So your fs changes +- requires users to stop using iterpool and scratch pool everywhere.
The fs layer should manage its own memory and not require API users to manage it for this library.
The cache pattern of allocating pointers in an unrelated pool, pointing to data sorted centrally with different lifetime is broken. To make this work you need cleanup handlers both ways… or a separate allocation strategy (refcounts, global allocator?)
(I'm not able to look at the svnmucc code in detail now, so there might be another bug that you fixed… but by reading your log message and change this looks like covering up for a design problem)
We can't fix all code using our APIs until 2.0. Until then our existing API surface should cover for requirements changes.
Bert
Sent from Windows Mail
From: Stefan Fuhrmann
Sent: Saturday, January 4, 2014 10:26 PM
To: Bert Huijben
Cc: Subversion Development, commits@subversion.apache.org
On Sat, Jan 4, 2014 at 9:47 PM, Bert Huijben <be...@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: stefan2@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);
Re: svn commit: r1555350 -/subversion/trunk/subversion/libsvn_client/mtcc.c
Posted by Bert Huijben <be...@qqmail.nl>.
In the case of svnmucc the passed scratch pool is a subpool of the pool that the Ra session id created it (where the mtcc is created in). This is a perfectly valid case of using pools, just like the common iterpool pattern.
If this shows a bug in the caching of the fs layer we should fix that bug instead of requiring the rest of our code to just use a single pool for everything. So your fs changes +- requires users to stop using iterpool and scratch pool everywhere.
The fs layer should manage its own memory and not require API users to manage it for this library.
The cache pattern of allocating pointers in an unrelated pool, pointing to data sorted centrally with different lifetime is broken. To make this work you need cleanup handlers both ways… or a separate allocation strategy (refcounts, global allocator?)
(I'm not able to look at the svnmucc code in detail now, so there might be another bug that you fixed… but by reading your log message and change this looks like covering up for a design problem)
We can't fix all code using our APIs until 2.0. Until then our existing API surface should cover for requirements changes.
Bert
Sent from Windows Mail
From: Stefan Fuhrmann
Sent: Saturday, January 4, 2014 10:26 PM
To: Bert Huijben
Cc: Subversion Development, commits@subversion.apache.org
On Sat, Jan 4, 2014 at 9:47 PM, Bert Huijben <be...@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: stefan2@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);
Re: svn commit: r1555350 -/subversion/trunk/subversion/libsvn_client/mtcc.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sat, Jan 4, 2014 at 9:47 PM, Bert Huijben <be...@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: stefan2@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);
>
>
>
>
Re: svn commit: r1555350 -/subversion/trunk/subversion/libsvn_client/mtcc.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sat, Jan 4, 2014 at 9:47 PM, Bert Huijben <be...@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: stefan2@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);
>
>
>
>