fixing crash bug, connection-limit bug and documenting connection-limit behavior better

5 files changed, 93 insertions, 37 deletions

diff --git a/ChangeLog b/ChangeLog
index 90b1ecf0..f79b8137 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+Mon Aug 15 13:06:52 CEST 2016
+	Fixed possible crash due to write to read-only region of
+	memory given ill-formed HTTP request (write was otherwise
+	harmless, writing 0 to where there was already a 0).
+	Fixed issue with closed connection slots not immediately
+	being available again for new connections if we reached
+	our connection limit.
+	Avoid even accept()ing connections in certain thread modes
+	if we are at the connection limit and
+	MHD_USE_PIPE_FOR_SHUTDOWN is available.
+
 Wed Aug 10 16:42:57 MSK 2016
 	Moved threads, locks and mutex abstraction to separate files,
 	some minor errors fixed, added support for thread name functions
diff --git a/doc/libmicrohttpd.texi b/doc/libmicrohttpd.texi
index ed2e58c9..49a74b4c 100644
--- a/doc/libmicrohttpd.texi
+++ b/doc/libmicrohttpd.texi
@@ -626,6 +626,15 @@ maximum number of file descriptors supported by @code{select} minus
 four for @code{stdin}, @code{stdout}, @code{stderr} and the server
 socket).  In other words, the default is as large as possible.
 
+If the connection limit is reached, MHD's behavior depends a bit on
+other options.  If @code{MHD_USE_PIPE_FOR_SHUTDOWN} was given, MHD
+will stop accepting connections on the listen socket.  This will cause
+the operating system to queue connections (up to the @code{listen()}
+limit) above the connection limit.  Those connections will be held
+until MHD is done processing at least one of the active connections.
+If @code{MHD_USE_PIPE_FOR_SHUTDOWN} is not set, then MHD will continue
+to @code{accept()} and immediately @code{close()} these connections.
+
 Note that if you set a low connection limit, you can easily get into
 trouble with browsers doing request pipelining.  For example, if your
 connection limit is ``1'', a browser may open a first connection to
diff --git a/src/include/microhttpd.h b/src/include/microhttpd.h
index 4e30a40d..d5e08d82 100644
--- a/src/include/microhttpd.h
+++ b/src/include/microhttpd.h
@@ -131,7 +131,7 @@ typedef intptr_t ssize_t;
  * Current version of the library.
  * 0x01093001 = 1.9.30-1.
  */
-#define MHD_VERSION 0x00095004
+#define MHD_VERSION 0x00095005
 
 /**
  * MHD-internal return code for "YES".
diff --git a/src/microhttpd/connection.c b/src/microhttpd/connection.c
index b3c43b63..6223b994 100644
--- a/src/microhttpd/connection.c
+++ b/src/microhttpd/connection.c
@@ -483,6 +483,19 @@ MHD_connection_close_ (struct MHD_Connection *connection,
 			      &connection->client_context,
 			      termination_code);
   connection->client_aware = MHD_NO;
+
+  /* if we were at the connection limit before and are in
+     thread-per-connection mode, signal the main thread
+     to resume accepting connections */
+  if ( (0 != (daemon->options & MHD_USE_THREAD_PER_CONNECTION)) &&
+       (MHD_INVALID_PIPE_ != daemon->wpipe[1]) &&
+       (1 != MHD_pipe_write_ (daemon->wpipe[1], "c", 1)) )
+    {
+#ifdef HAVE_MESSAGES
+      MHD_DLOG (daemon,
+                "failed to signal end of connection via pipe");
+#endif
+    }
 }
 
 
