Consolidate all tcp/http tear down into a single http_close function to prevent a rare UAF for expiring connections

6 files changed, 83 insertions, 33 deletions

diff --git a/Makefile b/Makefile
index f79d6a6..3ce127e 100644
--- a/Makefile
+++ b/Makefile
@@ -128,7 +128,9 @@ check-sanitize: $(TESTBIN).sanitize
 # cannot reach. Built with the sanitizers on, so the tracker reports its own
 # memory errors and the harness turns those into failures.
 PYTHON?=python3
-FEATURES_integration=$(FEATURES) -DWANT_IP_FROM_QUERY_STRING -DWANT_ACCESSLIST_BLACK -DWANT_KEEPALIVE
+# The client timeout is cut right down so that tests can reach the expiry
+# sweep. tests/integration/test_expiry.py mirrors these two numbers.
+FEATURES_integration=$(FEATURES) -DWANT_IP_FROM_QUERY_STRING -DWANT_ACCESSLIST_BLACK -DWANT_KEEPALIVE -DOT_CLIENT_TIMEOUT=5 -DOT_CLIENT_TIMEOUT_CHECKINTERVAL=1
 CFLAGS_integration=$(CFLAGS) $(OPTS_debug) $(FEATURES_integration) -fsanitize=address,undefined -fno-omit-frame-pointer
 LDFLAGS_integration=$(LDFLAGS) -fsanitize=address,undefined
 
diff --git a/opentracker.c b/opentracker.c
index e0f16d7..6160696 100644
--- a/opentracker.c
+++ b/opentracker.c
@@ -115,29 +115,12 @@ static ssize_t header_complete(char *request, ssize_t byte_count) {
   return 0;
 }
 
-static void handle_dead(const int64_t sock) {
-  struct http_data *cookie = fdstate_cookie(sock);
-  if (cookie) {
-    if (cookie->batch)
-      batch_reset(cookie->batch);
-    free(cookie->batch);
-    buf_reset(&cookie->request);
-    if (cookie->flag & (STRUCT_HTTP_FLAG_WAITINGFORTASK | STRUCT_HTTP_FLAG_CHUNKED_IN_TRANSFER))
-      mutex_workqueue_canceltask(sock);
-    free(cookie);
-  }
-  /* The generation has to move before the kernel can reissue the number */
-  fdstate_close(sock);
-  event_del((int)sock);
-  close((int)sock);
-}
-
 static void handle_read(const int64_t sock, struct ot_workstruct *ws) {
   struct http_data *cookie     = fdstate_cookie(sock);
   ssize_t           byte_count = read((int)sock, ws->inbuf, G_INBUF_SIZE);
 
   if (byte_count == 0 || (byte_count == -1 && errno != EAGAIN && errno != EWOULDBLOCK && errno != EINTR)) {
-    handle_dead(sock);
+    http_close(sock);
     return;
   }
 
@@ -156,7 +139,7 @@ static void handle_read(const int64_t sock, struct ot_workstruct *ws) {
       ws->request_size = byte_count;
       http_handle_request(sock, ws);
     } else if (buf_cat(&cookie->request, ws->inbuf, (size_t)byte_count))
-      handle_dead(sock);
+      http_close(sock);
     return;
   }
 
