Re: [libmicrohttpd] MHD_handle_connection race leads to deadlock in MHD_stop_daemon

[Christian Grothoff]

Yes, this looks like the usual race of select with a signal.  The pipe 
approach is what I've usually taken in the past, but when MHD was written I 
really wanted to avoid it (and I guess then the issue fell through the cracks 
at some point).  

I've thought about the issue and I think the best solution is to do a bit of 
busy-waiting: I've changed the code around SELECT to ensure that the maximum 
timeout is 1s.  That way, if we miss the SIGALRM, we will still terminate 
after at most 1s.  My feeling is that this is better than wasting 2 FDs on a 
pipe.  Obviously I could be convinced of using a pipe instead if someone had 
benchmarks that show that calling select every 1s is horrible.

If someone has strong feelings about this matter in one way or the other, let 
me know, I'm open for debate (but clearly the existing code should not stay 
as-is).

Best,

Christian

On Tuesday 13 October 2009 17:41:12 Mike Crowe wrote:
> I've found a difficult to reproduce deadlock in v0.4.2 when calling
> MHD_stop_daemon while a connection is active. I narrowed it down to
> MHD_handle_connection racing against MHD_cleanup_connections. It can
> be reproduced much more easily by inserting a sleep just before the
> call to SELECT in MHD_handle_connection. I'm using
> MHD_USE_THREAD_PER_CONNECTION.
> 
> The problem appears to occur when MHD_stop_daemon sets
> daemon->shutdown and sends SIGALRM signal to the thread running inside
> MHD_handle_connection while it is between the top of the loop where
> con->daemon->shutdown is checked and the call to SELECT. The signal
> has no persistent effect and the thread ends up blocked forever inside
> select.
> 
> The only workarounds I can come up with are using an explicit pipe to
> wake the thread (which is stateful so it can be relied upon to wake
> the select) or to force the socket closed so that the select wakes up
> (but this sounds rather more dangerous and is racey if anyone else is
> opening file descriptors.)
> 
> Here's a patch which reproduces the problem under 'make check'. I'm
> running on Debian GNU/Linux 5.0 on amd64 but I've also reproduced the
> problem on i386.
> 
> I can clean up the patch to make it suitable for checking in but the
> chances of hitting the problem without the sleep in
> MHD_handle_connection are very small.
> 
> Index: src/daemon/daemon.c
> ===================================================================
> --- src/daemon/daemon.c       (revision 9153)
> +++ src/daemon/daemon.c       (working copy)
> @@ -487,6 +487,7 @@
>            timeout = 1;
>            tv.tv_sec = 0;
>        }
> +      usleep(1000000);
>        num_ready = SELECT (max + 1,
>                            &rs, &ws, &es, (timeout != 0) ? &tv : NULL);
>        if (num_ready < 0)
> Index: src/testcurl/daemontest_get.c
> ===================================================================
> --- src/testcurl/daemontest_get.c     (revision 9153)
> +++ src/testcurl/daemontest_get.c     (working copy)
> @@ -434,7 +434,48 @@
>    return 0;
>  }
> 
> +static int
> +testStopRace ()
> +{
> +    struct sockaddr_in sin;
> +    int fd;
> +    struct MHD_Daemon *d;
> +
> +    d = MHD_start_daemon (MHD_USE_THREAD_PER_CONNECTION | MHD_USE_DEBUG,
> +                       1081, NULL, NULL, &ahc_echo, "GET", MHD_OPTION_END);
> +    if (d == NULL)
> +     return 16;
> +
> +    fd = socket(PF_INET, SOCK_STREAM, 0);
> +    if (fd < 0)
> +    {
> +     fprintf(stderr, "socket: %m\n");
> +     return 256;
> +    }
> +
> +    memset(&sin, 0, sizeof(sin));
> +    sin.sin_family = AF_INET;
> +    sin.sin_port = htons(1081);
> +    sin.sin_addr.s_addr = htonl(0x7f000001);
> +
> +    if (connect(fd, (struct sockaddr *)(&sin), sizeof(sin)) < 0)
> +    {
> +     fprintf(stderr, "connect: %m\n");
> +     return 512;
> +    }
> +
> +    printf("Waiting\n");
> +    // Let the thread get going.
> +    usleep(500000);
> +
> +    printf("Stopping daemon\n");
> +    MHD_stop_daemon (d);
> 
> +    close(fd);
> +
> +    printf("good\n");
> +    return 0;
> +}
> 
>  int
>  main (int argc, char *const *argv)
> @@ -444,6 +485,7 @@
>    oneone = NULL != strstr (argv[0], "11");
>    if (0 != curl_global_init (CURL_GLOBAL_WIN32))
>      return 2;
> +  errorCount += testStopRace ();
>    errorCount += testInternalGet ();
>    errorCount += testMultithreadedGet ();
>    errorCount += testMultithreadedPoolGet ();
> 
> Thanks.
> 
> Mike.
> 

-- 
http://grothoff.org/christian/