From 79f4110365fe85f4377c50f0893134a8d5e45575 Mon Sep 17 00:00:00 2001 From: Dirk Engling Date: Fri, 30 Oct 2026 01:00:52 +0100 Subject: Consolidate all tcp/http tear down into a single http_close function to prevent a rare UAF for expiring connections --- Makefile | 4 +++- opentracker.c | 27 +++++--------------------- ot_http.c | 36 ++++++++++++++++++++++++---------- ot_http.h | 3 +++ tests/integration/test_expiry.py | 42 ++++++++++++++++++++++++++++++++++++++++ trackerlogic.h | 4 ++++ 6 files changed, 83 insertions(+), 33 deletions(-) create mode 100644 tests/integration/test_expiry.py 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) -- cgit v1.3