You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by "William A. Rowe, Jr." <wr...@rowe-clan.net> on 2007/04/27 17:46:54 UTC
Time to revisit 1.2.9 flavors
On Sunday I will be tagging and rolling, once I'm settled in Amsterdam.
Consider this a last call if you have some free cycles to hack on Saturday.
The indians at httpd are restless, and would like a bugfixed release to
roll on Wednesday. Just trying to leave enough time for a thorough release
vote between the two tags.
Re: Time to revisit 1.2.9 flavors
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
We can't simply document it, this is definately a significant bug.
Reviewing your patch - ty!
Bill
Bojan Smojver wrote:
> On Sat, 2007-04-28 at 09:02 +1000, Bojan Smojver wrote:
>
>> I guess 41119 will be left the way it is? Should we just document and
>> close?
>
> Here is that hackish patch (also attached to the bug report).
>
> What's the final verdict on this behaviour? Document and close? Or
> patch?
>
>
>
> ------------------------------------------------------------------------
>
> Index: memory/unix/apr_pools.c
> ===================================================================
> --- memory/unix/apr_pools.c (revision 537103)
> +++ memory/unix/apr_pools.c (working copy)
> @@ -2100,6 +2100,13 @@
> cleanup_pool_for_exec(p);
> }
>
> +static int cleanup_for_exec = 0;
> +
> +APR_DECLARE(int) apr_cleanup_is_for_exec()
> +{
> + return cleanup_for_exec;
> +}
> +
> APR_DECLARE(void) apr_pool_cleanup_for_exec(void)
> {
> #if !defined(WIN32) && !defined(OS2)
> @@ -2112,7 +2119,9 @@
> * I can do about that (except if the child decides
> * to go out and close them
> */
> + cleanup_for_exec = 1;
> cleanup_pool_for_exec(global_pool);
> + cleanup_for_exec = 0;
> #endif /* !defined(WIN32) && !defined(OS2) */
> }
>
> Index: include/arch/apr_private_common.h
> ===================================================================
> --- include/arch/apr_private_common.h (revision 537103)
> +++ include/arch/apr_private_common.h (working copy)
> @@ -39,4 +39,7 @@
> #define APR_UINT32_TRUNC_CAST apr_uint32_t
> #define APR_UINT32_MAX 0xFFFFFFFFUL
>
> +/* cleanup_for_exec */
> +int apr_cleanup_is_for_exec();
> +
> #endif /*APR_PRIVATE_COMMON_H*/
> Index: file_io/win32/readwrite.c
> ===================================================================
> --- file_io/win32/readwrite.c (revision 537103)
> +++ file_io/win32/readwrite.c (working copy)
> @@ -467,6 +467,10 @@
>
> APR_DECLARE(apr_status_t) apr_file_flush(apr_file_t *thefile)
> {
> + if (apr_cleanup_is_for_exec()) {
> + return APR_SUCCESS;
> + }
> +
> if (thefile->buffered) {
> DWORD numbytes, written = 0;
> apr_status_t rc = 0;
> Index: file_io/os2/readwrite.c
> ===================================================================
> --- file_io/os2/readwrite.c (revision 537103)
> +++ file_io/os2/readwrite.c (working copy)
> @@ -283,6 +283,10 @@
>
> APR_DECLARE(apr_status_t) apr_file_flush(apr_file_t *thefile)
> {
> + if (apr_cleanup_is_for_exec()) {
> + return APR_SUCCESS;
> + }
> +
> if (thefile->buffered) {
> ULONG written = 0;
> int rc = 0;
> Index: file_io/unix/readwrite.c
> ===================================================================
> --- file_io/unix/readwrite.c (revision 537103)
> +++ file_io/unix/readwrite.c (working copy)
> @@ -319,6 +319,10 @@
>
> APR_DECLARE(apr_status_t) apr_file_flush(apr_file_t *thefile)
> {
> + if (apr_cleanup_is_for_exec()) {
> + return APR_SUCCESS;
> + }
> +
> if (thefile->buffered) {
> if (thefile->direction == 1 && thefile->bufpos) {
> apr_ssize_t written;
Re: Time to revisit 1.2.9 flavors
Posted by Bojan Smojver <bo...@rexursive.com>.
On Sat, 2007-05-12 at 11:01 +1000, Bojan Smojver wrote:
> I think the current behaviour is a bug and we should change it. From my
> POV, it doesn't make sense to expect that a piece of data that's written
> to a file would end up in it twice in buffered mode and only once in
> non-buffered mode
Also a related issue here - Win32 and OS2 will behave differently at
present, given that they don't fork. So, we're already inconsistent
across platforms. Just another reason to fix this.
--
Bojan
Re: Time to revisit 1.2.9 flavors
Posted by Bojan Smojver <bo...@rexursive.com>.
On Fri, 2007-05-11 at 12:20 +0300, Lucian Adrian Grijincu wrote:
> because this breaks compatibility with former versions of APR (someone
> may actually be counting on buffer being flushed before exec) I'd
> suggest adding
> APR_DECLARE(void) apr_pool_cleanup_for_exec_ex(apr_int_32 flags)
>
> Which will have a flag to enable one of these behaviours.
> + cleanup_for_exec = flags & APR_CLEANUP_FOR_EXEC_FLUSHES_BUFFERS;
> cleanup_pool_for_exec(global_pool);
> + cleanup_for_exec = 0;
I think the current behaviour is a bug and we should change it. From my
POV, it doesn't make sense to expect that a piece of data that's written
to a file would end up in it twice in buffered mode and only once in
non-buffered mode
> nitpicking:
>
> maybe add an APR_INLINE here:
> +APR_DECLARE(int) APR_INLINE apr_cleanup_is_for_exec()
> +{
> + return cleanup_for_exec;
> +}
>
>
> and a closed parens to the comment.
> @@ -2112,7 +2119,9 @@
> * I can do about that (except if the child decides
> * to go out and close them )
> */
Thanks, shall fix.
--
Bojan
Re: Time to revisit 1.2.9 flavors
Posted by Bojan Smojver <bo...@rexursive.com>.
On Fri, 2007-05-11 at 12:20 +0300, Lucian Adrian Grijincu wrote:
> maybe add an APR_INLINE here:
> +APR_DECLARE(int) APR_INLINE apr_cleanup_is_for_exec()
> +{
> + return cleanup_for_exec;
> +}
Can we actually do this, given that variable cleanup_for_exec is only
visible in apr_pools.c?
I mean, if this function was static and used in apr_pools.c, we could do
it. Or, if we moved it to a header file, we could do it too, except that
we would not see cleanup_for_exec there.
Does that make sense?
--
Bojan
Re: Time to revisit 1.2.9 flavors
Posted by Lucian Adrian Grijincu <lu...@gmail.com>.
because this breaks compatibility with former versions of APR (someone
may actually be counting on buffer being flushed before exec) I'd
suggest adding
APR_DECLARE(void) apr_pool_cleanup_for_exec_ex(apr_int_32 flags)
Which will have a flag to enable one of these behaviours.
+ cleanup_for_exec = flags & APR_CLEANUP_FOR_EXEC_FLUSHES_BUFFERS;
cleanup_pool_for_exec(global_pool);
+ cleanup_for_exec = 0;
nitpicking:
maybe add an APR_INLINE here:
+APR_DECLARE(int) APR_INLINE apr_cleanup_is_for_exec()
+{
+ return cleanup_for_exec;
+}
and a closed parens to the comment.
@@ -2112,7 +2119,9 @@
* I can do about that (except if the child decides
* to go out and close them )
*/
On 5/11/07, Bojan Smojver <bo...@rexursive.com> wrote:
> On Sat, 2007-04-28 at 09:02 +1000, Bojan Smojver wrote:
>
> > I guess 41119 will be left the way it is? Should we just document and
> > close?
>
> Here is that hackish patch (also attached to the bug report).
>
> What's the final verdict on this behaviour? Document and close? Or
> patch?
>
> --
> Bojan
>
>
Re: Time to revisit 1.2.9 flavors
Posted by Lucian Adrian Grijincu <lu...@gmail.com>.
A few instructions after this function call APR will call exec.
That will obliterate the running program.
Depending on some other threads to do critical work while another
thread is trying it's best to kill the app is basically asking for
trouble (that is, if you don't setup some synchronization policies
between the threads).
static int cleanup_for_exec = 0;
It does seem to involve some sync issues.
At the beginning we have Ta Tb and cleanup_for_exec=0
fork() is called and we return in the child process to execute
apr_pool_cleanup_for_exec(), which is the only place where
cleanup_for_exec is modified.
The original threads don't see any modifications upon cleanup_for_exec.
Ta' and Tb' start running in the child process.
Ta' sets cleanup_for_exec = 1
Ta' is preempted (waits on a mutex, or whatever) and Tb' runs.
to have a race condition, Tb' must call apr_pool_cleanup_for_exec,
therefore Tb' does a fork().
The first level children (Ta' Tb') continue their run. Tb' will never
touch cleanup_for_exec, so at this level there's no race.
The second level children run.
Ta'' and Tb''
At this stage Tb'' sets cleanup_for_exec = 1, does it's cleanup stuff.
We have two cases:
a) Ta'' gets a chance to run and maybe corrupt data because of a race
on cleanup_for_exec.
b) Ta'' doesn't get the chance to run because Tb'' calls execve.
in the second case there's no race. so no problem.
In the first case, if Ta'' runs. it does indeed have the chance to
corrupt cleanup_for_exec:
Ta'' sets cleanup_for_exec = 1 again, it calles all the registered
functions, and sets cleanup_for_exec = 0.
Now either
a1) Ta'' reaches the original execve and we don't have a corruption
because Tb'' doesn't run, or
a2) Ta'' preempts and Tb'' continues running the clean up it formerly began.
Indeed we now have a race condition.
But think of the implications. Who'd write such ugly twisted code? And
if he did (not to test APR) but for production code, I hope I'm not
gonna run his binary :)
To sum things up:
After the first fork() and before the execve, while running the
cleanup routines, another thread has to run and do a second fork() and
before the execve, it must preempt in a very particular spot.
Considering the alternatives proposed in the bug-report and the odd
prerequisites that must be in place to see a bug, I'd say this is a
viable solution.
I, of course, can be wrong, so please correct me if thats the case :)
--
Lucian Adrian Grijincu
On 5/11/07, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> Bojan Smojver wrote:
> > On Sat, 2007-04-28 at 09:02 +1000, Bojan Smojver wrote:
> >
> >> I guess 41119 will be left the way it is? Should we just document and
> >> close?
>
> > +static int cleanup_for_exec = 0;
>
> Trouble with the submitted patch is that it isn't thread safe, I see all
> sorts of subtle races in this code :(
>
> But maybe I'm mis-reading things - does this only get toggled within the
> child process after fork()?
>
Re: Time to revisit 1.2.9 flavors
Posted by Bojan Smojver <bo...@rexursive.com>.
On Sat, 2007-05-12 at 15:35 +1000, Bojan Smojver wrote:
> Actually, Davi posted a better patch in the bug report,
Here is the most up to date patch. It should deal with all instances of
apr_unix_file_cleanup(). Netware folks, please make sure I didn't do
anything overly stupid in it :-)
http://issues.apache.org/bugzilla/attachment.cgi?id=20188
--
Bojan
Re: Time to revisit 1.2.9 flavors
Posted by Bojan Smojver <bo...@rexursive.com>.
On Sat, 2007-05-12 at 14:30 +1000, Bojan Smojver wrote:
> I'm attaching a patch that features a mutex to make things safer.
> Hopefully a bit better. Also, I dropped OS2 and Win32 stuff - no need.
Actually, Davi posted a better patch in the bug report, along the lines
of Joe's comment. I just added implementation of _unset by hand to it to
avoid macros resetting both cleanups to the same thing.
http://issues.apache.org/bugzilla/show_bug.cgi?id=41119
http://issues.apache.org/bugzilla/attachment.cgi?id=20186
It is completely local to open.c and has no threading issues, so I think
we should go with that.
--
Bojan
Re: Time to revisit 1.2.9 flavors
Posted by Bojan Smojver <bo...@rexursive.com>.
On Fri, 2007-05-11 at 08:14 -0500, William A. Rowe, Jr. wrote:
> Trouble with the submitted patch is that it isn't thread safe, I see all
> sorts of subtle races in this code :(
Yes, I was afraid someone may notice :-(
I'm attaching a patch that features a mutex to make things safer.
Hopefully a bit better. Also, I dropped OS2 and Win32 stuff - no need.
> But maybe I'm mis-reading things - does this only get toggled within the
> child process after fork()?
I think so.
--
Bojan
Re: Time to revisit 1.2.9 flavors
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Bojan Smojver wrote:
> On Sat, 2007-04-28 at 09:02 +1000, Bojan Smojver wrote:
>
>> I guess 41119 will be left the way it is? Should we just document and
>> close?
> +static int cleanup_for_exec = 0;
Trouble with the submitted patch is that it isn't thread safe, I see all
sorts of subtle races in this code :(
But maybe I'm mis-reading things - does this only get toggled within the
child process after fork()?
Re: Time to revisit 1.2.9 flavors
Posted by Bojan Smojver <bo...@rexursive.com>.
On Sat, 2007-04-28 at 09:02 +1000, Bojan Smojver wrote:
> I guess 41119 will be left the way it is? Should we just document and
> close?
Here is that hackish patch (also attached to the bug report).
What's the final verdict on this behaviour? Document and close? Or
patch?
--
Bojan
Re: Time to revisit 1.2.9 flavors
Posted by Bojan Smojver <bo...@rexursive.com>.
On Fri, 2007-04-27 at 10:46 -0500, William A. Rowe, Jr. wrote:
> Consider this a last call if you have some free cycles to hack on Saturday.
I guess 41119 will be left the way it is? Should we just document and
close?
--
Bojan