Mailing List Archive: Apache: Dev

worker MPM: close_worker_sockets race?

Index | Next | Previous | View Threaded

bojan at rexursive

Oct 21, 2009, 6:08 PM

Post #1 of 12 (1012 views)
Permalink

worker MPM: close_worker_sockets race?

Looking at this function in worker.c:
----------------
static void close_worker_sockets(void)
{
int i;
for (i = 0; i < ap_threads_per_child; i++) {
if (worker_sockets[i]) {
apr_socket_close(worker_sockets[i]);
worker_sockets[i] = NULL;
}
}
}
----------------

Isn't there are possible race condition there, given that worker threads
can also change worker_sockets[i] at the same time? Or are we suspending
worker threads before this gets called?

--
Bojan

trawick at gmail

Oct 22, 2009, 4:54 AM

Post #2 of 12 (967 views)
Permalink

Re: worker MPM: close_worker_sockets race? [In reply to]

On Wed, Oct 21, 2009 at 9:08 PM, Bojan Smojver <bojan [at] rexursive> wrote:
> Looking at this function in worker.c:
> ----------------
> static void close_worker_sockets(void)
> {
> int i;
> for (i = 0; i < ap_threads_per_child; i++) {
> if (worker_sockets[i]) {
> apr_socket_close(worker_sockets[i]);
> worker_sockets[i] = NULL;
> }
> }
> }
> ----------------
>
> Isn't there are possible race condition there, given that worker threads
> can also change worker_sockets[i] at the same time? Or are we suspending
> worker threads before this gets called?

I have long suspected that there is a race here, and that this code
should do shutdown for read and write rather than close in order to
avoid any possible funny business with descriptors.

bojan at rexursive

Oct 22, 2009, 1:58 PM

Post #3 of 12 (957 views)
Permalink

Re: worker MPM: close_worker_sockets race? [In reply to]

On Thu, 2009-10-22 at 07:54 -0400, Jeff Trawick wrote:
> I have long suspected that there is a race here, and that this code
> should do shutdown for read and write rather than close in order to
> avoid any possible funny business with descriptors.

Phew! So I'm not totally insane after all :-)

Note that the attached _still_ has a race. If worker_sockets[i] is set
to NULL by the worker (i.e. processing just finished) before we get that
csd, we are going to segfault.

Having ap_threads_per_child mutexes (i.e. one for each
worker_sockets[i]) would probably be an overkill, so if we had just one
for all worker_sockets, that would be an improvement, although it would
slow things down a little bit.

--
Bojan

Attachments:

close_worker_sockets.patch (0.58 KB)

bojan at rexursive

Oct 22, 2009, 2:17 PM

Post #4 of 12 (962 views)
Permalink

Re: worker MPM: close_worker_sockets race? [In reply to]

On Fri, 2009-10-23 at 07:58 +1100, Bojan Smojver wrote:
> + apr_os_sock_get((apr_os_sock_t *)&csd, worker_sockets[j]);

He, he... Cut'n'paste eh? It's not [j], it's [i], of course :-)

--
Bojan

bojan at rexursive

Oct 22, 2009, 4:12 PM

Post #5 of 12 (958 views)
Permalink

Re: worker MPM: close_worker_sockets race? [In reply to]

On Fri, 2009-10-23 at 07:58 +1100, Bojan Smojver wrote:
> Note that the attached _still_ has a race.

Slower, but safer attached (not even compiled, so could be utterly
bogus).

--
Bojan

Attachments:

close_worker_sockets.patch (2.12 KB)

bojan at rexursive

Oct 22, 2009, 4:13 PM

Post #6 of 12 (963 views)
Permalink

Re: worker MPM: close_worker_sockets race? [In reply to]

On Fri, 2009-10-23 at 10:12 +1100, Bojan Smojver wrote:
> int i;

AIEE! Missing csd declaration above.

--
Bojan

bojan at rexursive

Oct 29, 2009, 7:07 PM

Post #7 of 12 (892 views)
Permalink

Re: worker MPM: close_worker_sockets race? [In reply to]

