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