@@ -187,7 +170,7 @@ static void handle_write(const int64_t sock) {
 
     if (cookie->batch) {
       if (batch_send(sock, cookie->batch) == BATCH_FAILED) {
-        handle_dead(sock);
+        http_close(sock);
         return;
       }
       if (batch_pending(cookie->batch))
@@ -205,7 +188,7 @@ static void handle_write(const int64_t sock) {
     http_interest_add(sock, cookie, EVENT_IN);
     fdstate_set_expiry(sock, time_future_seconds(OT_CLIENT_TIMEOUT));
   } else
-    handle_dead(sock);
+    http_close(sock);
 }
 
 static void handle_accept(const int64_t serversocket, struct ot_workstruct *ws) {
@@ -303,7 +286,7 @@ static void *server_mainloop(void *args) {
       int cursor = 0;
 
       while ((sock = fdstate_next_expired(time_now_seconds(), &cursor)) != -1)
-        handle_dead(sock);
+        http_close(sock);
       next_timeout_check = time_future_seconds(OT_CLIENT_TIMEOUT_CHECKINTERVAL);
     }
 
diff --git a/ot_http.c b/ot_http.c
index 927d618..c2f7e5e 100644
--- a/ot_http.c
+++ b/ot_http.c
@@ -38,12 +38,34 @@ ssize_t      g_stats_path_len;
 
 enum { SUCCESS_HTTP_HEADER_LENGTH = 80, SUCCESS_HTTP_SIZE_OFF = 17 };
 
+/* Give up a connection: the one place that knows everything hanging off it.
+   The slot is released before anything is freed, so that a late event or the
+   expiry sweep finds a closed descriptor rather than a freed cookie. */
+void http_close(const int64_t sock) {
+  struct http_data *cookie = fdstate_cookie(sock);
+
+  fdstate_close((int)sock);
+
+  if (cookie) {
+    if (cookie->batch)
+      batch_reset(cookie->batch);
+    free(cookie->batch);
+    buf_reset(&cookie->request);
+    if (cookie->flag & (STRUCT_HTTP_FLAG_WAITINGFORTASK | STRUCT_HTTP_FLAG_CHUNKED_IN_TRANSFER))
+      mutex_workqueue_canceltask(sock);
+    free(cookie);
+  }
+
+  event_del((int)sock);
+  close((int)sock);
+}
+
 static void http_senddata(const int64_t sock, struct ot_workstruct *ws) {
   struct http_data *cookie = fdstate_cookie(sock);
   ssize_t           written_size;
 
   if (!cookie) {
-    event_del((int)sock), close((int)sock);
+    http_close(sock);
     return;
   }
 
@@ -65,9 +87,7 @@ static void http_senddata(const int64_t sock, struct ot_workstruct *ws) {
 
   written_size = write(sock, ws->reply, ws->reply_size);
   if ((written_size < 0) || ((written_size == ws->reply_size) && !ws->keep_alive)) {
-    buf_reset(&cookie->request);
-    free(cookie);
-    event_del((int)sock), close((int)sock);
+    http_close(sock);
     return;
   }
 
@@ -75,9 +95,7 @@ static void http_senddata(const int64_t sock, struct ot_workstruct *ws) {
     char   *outbuf;
 
     if (!(outbuf = malloc(ws->reply_size - written_size))) {
-      buf_reset(&cookie->request);
-      free(cookie);
-      event_del((int)sock), close((int)sock);
+      http_close(sock);
       return;
     }
 
@@ -86,9 +104,7 @@ static void http_senddata(const int64_t sock, struct ot_workstruct *ws) {
       cookie->batch = calloc(1, sizeof(ot_batch));
       if (!cookie->batch) {
         free(outbuf);
-        buf_reset(&cookie->request);
-        free(cookie);
-        event_del((int)sock), close((int)sock);
+        http_close(sock);
         return;
       }
     }
diff --git a/ot_http.h b/ot_http.h
index 1ccabfd..0f351c1 100644
--- a/ot_http.h
+++ b/ot_http.h
@@ -37,6 +37,9 @@ struct http_data {
 void           http_interest_add(const int64_t s, struct http_data *cookie, int what);
 void           http_interest_drop(const int64_t s, struct http_data *cookie, int what);
 
+/* Close a connection and release everything that belongs to it */
+void           http_close(const int64_t s);
+
 ssize_t        http_handle_request(const int64_t s, struct ot_workstruct *ws);
 ssize_t        http_sendiovecdata(const int64_t s, struct ot_workstruct *ws, size_t iovec_entries, struct iovec *iovector, int is_partial);
 ssize_t        http_issue_error(const int64_t s, struct ot_workstruct *ws, int code);
diff --git a/tests/integration/test_expiry.py b/tests/integration/test_expiry.py
new file mode 100644
index 0000000..4466724
--- /dev/null
+++ b/tests/integration/test_expiry.py
@@ -0,0 +1,42 @@
+"""What the expiry sweep finds after a connection has already gone.
+
+A plain request is answered and the connection closed by http_senddata, not
+by the sweep. If that path leaves the descriptor's slot behind, the sweep
+walks into it once the client timeout has passed and frees a cookie that is
+already gone. Under the sanitizer build that is reported, and teardown turns
+it into a failure.
+
+The two numbers below mirror OT_CLIENT_TIMEOUT and
+OT_CLIENT_TIMEOUT_CHECKINTERVAL in FEATURES_integration.
+"""
+
+import time
+import unittest
+
+from harness import TrackerTestCase
+
+CLIENT_TIMEOUT = 5
+CHECK_INTERVAL = 1
+
+
+class ExpirySweep(TrackerTestCase):
+    def test_an_answered_request_leaves_nothing_behind(self):
+        info_hash = self.hash_for("expiry-sweep")
+        self.assertEqual(self.tracker.announce(info_hash)[b"complete"], 1)
+
+        # Long enough for the slot to expire and for a sweep to visit it
+        time.sleep(CLIENT_TIMEOUT + 3 * CHECK_INTERVAL)
+
+        self.assertEqual(self.tracker.torrent_count(), 1, "the tracker stopped serving while sweeping")
+
+    def test_a_connection_that_never_speaks_is_swept(self):
+        """The other side of it: a slot that really is live must still go."""
+        import socket
+
+        idle = socket.create_connection(("127.0.0.1", self.tracker.port), timeout=5)
+        try:
+            time.sleep(CLIENT_TIMEOUT + 3 * CHECK_INTERVAL)
+            idle.settimeout(5)
+            self.assertEqual(idle.recv(64), b"", "the tracker held a connection that never sent a request")
+        finally:
+            idle.close()
diff --git a/trackerlogic.h b/trackerlogic.h
index 7d0693a..d269267 100644
--- a/trackerlogic.h
+++ b/trackerlogic.h
@@ -32,8 +32,12 @@ typedef struct {
 #define OT_TIME_SIZE                    1
 
 /* Some tracker behaviour tunable */
+#ifndef OT_CLIENT_TIMEOUT
 #define OT_CLIENT_TIMEOUT               30
+#endif
+#ifndef OT_CLIENT_TIMEOUT_CHECKINTERVAL
 #define OT_CLIENT_TIMEOUT_CHECKINTERVAL 10
+#endif
 #define OT_CLIENT_TIMEOUT_SEND          (60 * 15)
 #define OT_CLIENT_REQUEST_INTERVAL      (60 * 30)
 #define OT_CLIENT_REQUEST_VARIATION     (60 * 6)