From 93562917ff1ad8b800f10b374e275e353ce8562b Mon Sep 17 00:00:00 2001 From: Raphael Schlarb Date: Mon, 10 Aug 2026 04:17:02 -0500 Subject: [PATCH 1/4] refactor: split th_response_async_write into prepare_write + caller send - th_response_prepare_write finalizes headers and returns a th_response_write_plan (iov/file/offset/len), doing no I/O and dropping the th_conn dependency from th_response.h entirely - th_http_write_response now calls th_conn_send itself with the plan - fix an off-by-one in th_response_set_body_va's slow path: vsnprintf was given the formatted length instead of length + 1, truncating the last character on printf-style bodies >= 512 bytes test: rewrite th_response_test.c against the new plan-returning API - add coverage for th_set_body_from_file (missing/unknown extension, unknown root, open failure, explicit Content-Type) - add coverage for th_printf_body's slow (resize) path - add coverage for th_add_cookie (attributes, Expires, SameSite) - add coverage for th_response_reset --- src/th_http.c | 8 +- src/th_response.c | 26 +-- src/th_response.h | 25 ++- src/th_response_test.c | 444 +++++++++++++++++++++++++++++++---------- 4 files changed, 379 insertions(+), 124 deletions(-) diff --git a/src/th_http.c b/src/th_http.c index 32e6f7d..e31955a 100644 --- a/src/th_http.c +++ b/src/th_http.c @@ -52,7 +52,13 @@ th_http_complete(th_http* http) TH_LOCAL(void) th_http_write_response(th_http* http) { - th_response_async_write(&http->response, http->conn, th_http_handle_write_response, http); + th_response_write_plan plan; + th_err err = th_response_prepare_write(&http->response, &plan); + if (err != TH_ERR_OK) { + th_http_handle_write_response(http, 0, err); + return; + } + th_conn_send(http->conn, plan.iov, plan.iovcnt, plan.file, plan.offset, plan.len, th_http_handle_write_response, http); } TH_LOCAL(void) diff --git a/src/th_response.c b/src/th_response.c index ad61b33..0b2d700 100644 --- a/src/th_response.c +++ b/src/th_response.c @@ -176,7 +176,7 @@ th_response_set_body_va(th_response* response, const char* fmt, va_list args) } } else { th_string_resize(&response->body, (size_t)len, ' '); - vsnprintf(th_string_at(&response->body, 0), (size_t)len, fmt, args); + vsnprintf(th_string_at(&response->body, 0), (size_t)len + 1, fmt, args); } response->is_file = 0; return TH_ERR_OK; @@ -239,8 +239,8 @@ th_response_set_default_headers(th_response* response) return TH_ERR_OK; } -TH_PRIVATE(void) -th_response_async_write(th_response* response, th_conn* conn, th_send_cb callback, void* user_data) +TH_PRIVATE(th_err) +th_response_prepare_write(th_response* response, th_response_write_plan* plan) { th_err err = TH_ERR_OK; size_t iovcnt = 2; // start line + headers @@ -248,24 +248,26 @@ th_response_async_write(th_response* response, th_conn* conn, th_send_cb callbac response->file_len = response->fcache_entry->stream.size; } if ((err = th_response_set_default_headers(response)) != TH_ERR_OK) - goto cleanup; + return err; if ((err = th_response_finalize_headers(response)) != TH_ERR_OK) - goto cleanup; + return err; if (!response->only_headers && response->is_file == 0 && th_string_len(&response->body) > 0) { response->iov[iovcnt].base = (void*)th_string_data(&response->body); response->iov[iovcnt].len = th_string_len(&response->body); iovcnt++; } + plan->iov = response->iov; + plan->iovcnt = iovcnt; if (!response->only_headers && response->is_file != 0) { - th_conn_send(conn, response->iov, iovcnt, &response->fcache_entry->stream, 0, (size_t)response->file_len, callback, user_data); + plan->file = &response->fcache_entry->stream; + plan->offset = 0; + plan->len = (size_t)response->file_len; } else { - th_conn_send(conn, response->iov, iovcnt, NULL, 0, 0, callback, user_data); + plan->file = NULL; + plan->offset = 0; + plan->len = 0; } - return; -cleanup: - // Header formatting failed before any I/O was attempted (out of - // memory); safe to call back synchronously since no op is pending. - callback(user_data, 0, err); + return TH_ERR_OK; } /* Public response API begin */ diff --git a/src/th_response.h b/src/th_response.h index 6ffac34..fcb1a04 100644 --- a/src/th_response.h +++ b/src/th_response.h @@ -7,10 +7,11 @@ #include "th_allocator.h" #include "th_config.h" -#include "th_conn.h" #include "th_dir_mgr.h" #include "th_fcache.h" +#include "th_file.h" #include "th_header_id.h" +#include "th_iov.h" #include "th_string.h" /* th_response begin */ @@ -53,7 +54,25 @@ th_response_deinit(th_response* response); /* th_response end */ -TH_PRIVATE(void) -th_response_async_write(th_response* response, th_conn* conn, th_send_cb callback, void* user_data); +/** th_response_write_plan + * @brief What to send for a response: iov always (start line + headers, + * plus body if any), file/offset/len additionally if a file is being + * sent (file is NULL otherwise). + */ +typedef struct th_response_write_plan { + th_iov* iov; + size_t iovcnt; + th_file* file; + size_t offset; + size_t len; +} th_response_write_plan; + +/** th_response_prepare_write + * @brief Finalizes headers (default headers, start line) and fills out + * plan with what to send. Does no I/O - the caller sends plan itself + * (e.g. via th_conn_send). + */ +TH_PRIVATE(th_err) +th_response_prepare_write(th_response* response, th_response_write_plan* plan); #endif diff --git a/src/th_response_test.c b/src/th_response_test.c index d329150..326599d 100644 --- a/src/th_response_test.c +++ b/src/th_response_test.c @@ -1,170 +1,222 @@ #include "th_response.h" #include "th_test.h" +#include +#include #include -/* No test case here exercises th_set_body_from_file, so these ops are - * never actually invoked - th_fcache_init just needs a non-garbage - * pointer to store. */ -static th_file_ops th_unused_file_ops; +static bool +buf_contains(const char* haystack, size_t haystack_len, const char* needle) +{ + size_t needle_len = strlen(needle); + if (needle_len > haystack_len) + return false; + for (size_t i = 0; i + needle_len <= haystack_len; ++i) { + if (memcmp(haystack + i, needle, needle_len) == 0) + return true; + } + return false; +} -typedef struct th_fake_conn { - th_conn base; - char written[1024]; - size_t written_len; - bool sent_file; - size_t file_offset; - size_t file_len; -} th_fake_conn; +static bool +plan_contains(const th_response_write_plan* plan, const char* needle) +{ + for (size_t i = 0; i < plan->iovcnt; ++i) { + if (buf_contains(plan->iov[i].base, plan->iov[i].len, needle)) + return true; + } + return false; +} -static th_address* -th_fake_conn_get_address(void* self) +static bool +plan_start_line_is(const th_response_write_plan* plan, const char* expected) { - (void)self; - return NULL; + size_t expected_len = strlen(expected); + return plan->iov[0].len == expected_len && memcmp(plan->iov[0].base, expected, expected_len) == 0; } -static void -th_fake_conn_start(void* self) +static bool +plan_has_header_line(const th_response_write_plan* plan, const char* key, const char* value) { - (void)self; + char needle[256]; + snprintf(needle, sizeof(needle), "%s: %s\r\n", key, value); + return plan_contains(plan, needle); } -static void -th_fake_conn_send(void* self, th_iov* iov, size_t iovcnt, th_file* file, size_t offset, size_t len, th_send_cb callback, void* user_data) +static bool +plan_body_is(const th_response_write_plan* plan, const char* expected) { - th_fake_conn* conn = self; - size_t total = 0; - for (size_t i = 0; i < iovcnt; ++i) { - memcpy(conn->written + conn->written_len, iov[i].base, iov[i].len); - conn->written_len += iov[i].len; - total += iov[i].len; - } - conn->sent_file = file != NULL; - conn->file_offset = offset; - conn->file_len = len; - callback(user_data, total + len, TH_ERR_OK); + size_t expected_len = strlen(expected); + if (plan->iovcnt < 3) + return expected_len == 0; + return plan->iovcnt >= 3 && plan->iov[2].len == expected_len && memcmp(plan->iov[2].base, expected, expected_len) == 0; +} + +typedef struct fake_dir_ops { + th_dir_ops base; + int next_fd; +} fake_dir_ops; + +static th_err +fake_dir_ops_open(void* self, const char* path, int* fd) +{ + (void)path; + fake_dir_ops* ops = self; + *fd = ops->next_fd++; + return TH_ERR_OK; } static void -th_fake_conn_cancel(void* self) +fake_dir_ops_close(void* self, int fd) { (void)self; + (void)fd; } static void -th_fake_conn_destroy(void* self) +fake_dir_ops_init(fake_dir_ops* ops) { - (void)self; + ops->base.open = fake_dir_ops_open; + ops->base.close = fake_dir_ops_close; + ops->next_fd = 3; } -static const th_conn_methods th_fake_conn_methods = { - .get_address = th_fake_conn_get_address, - .start = th_fake_conn_start, - .recv = NULL, - .send = th_fake_conn_send, - .cancel = th_fake_conn_cancel, - .destroy = th_fake_conn_destroy, -}; +typedef struct fake_file_ops { + th_file_ops base; + int next_fd; + bool open_fails; + size_t file_size; +} fake_file_ops; -static void -th_fake_conn_init(th_fake_conn* conn) +static th_err +fake_file_ops_openat(void* self, int dirfd, const char* path, int flags, int* fd) { - conn->base.methods = &th_fake_conn_methods; - conn->written_len = 0; - conn->sent_file = false; - conn->file_offset = 0; - conn->file_len = 0; + (void)dirfd; + (void)path; + (void)flags; + fake_file_ops* ops = self; + if (ops->open_fails) + return TH_ERR_SYSTEM(ENOENT); + *fd = ops->next_fd++; + return TH_ERR_OK; } -/* Plain byte-substring search, rather than memmem: ASan's memmem - * interceptor in this environment spuriously returns NULL for a needle - * that is genuinely present (reproduced independent of this codebase). */ -static bool -th_buf_contains(const char* haystack, size_t haystack_len, const char* needle) +static th_err +fake_file_ops_seek(void* self, int fd, int whence, size_t* pos) { - size_t needle_len = strlen(needle); - if (needle_len > haystack_len) - return false; - for (size_t i = 0; i + needle_len <= haystack_len; ++i) { - if (memcmp(haystack + i, needle, needle_len) == 0) - return true; - } - return false; + (void)fd; + fake_file_ops* ops = self; + *pos = (whence == SEEK_END) ? ops->file_size : 0; + return TH_ERR_OK; +} + +static th_err +fake_file_ops_read(void* self, int fd, void* addr, size_t len, size_t offset, size_t* read) +{ + (void)self; + (void)fd; + (void)addr; + (void)offset; + *read = len; + return TH_ERR_OK; +} + +static th_err +fake_file_ops_write(void* self, int fd, const void* addr, size_t len, size_t offset, size_t* written) +{ + (void)self; + (void)fd; + (void)addr; + (void)offset; + *written = len; + return TH_ERR_OK; } -typedef struct th_recorded_result { - bool called; - size_t result; - th_err err; -} th_recorded_result; +static th_err +fake_file_ops_stat(void* self, int fd, struct stat* out) +{ + (void)fd; + fake_file_ops* ops = self; + *out = (struct stat){0}; + out->st_size = (off_t)ops->file_size; + return TH_ERR_OK; +} static void -th_recorded_result_cb(void* user_data, size_t size, th_err err) +fake_file_ops_close(void* self, int fd) { - th_recorded_result* result = user_data; - result->called = true; - result->result = size; - result->err = err; + (void)self; + (void)fd; } static void -th_recorded_result_init(th_recorded_result* result) +fake_file_ops_init(fake_file_ops* ops) { - result->called = false; - result->result = 0; - result->err = TH_ERR_OK; + ops->base.openat = fake_file_ops_openat; + ops->base.seek = fake_file_ops_seek; + ops->base.read = fake_file_ops_read; + ops->base.write = fake_file_ops_write; + ops->base.stat = fake_file_ops_stat; + ops->base.close = fake_file_ops_close; + ops->next_fd = 3; + ops->open_fails = false; + ops->file_size = 1234; } TH_TEST_BEGIN(response) { + fake_dir_ops dir_ops; + fake_dir_ops_init(&dir_ops); + th_dir dir; + th_dir_init(&dir, &dir_ops.base); + TH_EXPECT(th_dir_open(&dir, TH_STR("/")) == TH_ERR_OK); + th_dir_mgr dir_mgr; th_dir_mgr_init(&dir_mgr, th_default_allocator_get()); + TH_EXPECT(th_dir_mgr_add(&dir_mgr, TH_STR("root"), dir) == TH_ERR_OK); + + fake_file_ops file_ops; + fake_file_ops_init(&file_ops); th_fcache fcache; - th_fcache_init(&fcache, &th_unused_file_ops, th_default_allocator_get()); + th_fcache_init(&fcache, &file_ops.base, th_default_allocator_get()); + th_response response; th_response_init(&response, &dir_mgr, &fcache, th_default_allocator_get()); - th_fake_conn conn; - th_fake_conn_init(&conn); - TH_TEST_CASE_BEGIN(response_write_without_content) + TH_TEST_CASE_BEGIN(response_prepare_write_without_content) { - th_recorded_result result; - th_recorded_result_init(&result); - th_response_async_write(&response, &conn.base, th_recorded_result_cb, &result); + th_response_write_plan plan; + TH_EXPECT(th_response_prepare_write(&response, &plan) == TH_ERR_OK); - TH_EXPECT(result.called); - TH_EXPECT(result.err == TH_ERR_OK); - TH_EXPECT(conn.sent_file == false); + TH_EXPECT(plan.file == NULL); + TH_EXPECT(plan_body_is(&plan, "")); + TH_EXPECT(plan_start_line_is(&plan, "HTTP/1.1 200 OK\r\n")); + TH_EXPECT(plan_has_header_line(&plan, "Content-Length", "0")); } TH_TEST_CASE_END - TH_TEST_CASE_BEGIN(response_write_with_content) + TH_TEST_CASE_BEGIN(response_prepare_write_with_content) { th_set_body(&response, "Hello, World!"); - th_recorded_result result; - th_recorded_result_init(&result); - th_response_async_write(&response, &conn.base, th_recorded_result_cb, &result); + th_response_write_plan plan; + TH_EXPECT(th_response_prepare_write(&response, &plan) == TH_ERR_OK); - TH_EXPECT(result.called); - TH_EXPECT(result.err == TH_ERR_OK); - TH_EXPECT(th_buf_contains(conn.written, conn.written_len, "Hello, World!")); + TH_EXPECT(plan.file == NULL); + TH_EXPECT(plan_body_is(&plan, "Hello, World!")); + TH_EXPECT(plan_has_header_line(&plan, "Content-Length", "13")); } TH_TEST_CASE_END - TH_TEST_CASE_BEGIN(response_write_with_content_and_header) + TH_TEST_CASE_BEGIN(response_prepare_write_with_content_and_header) { th_set_body(&response, "Hello, World!"); th_add_header(&response, "Connection", "close"); th_add_header(&response, "Content-Type", "text/plain"); - th_recorded_result result; - th_recorded_result_init(&result); - th_response_async_write(&response, &conn.base, th_recorded_result_cb, &result); + th_response_write_plan plan; + TH_EXPECT(th_response_prepare_write(&response, &plan) == TH_ERR_OK); - TH_EXPECT(result.called); - TH_EXPECT(result.err == TH_ERR_OK); - TH_EXPECT(th_buf_contains(conn.written, conn.written_len, "Connection: close")); - TH_EXPECT(th_buf_contains(conn.written, conn.written_len, "Content-Type: text/plain")); + TH_EXPECT(plan_has_header_line(&plan, "Connection", "close")); + TH_EXPECT(plan_has_header_line(&plan, "Content-Type", "text/plain")); } TH_TEST_CASE_END TH_TEST_CASE_BEGIN(response_only_headers_skips_body) @@ -172,13 +224,189 @@ TH_TEST_BEGIN(response) th_set_body(&response, "Hello, World!"); response.only_headers = true; - th_recorded_result result; - th_recorded_result_init(&result); - th_response_async_write(&response, &conn.base, th_recorded_result_cb, &result); + th_response_write_plan plan; + TH_EXPECT(th_response_prepare_write(&response, &plan) == TH_ERR_OK); + + TH_EXPECT(plan_body_is(&plan, "")); + } + TH_TEST_CASE_END + TH_TEST_CASE_BEGIN(response_set_code) + { + th_response_set_code(&response, TH_CODE_NOT_FOUND); + + th_response_write_plan plan; + TH_EXPECT(th_response_prepare_write(&response, &plan) == TH_ERR_OK); + + TH_EXPECT(plan_start_line_is(&plan, "HTTP/1.1 404 Not Found\r\n")); + } + TH_TEST_CASE_END + TH_TEST_CASE_BEGIN(response_add_header_rejects_duplicate_known_header) + { + // Header ids are looked up case-sensitively against a lowercase + // gperf table, so the key must be lowercase to be recognized. + TH_EXPECT(th_add_header(&response, "content-type", "text/plain") == TH_ERR_OK); + TH_EXPECT(th_add_header(&response, "content-type", "text/html") != TH_ERR_OK); + } + TH_TEST_CASE_END + TH_TEST_CASE_BEGIN(response_printf_body_short) + { + TH_EXPECT(th_printf_body(&response, "%s is %d", "answer", 42) == TH_ERR_OK); + + th_response_write_plan plan; + TH_EXPECT(th_response_prepare_write(&response, &plan) == TH_ERR_OK); + + TH_EXPECT(plan_body_is(&plan, "answer is 42")); + TH_EXPECT(plan_has_header_line(&plan, "Content-Length", "12")); + } + TH_TEST_CASE_END + TH_TEST_CASE_BEGIN(response_printf_body_grows_past_stack_buffer) + { + // th_response_set_body_va's fast path uses a 512-byte stack + // buffer; force the slow (resize + reformat) path. + char long_arg[600]; + memset(long_arg, 'a', sizeof(long_arg) - 1); + long_arg[sizeof(long_arg) - 1] = '\0'; + + TH_EXPECT(th_printf_body(&response, "%s", long_arg) == TH_ERR_OK); + + th_response_write_plan plan; + TH_EXPECT(th_response_prepare_write(&response, &plan) == TH_ERR_OK); + + TH_EXPECT(plan_body_is(&plan, long_arg)); + } + TH_TEST_CASE_END + TH_TEST_CASE_BEGIN(response_set_body_from_file) + { + file_ops.file_size = 42; + TH_EXPECT(th_set_body_from_file(&response, "root", "index.html") == TH_ERR_OK); + + th_response_write_plan plan; + TH_EXPECT(th_response_prepare_write(&response, &plan) == TH_ERR_OK); + + TH_EXPECT(plan.file != NULL); + TH_EXPECT(plan.offset == 0); + TH_EXPECT(plan.len == 42); + TH_EXPECT(plan_has_header_line(&plan, "Content-Type", "text/html")); + TH_EXPECT(plan_has_header_line(&plan, "Content-Length", "42")); + + file_ops.file_size = 1234; + } + TH_TEST_CASE_END + TH_TEST_CASE_BEGIN(response_set_body_from_file_no_extension) + { + TH_EXPECT(th_set_body_from_file(&response, "root", "README") == TH_ERR_OK); + + th_response_write_plan plan; + TH_EXPECT(th_response_prepare_write(&response, &plan) == TH_ERR_OK); + + TH_EXPECT(plan_has_header_line(&plan, "Content-Type", "application/octet-stream")); + } + TH_TEST_CASE_END + TH_TEST_CASE_BEGIN(response_set_body_from_file_unknown_extension) + { + TH_EXPECT(th_set_body_from_file(&response, "root", "archive.zzz") == TH_ERR_OK); + + th_response_write_plan plan; + TH_EXPECT(th_response_prepare_write(&response, &plan) == TH_ERR_OK); + + TH_EXPECT(plan_has_header_line(&plan, "Content-Type", "application/octet-stream")); + } + TH_TEST_CASE_END + TH_TEST_CASE_BEGIN(response_set_body_from_file_unknown_root) + { + TH_EXPECT(th_set_body_from_file(&response, "does_not_exist", "index.html") != TH_ERR_OK); + } + TH_TEST_CASE_END + TH_TEST_CASE_BEGIN(response_set_body_from_file_open_fails) + { + file_ops.open_fails = true; + TH_EXPECT(th_set_body_from_file(&response, "root", "index.html") != TH_ERR_OK); + file_ops.open_fails = false; + } + TH_TEST_CASE_END + TH_TEST_CASE_BEGIN(response_set_body_from_file_keeps_explicit_content_type) + { + TH_EXPECT(th_add_header(&response, "content-type", "application/custom") == TH_ERR_OK); + TH_EXPECT(th_set_body_from_file(&response, "root", "index.html") == TH_ERR_OK); + + th_response_write_plan plan; + TH_EXPECT(th_response_prepare_write(&response, &plan) == TH_ERR_OK); + + // th_add_header preserves the caller's casing verbatim. + TH_EXPECT(plan_has_header_line(&plan, "content-type", "application/custom")); + TH_EXPECT(!plan_contains(&plan, "text/html")); + } + TH_TEST_CASE_END + TH_TEST_CASE_BEGIN(response_add_cookie_minimal) + { + TH_EXPECT(th_add_cookie(&response, "session", "abc123", NULL) == TH_ERR_OK); + + th_response_write_plan plan; + TH_EXPECT(th_response_prepare_write(&response, &plan) == TH_ERR_OK); + + TH_EXPECT(plan_has_header_line(&plan, "Set-Cookie", "session=abc123")); + } + TH_TEST_CASE_END + TH_TEST_CASE_BEGIN(response_add_cookie_with_expires) + { + th_cookie_attr attr = {0}; + // Mon, 1 Jan 2024 0:0:0 GMT + attr.expires = (th_date){.year = 124, .month = 0, .day = 1, .weekday = 1, .hour = 0, .minute = 0, .second = 0}; + + TH_EXPECT(th_add_cookie(&response, "session", "abc123", &attr) == TH_ERR_OK); + + th_response_write_plan plan; + TH_EXPECT(th_response_prepare_write(&response, &plan) == TH_ERR_OK); + + TH_EXPECT(plan_has_header_line(&plan, "Set-Cookie", "session=abc123; Expires=Mon, 1 Jan 2024 0:0:0 GMT")); + } + TH_TEST_CASE_END + TH_TEST_CASE_BEGIN(response_add_cookie_with_attributes) + { + th_cookie_attr attr = {0}; + attr.max_age = th_seconds(3600); + attr.domain = "example.com"; + attr.path = "/"; + attr.secure = true; + attr.http_only = true; + attr.same_site = TH_COOKIE_SAME_SITE_STRICT; + + TH_EXPECT(th_add_cookie(&response, "session", "abc123", &attr) == TH_ERR_OK); + + th_response_write_plan plan; + TH_EXPECT(th_response_prepare_write(&response, &plan) == TH_ERR_OK); + + TH_EXPECT(plan_has_header_line(&plan, "Set-Cookie", + "session=abc123; Max-Age=3600; Domain=example.com; " + "Path=/; Secure; HttpOnly; SameSite=Strict")); + } + TH_TEST_CASE_END + TH_TEST_CASE_BEGIN(response_add_cookie_same_site_none_requires_secure) + { + th_cookie_attr attr = {0}; + attr.same_site = TH_COOKIE_SAME_SITE_NONE; + attr.secure = false; + TH_EXPECT(th_add_cookie(&response, "session", "abc123", &attr) != TH_ERR_OK); + + attr.secure = true; + TH_EXPECT(th_add_cookie(&response, "session", "abc123", &attr) == TH_ERR_OK); + } + TH_TEST_CASE_END + TH_TEST_CASE_BEGIN(response_reset_clears_body_and_headers) + { + th_set_body(&response, "Hello, World!"); + th_add_header(&response, "Connection", "close"); + th_response_set_code(&response, TH_CODE_NOT_FOUND); + response.only_headers = true; + + th_response_reset(&response); + + th_response_write_plan plan; + TH_EXPECT(th_response_prepare_write(&response, &plan) == TH_ERR_OK); - TH_EXPECT(result.called); - TH_EXPECT(result.err == TH_ERR_OK); - TH_EXPECT(!th_buf_contains(conn.written, conn.written_len, "Hello, World!")); + TH_EXPECT(plan_start_line_is(&plan, "HTTP/1.1 200 OK\r\n")); + TH_EXPECT(!plan_has_header_line(&plan, "Connection", "close")); + TH_EXPECT(plan_body_is(&plan, "")); } TH_TEST_CASE_END From 55c666863691d5b558416cade7685d96c2a5b202 Mon Sep 17 00:00:00 2001 From: Raphael Schlarb Date: Mon, 10 Aug 2026 04:55:15 -0500 Subject: [PATCH 2/4] fix: reject over-limit connections before reading, fix body-relocation UB - too-many-connections check moved from th_http_handle_read_request to th_http_upgrader_upgrade: it was gated behind "request has a body still to read", so it silently never ran for bodyless requests (GET/HEAD/DELETE), and even when it did fire it only rejected after fully reading and parsing the request - now rejects immediately at upgrade time, before any recv - fix memcpy-param-overlap (caught by ASan) when relocating an already-buffered body to the front of the buffer before growing it; source and destination legitimately overlap here, so use memmove test: add coverage for header-too-large, body-too-large, too-many-connections, HEAD requests, and a body large enough to trigger the internal buffer growth path --- src/th_http.c | 12 +++--- src/th_http_test.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 6 deletions(-) diff --git a/src/th_http.c b/src/th_http.c index e31955a..f08f8c9 100644 --- a/src/th_http.c +++ b/src/th_http.c @@ -228,11 +228,6 @@ th_http_handle_read_request(void* user_data, size_t len, th_err err) th_conn_recv(http->conn, th_buf_vec_at(&http->buf, http->read_bytes), th_buf_vec_size(&http->buf) - http->read_bytes, false, th_http_handle_read_request, http); } else { - if (th_conn_tracker_count(http->tracker) > TH_CONFIG_MAX_CONNECTIONS) { - TH_LOG_WARN("Too many connections, rejecting new connection"); - th_http_write_error_response(http, TH_ERR_HTTP(TH_CODE_SERVICE_UNAVAILABLE)); - return; - } size_t content_received = http->read_bytes - http->parsed_bytes; size_t content_len = th_request_parser_content_len(&http->parser); if (content_len > TH_MAX_BODY_LEN) { @@ -242,7 +237,7 @@ th_http_handle_read_request(void* user_data, size_t len, th_err err) } size_t remaining = content_len - content_received; if (http->read_bytes + remaining > th_buf_vec_size(&http->buf)) { - memcpy(th_buf_vec_at(&http->buf, 0), th_buf_vec_at(&http->buf, http->parsed_bytes), content_received); + memmove(th_buf_vec_at(&http->buf, 0), th_buf_vec_at(&http->buf, http->parsed_bytes), content_received); http->read_bytes = content_received; http->parsed_bytes = 0; if (content_len > th_buf_vec_size(&http->buf)) { @@ -320,6 +315,11 @@ th_http_upgrader_upgrade(void* self, th_conn* conn) th_conn_destroy(conn); return; } + if (th_conn_tracker_count(upgrader->tracker) > TH_CONFIG_MAX_CONNECTIONS) { + TH_LOG_WARN("Too many connections, rejecting new connection"); + th_http_handle_error(http, TH_ERR_HTTP(TH_CODE_SERVICE_UNAVAILABLE)); + return; + } th_http_start(http); } diff --git a/src/th_http_test.c b/src/th_http_test.c index ef4ab21..cd2fea0 100644 --- a/src/th_http_test.c +++ b/src/th_http_test.c @@ -1,4 +1,5 @@ #include "th_conn_tracker.h" +#include "th_fmt.h" #include "th_http.h" #include "th_test.h" #include "th_utility.h" @@ -289,6 +290,97 @@ TH_TEST_BEGIN(http) TH_EXPECT(conn.destroyed); } TH_TEST_CASE_END + TH_TEST_CASE_BEGIN(http_rejects_header_too_large) + { + // Header never terminates and keeps growing past + // TH_CONFIG_LARGE_HEADER_LEN, so it's rejected outright rather + // than resized indefinitely. + char request[TH_CONFIG_LARGE_HEADER_LEN + 256]; + size_t pos = 0; + pos += th_fmt_str_append(request, pos, sizeof(request), "GET /test HTTP/1.1\r\n"); + while (pos + 32 < sizeof(request)) { + pos += th_fmt_str_append(request, pos, sizeof(request), "X-Pad: aaaaaaaaaaaaaaaaaaaaaaaa\r\n"); + } + th_fake_conn_set_request(&conn, th_str_make(request, pos)); + + th_conn_upgrader_upgrade(&upgrader.base, &conn.base); + while (!conn.destroyed && conn.callback != NULL) + th_fake_conn_run(&conn); + + TH_EXPECT(th_buf_starts_with(conn.written, conn.written_len, "HTTP/1.1 431 Request Header Fields Too Large\r\n")); + TH_EXPECT(conn.destroyed); + } + TH_TEST_CASE_END + TH_TEST_CASE_BEGIN(http_rejects_body_too_large) + { + char request[256]; + size_t pos = 0; + pos += th_fmt_str_append(request, pos, sizeof(request), "POST /test HTTP/1.1\r\nHost: example.com\r\nConnection: close\r\nContent-Length: "); + char content_len[32]; + pos += th_fmt_str_append(request, pos, sizeof(request), th_fmt_uint_to_str(content_len, sizeof(content_len), TH_MAX_BODY_LEN + 1)); + pos += th_fmt_str_append(request, pos, sizeof(request), "\r\n\r\n"); + th_fake_conn_set_request(&conn, th_str_make(request, pos)); + + th_conn_upgrader_upgrade(&upgrader.base, &conn.base); + while (!conn.destroyed && conn.callback != NULL) + th_fake_conn_run(&conn); + + TH_EXPECT(th_buf_starts_with(conn.written, conn.written_len, "HTTP/1.1 413 Payload Too Large\r\n")); + TH_EXPECT(conn.destroyed); + } + TH_TEST_CASE_END + TH_TEST_CASE_BEGIN(http_accepts_large_body_growing_internal_buffer) + { + // Body alone is bigger than TH_CONFIG_SMALL_HEADER_LEN (the + // initial buffer size), but well within TH_MAX_BODY_LEN, so the + // request is accepted and th_buf_vec_resize's growth path runs. + size_t body_len = TH_CONFIG_SMALL_HEADER_LEN + 1000; + char request[TH_CONFIG_SMALL_HEADER_LEN + 1200]; + size_t pos = 0; + pos += th_fmt_str_append(request, pos, sizeof(request), "POST /test HTTP/1.1\r\nHost: example.com\r\nConnection: close\r\nContent-Length: "); + char content_len[32]; + pos += th_fmt_str_append(request, pos, sizeof(request), th_fmt_uint_to_str(content_len, sizeof(content_len), (unsigned int)body_len)); + pos += th_fmt_str_append(request, pos, sizeof(request), "\r\n\r\n"); + for (size_t i = 0; i < body_len; ++i) + request[pos++] = 'a'; + th_fake_conn_set_request(&conn, th_str_make(request, pos)); + + th_conn_upgrader_upgrade(&upgrader.base, &conn.base); + while (!conn.destroyed && conn.callback != NULL) + th_fake_conn_run(&conn); + + TH_EXPECT(th_buf_starts_with(conn.written, conn.written_len, "HTTP/1.1 200 OK\r\n")); + TH_EXPECT(th_buf_ends_with(conn.written, conn.written_len, "Hello, World!")); + TH_EXPECT(conn.destroyed); + } + TH_TEST_CASE_END + TH_TEST_CASE_BEGIN(http_rejects_too_many_connections) + { + // Rejected outright at upgrade time, before any request is read. + tracker.count = TH_CONFIG_MAX_CONNECTIONS + 1; + + th_conn_upgrader_upgrade(&upgrader.base, &conn.base); + while (!conn.destroyed && conn.callback != NULL) + th_fake_conn_run(&conn); + + TH_EXPECT(th_buf_starts_with(conn.written, conn.written_len, "HTTP/1.1 503 Service Unavailable\r\n")); + TH_EXPECT(conn.destroyed); + } + TH_TEST_CASE_END + TH_TEST_CASE_BEGIN(http_head_request_writes_headers_without_body) + { + th_fake_conn_set_request(&conn, TH_STR("HEAD /test HTTP/1.1\r\nHost: example.com\r\nConnection: close\r\n\r\n")); + + th_conn_upgrader_upgrade(&upgrader.base, &conn.base); + while (!conn.destroyed && conn.callback != NULL) + th_fake_conn_run(&conn); + + TH_EXPECT(th_buf_starts_with(conn.written, conn.written_len, "HTTP/1.1 200 OK\r\n")); + TH_EXPECT(th_buf_has_header(conn.written, conn.written_len, "Content-Length", "13")); // matches GET's body length + TH_EXPECT(!th_buf_ends_with(conn.written, conn.written_len, "Hello, World!")); // but body itself is omitted + TH_EXPECT(conn.destroyed); + } + TH_TEST_CASE_END TH_TEST_CASE_BEGIN(http_handles_options_for_known_route) { th_fake_conn_set_request(&conn, TH_STR("OPTIONS /test HTTP/1.1\r\nHost: example.com\r\nConnection: close\r\n\r\n")); From 0e015cf6639bfffebf840222ea26842f2f84fc72 Mon Sep 17 00:00:00 2001 From: Raphael Schlarb Date: Mon, 10 Aug 2026 04:58:49 -0500 Subject: [PATCH 3/4] build: exclude th_bench.h from coverage report It's a header, not a .c file, so the existing _bench.c exclude pattern doesn't catch it. --- CMakeLists.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index b9417da..bae587b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -289,6 +289,7 @@ if (NOT TH_DISABLE_TESTS) --filter ${CMAKE_CURRENT_SOURCE_DIR}/src/ --exclude .*_test\\.c$$ --exclude .*_bench\\.c$$ + --exclude .*/th_bench\\.h$$ ${TH_COVERAGE_GPERF_EXCLUDES} --object-directory ${CMAKE_CURRENT_BINARY_DIR} --merge-mode-functions separate From 2898fa93a8e7bfd79723012303c7216ec37d9ffc Mon Sep 17 00:00:00 2001 From: Raphael Schlarb Date: Mon, 10 Aug 2026 05:10:29 -0500 Subject: [PATCH 4/4] chore: update amalgamation --- th.c | 68 +++++++++++++++++++++++++++++++++++++++++------------------- 1 file changed, 47 insertions(+), 21 deletions(-) diff --git a/th.c b/th.c index 890eee6..da78ce1 100644 --- a/th.c +++ b/th.c @@ -3054,8 +3054,26 @@ th_response_deinit(th_response* response); /* th_response end */ -TH_PRIVATE(void) -th_response_async_write(th_response* response, th_conn* conn, th_send_cb callback, void* user_data); +/** th_response_write_plan + * @brief What to send for a response: iov always (start line + headers, + * plus body if any), file/offset/len additionally if a file is being + * sent (file is NULL otherwise). + */ +typedef struct th_response_write_plan { + th_iov* iov; + size_t iovcnt; + th_file* file; + size_t offset; + size_t len; +} th_response_write_plan; + +/** th_response_prepare_write + * @brief Finalizes headers (default headers, start line) and fills out + * plan with what to send. Does no I/O - the caller sends plan itself + * (e.g. via th_conn_send). + */ +TH_PRIVATE(th_err) +th_response_prepare_write(th_response* response, th_response_write_plan* plan); /* End of th_response.h */ /* Start of th_router.h */ @@ -7786,7 +7804,7 @@ th_response_set_body_va(th_response* response, const char* fmt, va_list args) } } else { th_string_resize(&response->body, (size_t)len, ' '); - vsnprintf(th_string_at(&response->body, 0), (size_t)len, fmt, args); + vsnprintf(th_string_at(&response->body, 0), (size_t)len + 1, fmt, args); } response->is_file = 0; return TH_ERR_OK; @@ -7849,8 +7867,8 @@ th_response_set_default_headers(th_response* response) return TH_ERR_OK; } -TH_PRIVATE(void) -th_response_async_write(th_response* response, th_conn* conn, th_send_cb callback, void* user_data) +TH_PRIVATE(th_err) +th_response_prepare_write(th_response* response, th_response_write_plan* plan) { th_err err = TH_ERR_OK; size_t iovcnt = 2; // start line + headers @@ -7858,24 +7876,26 @@ th_response_async_write(th_response* response, th_conn* conn, th_send_cb callbac response->file_len = response->fcache_entry->stream.size; } if ((err = th_response_set_default_headers(response)) != TH_ERR_OK) - goto cleanup; + return err; if ((err = th_response_finalize_headers(response)) != TH_ERR_OK) - goto cleanup; + return err; if (!response->only_headers && response->is_file == 0 && th_string_len(&response->body) > 0) { response->iov[iovcnt].base = (void*)th_string_data(&response->body); response->iov[iovcnt].len = th_string_len(&response->body); iovcnt++; } + plan->iov = response->iov; + plan->iovcnt = iovcnt; if (!response->only_headers && response->is_file != 0) { - th_conn_send(conn, response->iov, iovcnt, &response->fcache_entry->stream, 0, (size_t)response->file_len, callback, user_data); + plan->file = &response->fcache_entry->stream; + plan->offset = 0; + plan->len = (size_t)response->file_len; } else { - th_conn_send(conn, response->iov, iovcnt, NULL, 0, 0, callback, user_data); + plan->file = NULL; + plan->offset = 0; + plan->len = 0; } - return; -cleanup: - // Header formatting failed before any I/O was attempted (out of - // memory); safe to call back synchronously since no op is pending. - callback(user_data, 0, err); + return TH_ERR_OK; } /* Public response API begin */ @@ -9188,7 +9208,13 @@ th_http_complete(th_http* http) TH_LOCAL(void) th_http_write_response(th_http* http) { - th_response_async_write(&http->response, http->conn, th_http_handle_write_response, http); + th_response_write_plan plan; + th_err err = th_response_prepare_write(&http->response, &plan); + if (err != TH_ERR_OK) { + th_http_handle_write_response(http, 0, err); + return; + } + th_conn_send(http->conn, plan.iov, plan.iovcnt, plan.file, plan.offset, plan.len, th_http_handle_write_response, http); } TH_LOCAL(void) @@ -9358,11 +9384,6 @@ th_http_handle_read_request(void* user_data, size_t len, th_err err) th_conn_recv(http->conn, th_buf_vec_at(&http->buf, http->read_bytes), th_buf_vec_size(&http->buf) - http->read_bytes, false, th_http_handle_read_request, http); } else { - if (th_conn_tracker_count(http->tracker) > TH_CONFIG_MAX_CONNECTIONS) { - TH_LOG_WARN("Too many connections, rejecting new connection"); - th_http_write_error_response(http, TH_ERR_HTTP(TH_CODE_SERVICE_UNAVAILABLE)); - return; - } size_t content_received = http->read_bytes - http->parsed_bytes; size_t content_len = th_request_parser_content_len(&http->parser); if (content_len > TH_MAX_BODY_LEN) { @@ -9372,7 +9393,7 @@ th_http_handle_read_request(void* user_data, size_t len, th_err err) } size_t remaining = content_len - content_received; if (http->read_bytes + remaining > th_buf_vec_size(&http->buf)) { - memcpy(th_buf_vec_at(&http->buf, 0), th_buf_vec_at(&http->buf, http->parsed_bytes), content_received); + memmove(th_buf_vec_at(&http->buf, 0), th_buf_vec_at(&http->buf, http->parsed_bytes), content_received); http->read_bytes = content_received; http->parsed_bytes = 0; if (content_len > th_buf_vec_size(&http->buf)) { @@ -9450,6 +9471,11 @@ th_http_upgrader_upgrade(void* self, th_conn* conn) th_conn_destroy(conn); return; } + if (th_conn_tracker_count(upgrader->tracker) > TH_CONFIG_MAX_CONNECTIONS) { + TH_LOG_WARN("Too many connections, rejecting new connection"); + th_http_handle_error(http, TH_ERR_HTTP(TH_CODE_SERVICE_UNAVAILABLE)); + return; + } th_http_start(http); }