@@ -1527,6 +1540,7 @@ parse_initial_message_line (struct MHD_Connection *connection,
                             size_t line_len)
 {
   struct MHD_Daemon *daemon = connection->daemon;
+  const char *curi;
   char *uri;
   char *http_version;
   char *args;
@@ -1539,16 +1553,19 @@ parse_initial_message_line (struct MHD_Connection *connection,
   uri++;
   /* Skip any spaces. Not required by standard but allow
      to be more tolerant. */
-  while (' ' == uri[0] && (size_t)(uri - line) < line_len)
+  while ( (' ' == uri[0]) &&
+          ( (size_t)(uri - line) < line_len) )
     uri++;
   if (uri - line == line_len)
     {
-      uri = "";
+      curi = "";
+      uri = NULL;
       connection->version = "";
       args = NULL;
     }
   else
     {
+      curi = uri;
       /* Search from back to accept misformed URI with space */
       http_version = line + line_len - 1;
       /* Skip any trailing spaces */
@@ -1572,7 +1589,7 @@ parse_initial_message_line (struct MHD_Connection *connection,
   if (NULL != daemon->uri_log_callback)
     connection->client_context
       = daemon->uri_log_callback (daemon->uri_log_callback_cls,
-				  uri,
+				  curi,
 				  connection);
   if (NULL != args)
     {
@@ -1585,10 +1602,11 @@ parse_initial_message_line (struct MHD_Connection *connection,
 			    &connection_add_header,
 			    &unused_num_headers);
     }
-  daemon->unescape_callback (daemon->unescape_callback_cls,
-			     connection,
-			     uri);
-  connection->url = uri;
+  if (NULL != uri)
+    daemon->unescape_callback (daemon->unescape_callback_cls,
+                               connection,
+                               uri);
+  connection->url = curi;
   return MHD_YES;
 }
 
diff --git a/src/microhttpd/daemon.c b/src/microhttpd/daemon.c
index 7e13e311..77f43537 100644
--- a/src/microhttpd/daemon.c
+++ b/src/microhttpd/daemon.c
@@ -1281,6 +1281,17 @@ send_param_adapter (struct MHD_Connection *connection,
 
 
 /**
+ * Free resources associated with all closed connections.
+ * (destroy responses, free buffers, etc.).  All closed
+ * connections are kept in the "cleanup" doubly-linked list.
+ *
+ * @param daemon daemon to clean up
+ */
+static void
+MHD_cleanup_connections (struct MHD_Daemon *daemon);
+
+
+/**
  * Add another client connection to the set of connections
  * managed by MHD.  This API is usually not needed (since
  * MHD will accept inbound connections on the server socket).
@@ -1371,6 +1382,8 @@ internal_add_connection (struct MHD_Daemon *daemon,
             client_socket);
 #endif
 #endif
+  //if (daemon->connections == daemon->connection_limit)
+  // MHD_cleanup_connections (daemon); /* try to aggressively clean up to make room */
   if ( (daemon->connections == daemon->connection_limit) ||
        (MHD_NO == MHD_ip_limit_add (daemon, addr, addrlen)) )
     {
@@ -1537,7 +1550,7 @@ internal_add_connection (struct MHD_Daemon *daemon,
 
   if (0 != (daemon->options & MHD_USE_THREAD_PER_CONNECTION))
   {
-    if (!MHD_mutex_lock_ (&daemon->cleanup_connection_mutex))
+    if (! MHD_mutex_lock_ (&daemon->cleanup_connection_mutex))
       MHD_PANIC ("Failed to acquire cleanup mutex\n");
   }
   else
@@ -2070,7 +2083,7 @@ MHD_cleanup_connections (struct MHD_Daemon *daemon)
   struct MHD_Connection *pos;
 
   if ( (0 != (daemon->options & MHD_USE_THREAD_PER_CONNECTION)) &&
-       (!MHD_mutex_lock_ (&daemon->cleanup_connection_mutex)) )
+       (! MHD_mutex_lock_ (&daemon->cleanup_connection_mutex)) )
     MHD_PANIC ("Failed to acquire cleanup mutex\n");
   while (NULL != (pos = daemon->cleanup_head))
     {
@@ -2080,7 +2093,7 @@ MHD_cleanup_connections (struct MHD_Daemon *daemon)
       if ( (0 != (daemon->options & MHD_USE_THREAD_PER_CONNECTION)) &&
 	   (MHD_NO == pos->thread_joined) )
 	{
-	  if (!MHD_join_thread_ (pos->pid))
+	  if (! MHD_join_thread_ (pos->pid))
 	    {
 	      MHD_PANIC ("Failed to join a thread\n");
 	    }
@@ -2092,6 +2105,8 @@ MHD_cleanup_connections (struct MHD_Daemon *daemon)
 #endif
       daemon->connections--;
       daemon->at_limit = MHD_NO;
+
+      /* clean up the connection */
       if (NULL != daemon->notify_connection)
         daemon->notify_connection (daemon->notify_connection_cls,
                                    pos,
@@ -2141,7 +2156,7 @@ MHD_cleanup_connections (struct MHD_Daemon *daemon)
       free (pos);
     }
   if ( (0 != (daemon->options & MHD_USE_THREAD_PER_CONNECTION)) &&
-       (!MHD_mutex_unlock_ (&daemon->cleanup_connection_mutex)) )
+       (! MHD_mutex_unlock_ (&daemon->cleanup_connection_mutex)) )
     MHD_PANIC ("Failed to release cleanup mutex\n");
 }
 
@@ -2364,17 +2379,6 @@ MHD_select (struct MHD_Daemon *daemon,
 #endif
           err_state = MHD_YES;
         }
-
-      /* If we're at the connection limit, no need to
-         accept new connections; however, make sure
-         we do not miss the shutdown, so only do this
-         optimization if we have a shutdown signaling
-         pipe. */
-      if ( (MHD_INVALID_SOCKET != daemon->socket_fd) &&
-           ( ( (daemon->connections == daemon->connection_limit) &&
-             (0 != (daemon->options & MHD_USE_PIPE_FOR_SHUTDOWN)) ) ||
-             (MHD_YES == daemon->at_limit) ) )
-        FD_CLR (daemon->socket_fd, &rs);
     }
   else
     {
@@ -2420,7 +2424,21 @@ MHD_select (struct MHD_Daemon *daemon,
         }
 #endif /* MHD_WINSOCK_SOCKETS */
     }
-
+  /* Stop listening if we are at the configured connection limit */
+  /* If we're at the connection limit, no point in really
+     accepting new connections; however, make sure we do not miss
+     the shutdown OR the termination of an existing connection; so
+     only do this optimization if we have a signaling pipe in
+     place. */
+  if ( (MHD_INVALID_SOCKET != daemon->socket_fd) &&
+       (MHD_INVALID_PIPE_ != daemon->wpipe[0]) &&
+       (0 != (daemon->options & MHD_USE_PIPE_FOR_SHUTDOWN)) &&
+       ( (daemon->connections == daemon->connection_limit) ||
+         (MHD_YES == daemon->at_limit) ) )
+    {
+      FD_CLR (daemon->socket_fd,
+              &rs);
+    }
   tv = NULL;
   if (MHD_YES == err_state)
     may_block = MHD_NO;
@@ -4185,7 +4203,7 @@ MHD_start_daemon_va (unsigned int flags,
     }
 #endif
 
-  if (!MHD_mutex_init_ (&daemon->per_ip_connection_mutex))
+  if (! MHD_mutex_init_ (&daemon->per_ip_connection_mutex))
     {
 #ifdef HAVE_MESSAGES
       MHD_DLOG (daemon,
@@ -4196,7 +4214,7 @@ MHD_start_daemon_va (unsigned int flags,
 	MHD_PANIC ("close failed\n");
       goto free_and_fail;
     }
-  if (!MHD_mutex_init_ (&daemon->cleanup_connection_mutex))
+  if (! MHD_mutex_init_ (&daemon->cleanup_connection_mutex))
     {
 #ifdef HAVE_MESSAGES
       MHD_DLOG (daemon,
@@ -4229,12 +4247,12 @@ MHD_start_daemon_va (unsigned int flags,
 	 ( (0 != (flags & MHD_USE_SELECT_INTERNALLY)) &&
 	   (0 == daemon->worker_pool_size)) ) &&
        (0 == (daemon->options & MHD_USE_NO_LISTEN_SOCKET)) &&
-       (!MHD_create_named_thread_ (&daemon->pid,
-                                   (flags & MHD_USE_THREAD_PER_CONNECTION) ?
-                                       "MHD-listen" : "MHD-single",
-                                   daemon->thread_stack_size,
-                                   &MHD_select_thread,
-                                   daemon) ) )
+       (! MHD_create_named_thread_ (&daemon->pid,
+                                    (flags & MHD_USE_THREAD_PER_CONNECTION) ?
+                                    "MHD-listen" : "MHD-single",
+                                    daemon->thread_stack_size,
+                                    &MHD_select_thread,
+                                    daemon) ) )
     {
 #ifdef HAVE_MESSAGES
       MHD_DLOG (daemon,
@@ -4342,11 +4360,11 @@ MHD_start_daemon_va (unsigned int flags,
             }
 
           /* Spawn the worker thread */
-          if (!MHD_create_named_thread_(&d->pid,
-                                        "MHD-worker",
-                                        daemon->thread_stack_size,
-                                        &MHD_select_thread,
-                                        d))
+          if (! MHD_create_named_thread_(&d->pid,
+                                         "MHD-worker",
+                                         daemon->thread_stack_size,
+                                         &MHD_select_thread,
+                                         d))
             {
 #ifdef HAVE_MESSAGES
               MHD_DLOG (daemon,