apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lucian Adrian Grijincu" <lucian.griji...@gmail.com>
Subject Re: Time to revisit 1.2.9 flavors
Date Fri, 11 May 2007 15:03:32 GMT
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. <wrowe@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()?
>

Mime
View raw message