You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by hw...@apache.org on 2011/12/06 22:22:08 UTC
svn commit: r1211162 - /subversion/trunk/subversion/libsvn_wc/context.c
Author: hwright
Date: Tue Dec 6 21:22:08 2011
New Revision: 1211162
URL: http://svn.apache.org/viewvc?rev=1211162&view=rev
Log:
Fix a potential memory cleanup ordering bug.
* subversion/libsvn_wc/context.c
(svn_wc_context_create): Use an improved pool cleanup method, if available.
Modified:
subversion/trunk/subversion/libsvn_wc/context.c
Modified: subversion/trunk/subversion/libsvn_wc/context.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/context.c?rev=1211162&r1=1211161&r2=1211162&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/context.c (original)
+++ subversion/trunk/subversion/libsvn_wc/context.c Tue Dec 6 21:22:08 2011
@@ -28,6 +28,8 @@
#include "svn_dirent_uri.h"
#include "svn_path.h"
+#include "private/svn_dep_compat.h"
+
#include "wc.h"
#include "wc_db.h"
@@ -71,8 +73,21 @@ svn_wc_context_create(svn_wc_context_t *
TRUE, TRUE, ctx->state_pool, scratch_pool));
ctx->close_db_on_destroy = TRUE;
+ /* We should be doing this cleanup prior to the subpools actually getting
+ cleaned up, because chances are that our various data structures and DB
+ handles live in those subpools, which we could be invalidating.
+ Unfortunately, the pre-cleanup handler is only available in APR >= 1.3.0,
+ but most of the platforms we're running on these days probably have that
+ installed anyway.
+
+ This problem manifests itself via segfault when running against an APR
+ compiled with pool lifetime debugging. */
+#if APR_VERSION_AT_LEAST(1, 3, 0)
+ apr_pool_pre_cleanup_register(result_pool, ctx, close_ctx_apr);
+#else
apr_pool_cleanup_register(result_pool, ctx, close_ctx_apr,
apr_pool_cleanup_null);
+#endif
*wc_ctx = ctx;
Re: svn commit: r1211162 - /subversion/trunk/subversion/libsvn_wc/context.c
Posted by Hyrum K Wright <hy...@wandisco.com>.
On Wed, Dec 7, 2011 at 9:30 AM, Philip Martin
<ph...@wandisco.com> wrote:
> Hyrum K Wright <hy...@wandisco.com> writes:
>
>> On Wed, Dec 7, 2011 at 9:03 AM, Philip Martin
>> <ph...@wandisco.com> wrote:
>>> Hyrum K Wright <hy...@wandisco.com> writes:
>>>
>>>> I happy to revert, though I don't know how to fix the problem I'm
>>>> seeing. The symptom is manifest when we go to close the context, we
>>>> iterate over all the wcroot's that the context holds. When running
>>>> against an APR compiled with pool lifetime debugging, this causes NULL
>>>> values to come out of the wcroot hash in the context, which is
>>>> something of an impossibility (setting a NULL value in an APR hash
>>>> erases the entry).
>>>
>>> What operation or test triggers the bug?
>>
>> I was seeing it in any invokation of 'svn commit', but my specific
>> test case was svnlook test 1.
>
> I've reverted r12111162 locally and I cannot reproduce using Subversion
> and APR compiled with -DAPR_POOL_DEBUG=5.
Hmm, well maybe I was seeing ghosts. I've reverted r1211162 in r1211476.
-Hyrum
PS - Are you from the future? ;)
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/
Re: svn commit: r1211162 - /subversion/trunk/subversion/libsvn_wc/context.c
Posted by Philip Martin <ph...@wandisco.com>.
Hyrum K Wright <hy...@wandisco.com> writes:
> On Wed, Dec 7, 2011 at 9:03 AM, Philip Martin
> <ph...@wandisco.com> wrote:
>> Hyrum K Wright <hy...@wandisco.com> writes:
>>
>>> I happy to revert, though I don't know how to fix the problem I'm
>>> seeing. The symptom is manifest when we go to close the context, we
>>> iterate over all the wcroot's that the context holds. When running
>>> against an APR compiled with pool lifetime debugging, this causes NULL
>>> values to come out of the wcroot hash in the context, which is
>>> something of an impossibility (setting a NULL value in an APR hash
>>> erases the entry).
>>
>> What operation or test triggers the bug?
>
> I was seeing it in any invokation of 'svn commit', but my specific
> test case was svnlook test 1.
I've reverted r12111162 locally and I cannot reproduce using Subversion
and APR compiled with -DAPR_POOL_DEBUG=5.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: svn commit: r1211162 - /subversion/trunk/subversion/libsvn_wc/context.c
Posted by Hyrum K Wright <hy...@wandisco.com>.
On Wed, Dec 7, 2011 at 9:03 AM, Philip Martin
<ph...@wandisco.com> wrote:
> Hyrum K Wright <hy...@wandisco.com> writes:
>
>> I happy to revert, though I don't know how to fix the problem I'm
>> seeing. The symptom is manifest when we go to close the context, we
>> iterate over all the wcroot's that the context holds. When running
>> against an APR compiled with pool lifetime debugging, this causes NULL
>> values to come out of the wcroot hash in the context, which is
>> something of an impossibility (setting a NULL value in an APR hash
>> erases the entry).
>
> What operation or test triggers the bug?
I was seeing it in any invokation of 'svn commit', but my specific
test case was svnlook test 1.
-Hyrum
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/
Re: svn commit: r1211162 - /subversion/trunk/subversion/libsvn_wc/context.c
Posted by Philip Martin <ph...@wandisco.com>.
Hyrum K Wright <hy...@wandisco.com> writes:
> I happy to revert, though I don't know how to fix the problem I'm
> seeing. The symptom is manifest when we go to close the context, we
> iterate over all the wcroot's that the context holds. When running
> against an APR compiled with pool lifetime debugging, this causes NULL
> values to come out of the wcroot hash in the context, which is
> something of an impossibility (setting a NULL value in an APR hash
> erases the entry).
What operation or test triggers the bug?
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: svn commit: r1211162 - /subversion/trunk/subversion/libsvn_wc/context.c
Posted by Hyrum K Wright <hy...@wandisco.com>.
On Wed, Dec 7, 2011 at 7:23 AM, Bert Huijben <be...@qqmail.nl> wrote:
>
>
>> -----Original Message-----
>> From: Bert Huijben [mailto:bert@qqmail.nl]
>> Sent: woensdag 7 december 2011 11:42
>> To: dev@subversion.apache.org; hwright@apache.org
>> Subject: RE: svn commit: r1211162 -
>> /subversion/trunk/subversion/libsvn_wc/context.c
>>
>>
>>
>> > -----Original Message-----
>> > From: hwright@apache.org [mailto:hwright@apache.org]
>> > Sent: dinsdag 6 december 2011 22:22
>> > To: commits@subversion.apache.org
>> > Subject: svn commit: r1211162 -
>> > /subversion/trunk/subversion/libsvn_wc/context.c
>> >
>> > Author: hwright
>> > Date: Tue Dec 6 21:22:08 2011
>> > New Revision: 1211162
>> >
>> > URL: http://svn.apache.org/viewvc?rev=1211162&view=rev
>> > Log:
>> > Fix a potential memory cleanup ordering bug.
>>
>> This introduces too many other cleanup problems. This makes the context
>> and db unavailable from the normal cleanup handlers in the same pool, that
>> will run as normal cleanup.
>>
>> The wc_db is carefull to not make assumptions and allocates everything in
>> the same pool and *this* change breaks that by closing the db earlier than
>> those cleanups.
>>
>> -0.5 (open for discussion).
>>
>>
>> If not extremely necessary we should never use a pre-cleanup handler in our
>> code as it just *starts* a race on who can cleanup first and thereby pushes
>> the segfaults in other code
>>
>> There should be no race: everything should be implemented as a proper last-
>> in, first out.
>
> One (of many) examples is how the the update editor uses pools.
> This editor installs (or installed) cleanup handlers to make sure it doesn't forget to run the workingqueue.
>
> This change (or similar changes in other places) will close the wc_db before the subpools had the option to access the database for their cleanup.
>
>
> Please revert this change and fix the real cause of the 'memory cleanup ordering bug' you describe. This change introduces too many other reordering bugs, which will be impossible to fix without introducing things like pre-pre-post-pre cleanup handlers.
>
> The only thing that works understandable for pool cleanup is a proper last-in-first-out. If somebody bends the rules by going out of order, other locations will have to do the same thing. And in the end everybody has to go this way :(
I happy to revert, though I don't know how to fix the problem I'm
seeing. The symptom is manifest when we go to close the context, we
iterate over all the wcroot's that the context holds. When running
against an APR compiled with pool lifetime debugging, this causes NULL
values to come out of the wcroot hash in the context, which is
something of an impossibility (setting a NULL value in an APR hash
erases the entry).
I figured this was due to the pool holding the above hash being
destroy *before* the context cleanup handler was run, and the change
in r1211162 seemed to bear that out. If possible, it'd be nice to get
some independent confirmation, to make sure I'm not imagining things.
If I do revert this change, it pretty much makes pool lifetime
debugging useless, as any operation would fail in this cleanup stage,
rather than where the real issue is. :(
In other words, there is a real bug here which is reproducible and
this change appears to fix. Are there real bugs which can be
demonstrated in response to your concerns above? I'm not discounting
your analysis or concerns, I'm just trying to justify reintroducing a
real issue in favor of fixing a theoretical one.
-Hyrum
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/
RE: svn commit: r1211162 - /subversion/trunk/subversion/libsvn_wc/context.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Bert Huijben [mailto:bert@qqmail.nl]
> Sent: woensdag 7 december 2011 11:42
> To: dev@subversion.apache.org; hwright@apache.org
> Subject: RE: svn commit: r1211162 -
> /subversion/trunk/subversion/libsvn_wc/context.c
>
>
>
> > -----Original Message-----
> > From: hwright@apache.org [mailto:hwright@apache.org]
> > Sent: dinsdag 6 december 2011 22:22
> > To: commits@subversion.apache.org
> > Subject: svn commit: r1211162 -
> > /subversion/trunk/subversion/libsvn_wc/context.c
> >
> > Author: hwright
> > Date: Tue Dec 6 21:22:08 2011
> > New Revision: 1211162
> >
> > URL: http://svn.apache.org/viewvc?rev=1211162&view=rev
> > Log:
> > Fix a potential memory cleanup ordering bug.
>
> This introduces too many other cleanup problems. This makes the context
> and db unavailable from the normal cleanup handlers in the same pool, that
> will run as normal cleanup.
>
> The wc_db is carefull to not make assumptions and allocates everything in
> the same pool and *this* change breaks that by closing the db earlier than
> those cleanups.
>
> -0.5 (open for discussion).
>
>
> If not extremely necessary we should never use a pre-cleanup handler in our
> code as it just *starts* a race on who can cleanup first and thereby pushes
> the segfaults in other code
>
> There should be no race: everything should be implemented as a proper last-
> in, first out.
One (of many) examples is how the the update editor uses pools.
This editor installs (or installed) cleanup handlers to make sure it doesn't forget to run the workingqueue.
This change (or similar changes in other places) will close the wc_db before the subpools had the option to access the database for their cleanup.
Please revert this change and fix the real cause of the 'memory cleanup ordering bug' you describe. This change introduces too many other reordering bugs, which will be impossible to fix without introducing things like pre-pre-post-pre cleanup handlers.
The only thing that works understandable for pool cleanup is a proper last-in-first-out. If somebody bends the rules by going out of order, other locations will have to do the same thing. And in the end everybody has to go this way :(
>
> Bert
RE: svn commit: r1211162 - /subversion/trunk/subversion/libsvn_wc/context.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: hwright@apache.org [mailto:hwright@apache.org]
> Sent: dinsdag 6 december 2011 22:22
> To: commits@subversion.apache.org
> Subject: svn commit: r1211162 -
> /subversion/trunk/subversion/libsvn_wc/context.c
>
> Author: hwright
> Date: Tue Dec 6 21:22:08 2011
> New Revision: 1211162
>
> URL: http://svn.apache.org/viewvc?rev=1211162&view=rev
> Log:
> Fix a potential memory cleanup ordering bug.
This introduces too many other cleanup problems. This makes the context and db unavailable from the normal cleanup handlers in the same pool, that will run as normal cleanup.
The wc_db is carefull to not make assumptions and allocates everything in the same pool and *this* change breaks that by closing the db earlier than those cleanups.
-0.5 (open for discussion).
If not extremely necessary we should never use a pre-cleanup handler in our code as it just *starts* a race on who can cleanup first and thereby pushes the segfaults in other code
There should be no race: everything should be implemented as a proper last-in, first out.
Bert