You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Fuhrmann <st...@wandisco.com> on 2012/11/17 14:01:57 UTC
Serf crashes on fork [was: r1380608 breaks svnsync]
On Fri, Nov 16, 2012 at 5:24 PM, Stefan Sperling wrote:
> On Fri, Nov 16, 2012 at 03:35:12PM +0100, Stefan Fuhrmann wrote:
> > Hi Stefan,
> >
> > For some reason, this commit causes the forked process
> > executing the pre-revprop-change script to segfault under
> > certain circumstances.
>
> I don't have time to look into this right now, unfortunately :(
> Can you please file an issue with a 1.8.0 milestone? I'll get
> to it later.
>
As it turns out, your commit has only be the trigger but
not the root cause.
serf_trunk/allocator.c, serf_bucket_allocator_create(), line 147:
/* ### this implies buckets cannot cross a fork/exec. desirable?
*
* ### hmm. it probably also means that buckets cannot be AROUND
* ### during a fork/exec. the new process will try to clean them
* ### up and figure out there are unfreed blocks...
*/
apr_pool_cleanup_register(pool, allocator,
allocator_cleanup, allocator_cleanup);
Since we fork() for hooks, we can't use hooks in ra_local
while there is an open serf connection. Otherwise, we get
into trouble with pool cleanups:
==7392== Invalid read of size 8
==7392== at 0x5055694: clean_resp (outgoing.c:51)
==7392== by 0x50C65D: cleanup_pool_for_exec (apr_pools.c:2359)
==7392== by 0x50C677: cleanup_pool_for_exec (apr_pools.c:2369)
==7392== by 0x50C677: cleanup_pool_for_exec (apr_pools.c:2369)
==7392== by 0x50C677: cleanup_pool_for_exec (apr_pools.c:2369)
==7392== by 0x50C677: cleanup_pool_for_exec (apr_pools.c:2369)
==7392== by 0x5142F6: apr_proc_create (proc.c:430)
==7392== by 0x4D24F4: svn_io_start_cmd3 (io.c:2686)
==7392== by 0x42308D: run_hook_cmd (hooks.c:249)
==7392== by 0x423BC2: svn_repos__hooks_pre_revprop_change (hooks.c:565)
==7392== by 0x421A82: svn_repos_fs_change_rev_prop4 (fs-wrap.c:351)
==7392== by 0x41B56D: svn_ra_local__change_rev_prop (ra_plugin.c:674)
==7392== Address 0x72457c8 is 13,448 bytes inside a block of size 106,496
free'd
==7392== at 0x4C2A739: free (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7392== by 0x50D002: apr_allocator_free (apr_pools.c:425)
==7392== by 0x5057B45: allocator_cleanup (allocator.c:108)
==7392== by 0x50C65D: cleanup_pool_for_exec (apr_pools.c:2359)
==7392== by 0x50C677: cleanup_pool_for_exec (apr_pools.c:2369)
==7392== by 0x50C677: cleanup_pool_for_exec (apr_pools.c:2369)
==7392== by 0x50C677: cleanup_pool_for_exec (apr_pools.c:2369)
==7392== by 0x5142F6: apr_proc_create (proc.c:430)
==7392== by 0x4D24F4: svn_io_start_cmd3 (io.c:2686)
==7392== by 0x42308D: run_hook_cmd (hooks.c:249)
==7392== by 0x423BC2: svn_repos__hooks_pre_revprop_change (hooks.c:565)
==7392== by 0x421A82: svn_repos_fs_change_rev_prop4 (fs-wrap.c:351)
So, no svnsync from http[s]:// to file:// anymore :(
Any further operations where that might bite us?
-- Stefan^2.
--
Certified & Supported Apache Subversion Downloads:
*
http://www.wandisco.com/subversion/download
*
Re: Serf crashes on fork
Posted by Greg Stein <gs...@gmail.com>.
On Mon, Dec 3, 2012 at 7:30 AM, Stefan Fuhrmann
<st...@wandisco.com> wrote:
> On Thu, Nov 29, 2012 at 5:51 PM, C. Michael Pilato <cm...@collab.net>
> wrote:
>>
>> Perhaps this should be discussed on the serf dev list? Or an issue filed
>> in
>> their tracker?
>
> Since I apprently can't post on serf-dev,
Sure you can. Subscribe first :-)
> I opened an issue for 1.8:
> http://subversion.tigris.org/issues/show_bug.cgi?id=4266
Marked as fixed, along with serf's corresponding issue 93.
it is fixed on serf trunk/ in r1714, and on branches/1.2.x (the
upcoming release) in r1716.
I imagine it should work just fine, but would be happy to hear any
testing reports.
Cheers,
-g
Re: Serf crashes on fork
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Thu, Nov 29, 2012 at 5:51 PM, C. Michael Pilato <cm...@collab.net>wrote:
> Perhaps this should be discussed on the serf dev list? Or an issue filed
> in
> their tracker?
>
Since I apprently can't post on serf-dev, I opened an issue for 1.8:
http://subversion.tigris.org/issues/show_bug.cgi?id=4266
-- Stefan^2.
--
Certified & Supported Apache Subversion Downloads:
*
http://www.wandisco.com/subversion/download
*
Re: Serf crashes on fork
Posted by "C. Michael Pilato" <cm...@collab.net>.
Perhaps this should be discussed on the serf dev list? Or an issue filed in
their tracker?
On 11/19/2012 10:09 AM, Stefan Fuhrmann wrote:
> On Mon, Nov 19, 2012 at 2:44 PM, Philip Martin <philip.martin@wandisco.com
> <ma...@wandisco.com>> wrote:
>
> Philip Martin <philip.martin@wandisco.com
> <ma...@wandisco.com>> writes:
>
> > Stefan Fuhrmann <stefan.fuhrmann@wandisco.com
> <ma...@wandisco.com>> writes:
> >
> >> On Mon, Nov 19, 2012 at 12:24 PM, Philip Martin
> >> <philip.martin@wandisco.com <ma...@wandisco.com>>wrote:
> >>
> >>> Stefan Fuhrmann <stefan.fuhrmann@wandisco.com
> <ma...@wandisco.com>> writes:
> >>>
> >>> > As it turns out, your commit has only be the trigger but
> >>> > not the root cause.
> >>> >
> >>> > serf_trunk/allocator.c, serf_bucket_allocator_create(), line 147:
> >>> >
> >>> > /* ### this implies buckets cannot cross a fork/exec. desirable?
> >>> > *
> >>> > * ### hmm. it probably also means that buckets cannot be AROUND
> >>> > * ### during a fork/exec. the new process will try to clean them
> >>> > * ### up and figure out there are unfreed blocks...
> >>> > */
> >>> > apr_pool_cleanup_register(pool, allocator,
> >>> > allocator_cleanup, allocator_cleanup);
> >>> >
> >>> > Since we fork() for hooks, we can't use hooks in ra_local
> >>> > while there is an open serf connection. Otherwise, we get
> >>> > into trouble with pool cleanups:
> >>>
> >>> Does it ever make sense for the child process to run that handler? Is
> >>> that to allow a parent process to allocate a serf connection and then
> >>> fork off a child process to use the connection?
>
> If the processes were behaving like that the child cleanup handlers
> would not be involved.
>
> >>
> >> From the comments in APR/threadproc/unix/proc.c,
> >> it seems that apr_proc_create runs *all* pool cleanups
> >> in the child process to clean up duplicated file handles
> >> and such.
> >
> > apr_proc_create runs the child cleanup handlers. Note that two handlers
> > are passed to _register, one for the parent one for the child. I'm
> > asking why serf installs a child handler rather than passing
> > apr_pool_cleanup_null.
>
> The cleanup handler is just releasing memory via apr_allocator_free. I
> see no reason for it to be installed as a child cleanup handler.
>
>
> Simply patching serf fixes the problem for me.
>
> -- Stefan^2.
>
> [[[
> Index: buckets/allocator.c
> ===================================================================
> --- buckets/allocator.c (revision 1685)
> +++ buckets/allocator.c (working copy)
> @@ -151,7 +151,7 @@
> * ### up and figure out there are unfreed blocks...
> */
> apr_pool_cleanup_register(pool, allocator,
> - allocator_cleanup, allocator_cleanup);
> + allocator_cleanup, apr_pool_cleanup_null);
>
> return allocator;
> }
> ]]]
>
> --
> Certified & Supported Apache Subversion Downloads:
> /
>
> http://www.wandisco.com/subversion/download
>
> /
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: Serf crashes on fork
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Mon, Nov 19, 2012 at 2:44 PM, Philip Martin
<ph...@wandisco.com>wrote:
> Philip Martin <ph...@wandisco.com> writes:
>
> > Stefan Fuhrmann <st...@wandisco.com> writes:
> >
> >> On Mon, Nov 19, 2012 at 12:24 PM, Philip Martin
> >> <ph...@wandisco.com>wrote:
> >>
> >>> Stefan Fuhrmann <st...@wandisco.com> writes:
> >>>
> >>> > As it turns out, your commit has only be the trigger but
> >>> > not the root cause.
> >>> >
> >>> > serf_trunk/allocator.c, serf_bucket_allocator_create(), line 147:
> >>> >
> >>> > /* ### this implies buckets cannot cross a fork/exec. desirable?
> >>> > *
> >>> > * ### hmm. it probably also means that buckets cannot be AROUND
> >>> > * ### during a fork/exec. the new process will try to clean them
> >>> > * ### up and figure out there are unfreed blocks...
> >>> > */
> >>> > apr_pool_cleanup_register(pool, allocator,
> >>> > allocator_cleanup, allocator_cleanup);
> >>> >
> >>> > Since we fork() for hooks, we can't use hooks in ra_local
> >>> > while there is an open serf connection. Otherwise, we get
> >>> > into trouble with pool cleanups:
> >>>
> >>> Does it ever make sense for the child process to run that handler? Is
> >>> that to allow a parent process to allocate a serf connection and then
> >>> fork off a child process to use the connection?
>
> If the processes were behaving like that the child cleanup handlers
> would not be involved.
>
> >>
> >> From the comments in APR/threadproc/unix/proc.c,
> >> it seems that apr_proc_create runs *all* pool cleanups
> >> in the child process to clean up duplicated file handles
> >> and such.
> >
> > apr_proc_create runs the child cleanup handlers. Note that two handlers
> > are passed to _register, one for the parent one for the child. I'm
> > asking why serf installs a child handler rather than passing
> > apr_pool_cleanup_null.
>
> The cleanup handler is just releasing memory via apr_allocator_free. I
> see no reason for it to be installed as a child cleanup handler.
>
Simply patching serf fixes the problem for me.
-- Stefan^2.
[[[
Index: buckets/allocator.c
===================================================================
--- buckets/allocator.c (revision 1685)
+++ buckets/allocator.c (working copy)
@@ -151,7 +151,7 @@
* ### up and figure out there are unfreed blocks...
*/
apr_pool_cleanup_register(pool, allocator,
- allocator_cleanup, allocator_cleanup);
+ allocator_cleanup, apr_pool_cleanup_null);
return allocator;
}
]]]
--
Certified & Supported Apache Subversion Downloads:
*
http://www.wandisco.com/subversion/download
*
Re: Serf crashes on fork
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> Stefan Fuhrmann <st...@wandisco.com> writes:
>
>> On Mon, Nov 19, 2012 at 12:24 PM, Philip Martin
>> <ph...@wandisco.com>wrote:
>>
>>> Stefan Fuhrmann <st...@wandisco.com> writes:
>>>
>>> > As it turns out, your commit has only be the trigger but
>>> > not the root cause.
>>> >
>>> > serf_trunk/allocator.c, serf_bucket_allocator_create(), line 147:
>>> >
>>> > /* ### this implies buckets cannot cross a fork/exec. desirable?
>>> > *
>>> > * ### hmm. it probably also means that buckets cannot be AROUND
>>> > * ### during a fork/exec. the new process will try to clean them
>>> > * ### up and figure out there are unfreed blocks...
>>> > */
>>> > apr_pool_cleanup_register(pool, allocator,
>>> > allocator_cleanup, allocator_cleanup);
>>> >
>>> > Since we fork() for hooks, we can't use hooks in ra_local
>>> > while there is an open serf connection. Otherwise, we get
>>> > into trouble with pool cleanups:
>>>
>>> Does it ever make sense for the child process to run that handler? Is
>>> that to allow a parent process to allocate a serf connection and then
>>> fork off a child process to use the connection?
If the processes were behaving like that the child cleanup handlers
would not be involved.
>>
>> From the comments in APR/threadproc/unix/proc.c,
>> it seems that apr_proc_create runs *all* pool cleanups
>> in the child process to clean up duplicated file handles
>> and such.
>
> apr_proc_create runs the child cleanup handlers. Note that two handlers
> are passed to _register, one for the parent one for the child. I'm
> asking why serf installs a child handler rather than passing
> apr_pool_cleanup_null.
The cleanup handler is just releasing memory via apr_allocator_free. I
see no reason for it to be installed as a child cleanup handler.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: Serf crashes on fork
Posted by Philip Martin <ph...@wandisco.com>.
Stefan Fuhrmann <st...@wandisco.com> writes:
> On Mon, Nov 19, 2012 at 12:24 PM, Philip Martin
> <ph...@wandisco.com>wrote:
>
>> Stefan Fuhrmann <st...@wandisco.com> writes:
>>
>> > As it turns out, your commit has only be the trigger but
>> > not the root cause.
>> >
>> > serf_trunk/allocator.c, serf_bucket_allocator_create(), line 147:
>> >
>> > /* ### this implies buckets cannot cross a fork/exec. desirable?
>> > *
>> > * ### hmm. it probably also means that buckets cannot be AROUND
>> > * ### during a fork/exec. the new process will try to clean them
>> > * ### up and figure out there are unfreed blocks...
>> > */
>> > apr_pool_cleanup_register(pool, allocator,
>> > allocator_cleanup, allocator_cleanup);
>> >
>> > Since we fork() for hooks, we can't use hooks in ra_local
>> > while there is an open serf connection. Otherwise, we get
>> > into trouble with pool cleanups:
>>
>> Does it ever make sense for the child process to run that handler? Is
>> that to allow a parent process to allocate a serf connection and then
>> fork off a child process to use the connection?
>>
>
> From the comments in APR/threadproc/unix/proc.c,
> it seems that apr_proc_create runs *all* pool cleanups
> in the child process to clean up duplicated file handles
> and such.
apr_proc_create runs the child cleanup handlers. Note that two handlers
are passed to _register, one for the parent one for the child. I'm
asking why serf installs a child handler rather than passing
apr_pool_cleanup_null.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: Serf crashes on fork
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Mon, Nov 19, 2012 at 12:24 PM, Philip Martin
<ph...@wandisco.com>wrote:
> Stefan Fuhrmann <st...@wandisco.com> writes:
>
> > As it turns out, your commit has only be the trigger but
> > not the root cause.
> >
> > serf_trunk/allocator.c, serf_bucket_allocator_create(), line 147:
> >
> > /* ### this implies buckets cannot cross a fork/exec. desirable?
> > *
> > * ### hmm. it probably also means that buckets cannot be AROUND
> > * ### during a fork/exec. the new process will try to clean them
> > * ### up and figure out there are unfreed blocks...
> > */
> > apr_pool_cleanup_register(pool, allocator,
> > allocator_cleanup, allocator_cleanup);
> >
> > Since we fork() for hooks, we can't use hooks in ra_local
> > while there is an open serf connection. Otherwise, we get
> > into trouble with pool cleanups:
>
> Does it ever make sense for the child process to run that handler? Is
> that to allow a parent process to allocate a serf connection and then
> fork off a child process to use the connection?
>
>From the comments in APR/threadproc/unix/proc.c,
it seems that apr_proc_create runs *all* pool cleanups
in the child process to clean up duplicated file handles
and such.
-- Stefan^2.
--
Certified & Supported Apache Subversion Downloads:
*
http://www.wandisco.com/subversion/download
*
Re: Serf crashes on fork
Posted by Philip Martin <ph...@wandisco.com>.
Stefan Fuhrmann <st...@wandisco.com> writes:
> As it turns out, your commit has only be the trigger but
> not the root cause.
>
> serf_trunk/allocator.c, serf_bucket_allocator_create(), line 147:
>
> /* ### this implies buckets cannot cross a fork/exec. desirable?
> *
> * ### hmm. it probably also means that buckets cannot be AROUND
> * ### during a fork/exec. the new process will try to clean them
> * ### up and figure out there are unfreed blocks...
> */
> apr_pool_cleanup_register(pool, allocator,
> allocator_cleanup, allocator_cleanup);
>
> Since we fork() for hooks, we can't use hooks in ra_local
> while there is an open serf connection. Otherwise, we get
> into trouble with pool cleanups:
Does it ever make sense for the child process to run that handler? Is
that to allow a parent process to allocate a serf connection and then
fork off a child process to use the connection?
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download