Re: [libmicrohttpd] [PATCH] epoll: add locking around eready DLL

[Dan Dedrick]

I actually did see a SIGSEGV in EDLL_insert() on "(head)->prevE = element;". tail was a valid pointer but the value for head was NULL. When I look at the coredump though I see that the head and tail are both valid pointers at this point and they point to a different connection pointer than the one we were looking to add. I can see that there are 2 threads in this process in MHD_handle_connection() for the same daemon. It might be worth noting that this is running with MHD_USE_THREAD_PER_CONNECTION where as far as I can see does share a daemon structure across multiple threads.

Where this failed it was running 0.9.34 of libmicrohttpd. I'm going to dig in to see if I can see how this could get into this state.

On Tue, Mar 15, 2016 at 2:07 PM, Christian Grothoff <address@hidden> wrote:

Hi Dan,

I'm a tad confused. You do realize that each _thread_ has its own
"struct MHD_Daemon" with its own "eready_head" and "eready_tail", and
that thus there isn't really the possibility of a race as long as each
thread sticks to its list?

This was done deliberately to avoid having to do any locking like this.


So did you actually have a SIGSEGV that you did trace to this particular
action (which should be impossible), or were you merely speculating that
it "might" happen?


Happy hacking!

Christian

On 03/15/2016 05:52 PM, Dan Dedrick wrote:
> This list was being modified on multiple threads with no locking.
> Specifically if 2 connections were made close together and each spawned
> their own threads they could both be adding themselves to the list at
> the same time. The resulting issue is that one might add itself to the
> tail first and then the second thread could come in before the first
> added itself to the head. The result is that it would see that there was
> a tail and attempt to access the head which, since it is NULL, causes a
> SIGSEGV. Adding this locking should prevent this issue.
>
> Additionally it is worth noting that if EPOLL_SUPPORT is set the eready
> DLL is inserted to even if the MHD_USE_EPOLL_LINUX_ONLY option is not
> set. So not only are users of MHD_USE_EPOLL_LINUX_ONLY vulernerable but
> others are as well.
> ---
>  src/microhttpd/connection.c | 12 +++++++++
>  src/microhttpd/daemon.c     | 62 +++++++++++++++++++++++++++++++++++++++++++++
>  src/microhttpd/internal.h   |  7 +++++
>  3 files changed, 81 insertions(+)
>
> diff --git a/src/microhttpd/connection.c b/src/microhttpd/connection.c
> index 0a9f988..0be52ff 100644
> --- a/src/microhttpd/connection.c
> +++ b/src/microhttpd/connection.c
> @@ -2867,9 +2867,13 @@ MHD_connection_handle_idle (struct MHD_Connection *connection)
>             (0 == (connection->epoll_state & MHD_EPOLL_STATE_SUSPENDED)) &&
>          (0 == (connection->epoll_state & MHD_EPOLL_STATE_IN_EREADY_EDLL)) )
>       {
> +          if (MHD_YES != MHD_mutex_lock_ (&daemon->eready_mutex))
> +            MHD_PANIC ("Failed to acquire epoll ready mutex\n");
>         EDLL_insert (daemon->eready_head,
>                      daemon->eready_tail,
>                      connection);
> +          if (MHD_YES != MHD_mutex_unlock_ (&daemon->eready_mutex))
> +            MHD_PANIC ("Failed to release epoll mutex\n");
>         connection->epoll_state |= MHD_EPOLL_STATE_IN_EREADY_EDLL;
>       }
>        break;
> @@ -2878,9 +2882,13 @@ MHD_connection_handle_idle (struct MHD_Connection *connection)
>             (0 == (connection->epoll_state & MHD_EPOLL_STATE_SUSPENDED)) &&
>          (0 == (connection->epoll_state & MHD_EPOLL_STATE_IN_EREADY_EDLL)) )
>       {
> +          if (MHD_YES != MHD_mutex_lock_ (&daemon->eready_mutex))
> +            MHD_PANIC ("Failed to acquire epoll ready mutex\n");
>         EDLL_insert (daemon->eready_head,
>                      daemon->eready_tail,
>                      connection);
> +          if (MHD_YES != MHD_mutex_unlock_ (&daemon->eready_mutex))
> +            MHD_PANIC ("Failed to release epoll mutex\n");
>         connection->epoll_state |= MHD_EPOLL_STATE_IN_EREADY_EDLL;
>       }
>        break;
> @@ -2890,9 +2898,13 @@ MHD_connection_handle_idle (struct MHD_Connection *connection)
>        if ( (0 == (connection->epoll_state & MHD_EPOLL_STATE_IN_EREADY_EDLL) &&
>             (0 == (connection->epoll_state & MHD_EPOLL_STATE_SUSPENDED))) )
>       {
> +          if (MHD_YES != MHD_mutex_lock_ (&daemon->eready_mutex))
> +            MHD_PANIC ("Failed to acquire epoll ready mutex\n");
>         EDLL_insert (daemon->eready_head,
>                      daemon->eready_tail,
>                      connection);
> +          if (MHD_YES != MHD_mutex_unlock_ (&daemon->eready_mutex))
> +            MHD_PANIC ("Failed to release epoll mutex\n");
>         connection->epoll_state |= MHD_EPOLL_STATE_IN_EREADY_EDLL;
>       }
>        break;
> diff --git a/src/microhttpd/daemon.c b/src/microhttpd/daemon.c
> index bbd7b42..8893b71 100644
> --- a/src/microhttpd/daemon.c
> +++ b/src/microhttpd/daemon.c
> @@ -1640,9 +1640,13 @@ internal_add_connection (struct MHD_Daemon *daemon,
>       {
>         connection->epoll_state |= MHD_EPOLL_STATE_READ_READY | MHD_EPOLL_STATE_WRITE_READY
>           | MHD_EPOLL_STATE_IN_EREADY_EDLL;
> +          if (MHD_YES != MHD_mutex_lock_ (&daemon->eready_mutex))
> +            MHD_PANIC ("Failed to acquire epoll ready mutex\n");
>         EDLL_insert (daemon->eready_head,
>                      daemon->eready_tail,
>                      connection);
> +          if (MHD_YES != MHD_mutex_unlock_ (&daemon->eready_mutex))
> +            MHD_PANIC ("Failed to release cleanup mutex\n");
>       }
>      }
>  #endif
> @@ -1736,9 +1740,13 @@ MHD_suspend_connection (struct MHD_Connection *connection)
>      {
>        if (0 != (connection->epoll_state & MHD_EPOLL_STATE_IN_EREADY_EDLL))
>          {
> +          if (MHD_YES != MHD_mutex_lock_ (&daemon->eready_mutex))
> +            MHD_PANIC ("Failed to acquire epoll ready mutex\n");
>            EDLL_remove (daemon->eready_head,
>                         daemon->eready_tail,
>                         connection);
> +          if (MHD_YES != MHD_mutex_unlock_ (&daemon->eready_mutex))
> +            MHD_PANIC ("Failed to release epoll mutex\n");
>            connection->epoll_state &= ~MHD_EPOLL_STATE_IN_EREADY_EDLL;
>          }
>        if (0 != (connection->epoll_state & MHD_EPOLL_STATE_IN_EPOLL_SET))
> @@ -1843,9 +1851,13 @@ resume_suspended_connections (struct MHD_Daemon *daemon)
>              MHD_PANIC ("Resumed connection was already in EREADY set\n");
>            /* we always mark resumed connections as ready, as we
>               might have missed the edge poll event during suspension */
> +          if (MHD_YES != MHD_mutex_lock_ (&daemon->eready_mutex))
> +            MHD_PANIC ("Failed to acquire epoll ready mutex\n");
>            EDLL_insert (daemon->eready_head,
>                         daemon->eready_tail,
>                         pos);
> +          if (MHD_YES != MHD_mutex_unlock_ (&daemon->eready_mutex))
> +            MHD_PANIC ("Failed to release epoll mutex\n");
>            pos->epoll_state |= MHD_EPOLL_STATE_IN_EREADY_EDLL;
>            pos->epoll_state &= ~MHD_EPOLL_STATE_SUSPENDED;
>          }
> @@ -2083,9 +2095,13 @@ MHD_cleanup_connections (struct MHD_Daemon *daemon)
>  #if EPOLL_SUPPORT
>        if (0 != (pos->epoll_state & MHD_EPOLL_STATE_IN_EREADY_EDLL))
>       {
> +          if (MHD_YES != MHD_mutex_lock_ (&daemon->eready_mutex))
> +            MHD_PANIC ("Failed to acquire epoll ready mutex\n");
>         EDLL_remove (daemon->eready_head,
>                      daemon->eready_tail,
>                      pos);
> +          if (MHD_YES != MHD_mutex_unlock_ (&daemon->eready_mutex))
> +            MHD_PANIC ("Failed to release epoll mutex\n");
>         pos->epoll_state &= ~MHD_EPOLL_STATE_IN_EREADY_EDLL;
>       }
>        if ( (0 != (daemon->options & MHD_USE_EPOLL_LINUX_ONLY)) &&
> @@ -2863,9 +2879,13 @@ MHD_epoll (struct MHD_Daemon *daemon,
>                        (pos->read_buffer_size > pos->read_buffer_offset) ) &&
>                      (0 == (pos->epoll_state & MHD_EPOLL_STATE_IN_EREADY_EDLL) ) )
>                   {
> +                      if (MHD_YES != MHD_mutex_lock_ (&daemon->eready_mutex))
> +                        MHD_PANIC ("Failed to acquire epoll ready mutex\n");
>                     EDLL_insert (daemon->eready_head,
>                                  daemon->eready_tail,
>                                  pos);
> +                      if (MHD_YES != MHD_mutex_unlock_ (&daemon->eready_mutex))
> +                        MHD_PANIC ("Failed to release epoll mutex\n");
>                     pos->epoll_state |= MHD_EPOLL_STATE_IN_EREADY_EDLL;
>                   }
>               }
> @@ -2875,9 +2895,13 @@ MHD_epoll (struct MHD_Daemon *daemon,
>                 if ( (MHD_EVENT_LOOP_INFO_WRITE == pos->event_loop_info) &&
>                      (0 == (pos->epoll_state & MHD_EPOLL_STATE_IN_EREADY_EDLL) ) )
>                   {
> +                      if (MHD_YES != MHD_mutex_lock_ (&daemon->eready_mutex))
> +                        MHD_PANIC ("Failed to acquire epoll ready mutex\n");
>                     EDLL_insert (daemon->eready_head,
>                                  daemon->eready_tail,
>                                  pos);
> +                      if (MHD_YES != MHD_mutex_unlock_ (&daemon->eready_mutex))
> +                        MHD_PANIC ("Failed to release epoll mutex\n");
>                     pos->epoll_state |= MHD_EPOLL_STATE_IN_EREADY_EDLL;
>                   }
>               }
> @@ -2902,18 +2926,26 @@ MHD_epoll (struct MHD_Daemon *daemon,
>      may_block = MHD_NO;
>
>    /* process events for connections */
> +  if (MHD_YES != MHD_mutex_lock_ (&daemon->eready_mutex))
> +    MHD_PANIC ("Failed to acquire epoll ready mutex\n");
>    while (NULL != (pos = daemon->eready_tail))
>      {
>        EDLL_remove (daemon->eready_head,
>                  daemon->eready_tail,
>                  pos);
> +      if (MHD_YES != MHD_mutex_unlock_ (&daemon->eready_mutex))
> +        MHD_PANIC ("Failed to release epoll mutex\n");
>        pos->epoll_state &= ~MHD_EPOLL_STATE_IN_EREADY_EDLL;
>        if (MHD_EVENT_LOOP_INFO_READ == pos->event_loop_info)
>       pos->read_handler (pos);
>        if (MHD_EVENT_LOOP_INFO_WRITE == pos->event_loop_info)
>       pos->write_handler (pos);
>        pos->idle_handler (pos);
> +      if (MHD_YES != MHD_mutex_lock_ (&daemon->eready_mutex))
> +        MHD_PANIC ("Failed to acquire epoll ready mutex\n");
>      }
> +  if (MHD_YES != MHD_mutex_unlock_ (&daemon->eready_mutex))
> +    MHD_PANIC ("Failed to release epoll mutex\n");
>
>    /* Finally, handle timed-out connections; we need to do this here
>       as the epoll mechanism won't call the 'idle_handler' on everything,
> @@ -4175,6 +4207,15 @@ MHD_start_daemon_va (unsigned int flags,
>        if (MHD_YES != setup_epoll_to_listen (daemon))
>       goto free_and_fail;
>      }
> +  if (MHD_YES != MHD_mutex_create_ (&daemon->eready_mutex))
> +    {
> +#ifdef HAVE_MESSAGES
> +      MHD_DLOG (daemon,
> +            "MHD failed to initialize epoll ready mutex\n");
> +#endif
> +      (void) MHD_mutex_destroy_ (&daemon->eready_mutex);
> +      goto free_and_fail;
> +    }
>  #else
>    if (0 != (flags & MHD_USE_EPOLL_LINUX_ONLY))
>      {
> @@ -4182,6 +4223,9 @@ MHD_start_daemon_va (unsigned int flags,
>        MHD_DLOG (daemon,
>               "epoll is not supported on this platform by this build.\n");
>  #endif
> +#if EPOLL_SUPPORT
> +      (void) MHD_mutex_destroy_ (&daemon->eready_mutex);
> +#endif
>        goto free_and_fail;
>      }
>  #endif
> @@ -4195,6 +4239,9 @@ MHD_start_daemon_va (unsigned int flags,
>        if ( (MHD_INVALID_SOCKET != socket_fd) &&
>          (0 != MHD_socket_close_ (socket_fd)) )
>       MHD_PANIC ("close failed\n");
> +#if EPOLL_SUPPORT
> +      (void) MHD_mutex_destroy_ (&daemon->eready_mutex);
> +#endif
>        goto free_and_fail;
>      }
>    if (MHD_YES != MHD_mutex_create_ (&daemon->cleanup_connection_mutex))
> @@ -4204,6 +4251,9 @@ MHD_start_daemon_va (unsigned int flags,
>                 "MHD failed to initialize IP connection limit mutex\n");
>  #endif
>        (void) MHD_mutex_destroy_ (&daemon->cleanup_connection_mutex);
> +#if EPOLL_SUPPORT
> +      (void) MHD_mutex_destroy_ (&daemon->eready_mutex);
> +#endif
>        if ( (MHD_INVALID_SOCKET != socket_fd) &&
>          (0 != MHD_socket_close_ (socket_fd)) )
>       MHD_PANIC ("close failed\n");
> @@ -4223,6 +4273,9 @@ MHD_start_daemon_va (unsigned int flags,
>       MHD_PANIC ("close failed\n");
>        (void) MHD_mutex_destroy_ (&daemon->cleanup_connection_mutex);
>        (void) MHD_mutex_destroy_ (&daemon->per_ip_connection_mutex);
> +#if EPOLL_SUPPORT
> +      (void) MHD_mutex_destroy_ (&daemon->eready_mutex);
> +#endif
>        goto free_and_fail;
>      }
>  #endif
> @@ -4240,6 +4293,9 @@ MHD_start_daemon_va (unsigned int flags,
>  #endif
>        (void) MHD_mutex_destroy_ (&daemon->cleanup_connection_mutex);
>        (void) MHD_mutex_destroy_ (&daemon->per_ip_connection_mutex);
> +#if EPOLL_SUPPORT
> +      (void) MHD_mutex_destroy_ (&daemon->eready_mutex);
> +#endif
>        if ( (MHD_INVALID_SOCKET != socket_fd) &&
>          (0 != MHD_socket_close_ (socket_fd)) )
>       MHD_PANIC ("close failed\n");
> @@ -4362,6 +4418,9 @@ thread_failed:
>       MHD_PANIC ("close failed\n");
>        (void) MHD_mutex_destroy_ (&daemon->cleanup_connection_mutex);
>        (void) MHD_mutex_destroy_ (&daemon->per_ip_connection_mutex);
> +#if EPOLL_SUPPORT
> +      (void) MHD_mutex_destroy_ (&daemon->eready_mutex);
> +#endif
>        if (NULL != daemon->worker_pool)
>          free (daemon->worker_pool);
>        goto free_and_fail;
> @@ -4663,6 +4722,9 @@ MHD_stop_daemon (struct MHD_Daemon *daemon)
>  #endif
>    (void) MHD_mutex_destroy_ (&daemon->per_ip_connection_mutex);
>    (void) MHD_mutex_destroy_ (&daemon->cleanup_connection_mutex);
> +#if EPOLL_SUPPORT
> +  (void) MHD_mutex_destroy_ (&daemon->eready_mutex);
> +#endif
>
>    if (MHD_INVALID_PIPE_ != daemon->wpipe[1])
>      {
> diff --git a/src/microhttpd/internal.h b/src/microhttpd/internal.h
> index 3856a62..ae8190c 100644
> --- a/src/microhttpd/internal.h
> +++ b/src/microhttpd/internal.h
> @@ -1306,6 +1306,13 @@ struct MHD_Daemon
>     * The size of queue for listen socket.
>     */
>    unsigned int listen_backlog_size;
> +
> +#if EPOLL_SUPPORT
> +  /**
> +   * Mutex for access to the epoll ready DLL.
> +   */
> +  MHD_mutex_ eready_mutex;
> +#endif
>  };
>
>
>