On Fri, 2009-10-23 at 10:12 +1100, Bojan Smojver wrote:
> Slower, but safer attached (not even compiled, so could be utterly
> bogus).

BTW, this approach still isn't safe. The socket can disappear (i.e. be
closed) between the test and the closing (or shutdown()). Worse, with
shutdown(), we could attempt to shut down the wrong FD, if we have fast
succession of close()/open() in various threads.

I think the correct approach would be to make sure that we actually
suspend worker threads before we do this. In other words, we send a
signal to each worker thread and their signal handler calls
sigsuspend(), thus waiting to be resumed. We also set thread specific
variables, notifying the listener thread about workers that have been
suspended.

Once the listener thread knows this (i.e. that all workers are
suspended), it shuts the sockets down and resumes the worker threads by
sending them a signal.

Workable?

--
Bojan

bojan at rexursive

Oct 29, 2009, 8:59 PM

Post #8 of 12 (894 views)
Permalink

Re: worker MPM: close_worker_sockets race? [In reply to]

On Fri, 2009-10-30 at 13:07 +1100, Bojan Smojver wrote:
> Workable?

Something like the attached. This compiles and kinda does something, but
it has not been extensively tested. Of course, SIGINT plucked out of
thin air.

--
Bojan

Attachments:

safe_close_worker_sockets.patch (4.69 KB)

bojan at rexursive

Oct 29, 2009, 9:48 PM

Post #9 of 12 (889 views)
Permalink

Re: worker MPM: close_worker_sockets race? [In reply to]

On Fri, 2009-10-30 at 14:59 +1100, Bojan Smojver wrote:
> + apr_os_sock_get((apr_os_sock_t *)&csd,
> worker_data[i].socket);
> + if (csd != -1) {
> + shutdown(csd, SHUT_RDWR);

BTW, there is a possibility of a race condition above. Slight, but still
there.

In APR, we set the FD to -1 only if close() finished OK. This means that
there is a chance that close() already closed this FD and that another
thread already reused it, but we are still getting it from
apr_os_sock_get(). In such a case, we'd be shutting down the wrong FD.

Maybe the APR code should set the FD to -1 before close() and then
restore it back to original FD if close() was not successful in
apr_socket_close(). With that approach, this code would not attempt to
shut down that FD. Instead, there would be a good chance that close()
whacks it instead upon resume and things continue as normal.

--
Bojan

bojan at rexursive

Oct 30, 2009, 7:14 PM

Post #10 of 12 (883 views)
Permalink

Re: worker MPM: close_worker_sockets race? [In reply to]

On Fri, 2009-10-30 at 14:59 +1100, Bojan Smojver wrote:
> Of course, SIGINT plucked out of thin air.

Actually, we can just reuse WORKER_SIGNAL for all this.

--
Bojan

Attachments:

safe_close_worker_sockets.patch (4.79 KB)

bojan at rexursive

Oct 31, 2009, 5:00 AM

Post #11 of 12 (869 views)
Permalink

Re: worker MPM: close_worker_sockets race? [In reply to]

On Sat, 2009-10-31 at 13:14 +1100, Bojan Smojver wrote:
> Actually, we can just reuse WORKER_SIGNAL for all this.

And even simpler.

--
Bojan

Attachments:

safe_close_worker_sockets.patch (4.31 KB)

bojan at rexursive

Oct 31, 2009, 5:34 AM

Post #12 of 12 (865 views)
Permalink

Re: worker MPM: close_worker_sockets race? [In reply to]

On Sat, 2009-10-31 at 23:00 +1100, Bojan Smojver wrote:
> + /* wait to resume */
> + while (suspend_workers) {
> + apr_sleep(SCOREBOARD_MAINTENANCE_INTERVAL / 10);
> + }

Er, that should be:

+ /* suspend and then wait to resume */
+ if (suspend_workers) {
+ worker_data[i].suspended = 1;
+
+ while (suspend_workers) {
+ apr_sleep(SCOREBOARD_MAINTENANCE_INTERVAL / 10);
+ }
+ }

--
Bojan

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads