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

[Dan Dedrick]

Somewhat related it's probably worth putting the whole epoll switch statement in connection.c under an "if (0 != (daemon->options & MHD_USE_EPOLL_LINUX_ONLY))" because there is no point in adding the connection to the epoll linked list or updating the epoll_state when we are not running in epoll mode.

Dan

On Tue, Mar 15, 2016 at 12:52 PM, Dan Dedrick <address@hidden> 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
 };


--
2.4.3