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 <[email protected]> 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 > >
