From a6df8876933c150cdb399687ed2a83912f217bfe Mon Sep 17 00:00:00 2001 From: mattsva Date: Fri, 9 Oct 2026 02:57:02 +0200 Subject: [PATCH] Fix localtime() data race in the welcome-page handler welcome_response() runs on worker threads and called localtime(), which is not reentrant: it mutates a static struct tm shared process-wide. Confirmed under ThreadSanitizer (data race via tzset_internal). Replaced with localtime_r() on POSIX and localtime_s() on Windows. Regression test: test/test_reentrancy.c runs 32 concurrent welcome-page requests; it reported the race under TSan before the fix and is clean after. Wired into Make, CMake, Meson and CI. The new test binds port 18780, not 18779: meson runs its tests in parallel and test_security already binds 18779, so the two collide. --- .github/workflows/make_test.yml | 10 +- CMakeLists.txt | 5 + Makefile | 8 +- meson.build | 8 ++ src/mnet_app.c | 17 ++- test/test_reentrancy.c | 202 ++++++++++++++++++++++++++++++++ 6 files changed, 242 insertions(+), 8 deletions(-) create mode 100644 test/test_reentrancy.c diff --git a/.github/workflows/make_test.yml b/.github/workflows/make_test.yml index 73952aa..c9648c4 100644 --- a/.github/workflows/make_test.yml +++ b/.github/workflows/make_test.yml @@ -84,7 +84,7 @@ jobs: CFLAGS="-std=c17 -Wall -Wextra -Wpedantic -Werror -g \ -fsanitize=address,undefined -fno-omit-frame-pointer \ -D_POSIX_C_SOURCE=200112L -Iinclude -Iinclude/mnet -Isrc -pthread" - for t in test_mnet test_http test_mnet_parser test_stress test_features test_security test_client test_welcome test_starvation; do + for t in test_mnet test_http test_mnet_parser test_stress test_features test_security test_client test_welcome test_starvation test_reentrancy; do gcc $CFLAGS src/mnet.c src/mnet_app.c src/mnet_response.c \ src/mnet_request.c src/mnet_router.c test/$t.c -o ${t}_asan || exit 1 done @@ -116,6 +116,9 @@ jobs: - name: Run starvation tests under ASan/UBSan run: ASAN_OPTIONS=detect_leaks=0 ./test_starvation_asan + - name: Run reentrancy tests under ASan/UBSan + run: ASAN_OPTIONS=detect_leaks=0 ./test_reentrancy_asan + test-valgrind: runs-on: ubuntu-latest steps: @@ -126,7 +129,7 @@ jobs: run: sudo apt-get update && sudo apt-get install -y valgrind - name: Build tests - run: make test_mnet test_http test_parser test_stress test_features test_security test_client test_welcome test_starvation + run: make test_mnet test_http test_parser test_stress test_features test_security test_client test_welcome test_starvation test_reentrancy - name: Valgrind unit tests run: valgrind --leak-check=full --error-exitcode=1 ./test_mnet @@ -149,6 +152,9 @@ jobs: - name: Valgrind starvation tests run: valgrind --leak-check=full --error-exitcode=1 --trace-children=yes ./test_starvation + - name: Valgrind reentrancy tests + run: valgrind --leak-check=full --error-exitcode=1 --trace-children=yes ./test_reentrancy + # The worker pool is the one part of the library that is only exercised with # threads, so run the concurrency tests under ThreadSanitizer too. test-tsan: diff --git a/CMakeLists.txt b/CMakeLists.txt index 25a5d58..3571685 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -126,6 +126,11 @@ target_link_libraries(test_starvation PRIVATE mnet_static) target_include_directories(test_starvation PRIVATE include src test) add_test(NAME mnet_starvation_tests COMMAND test_starvation) +add_executable(test_reentrancy test/test_reentrancy.c) +target_link_libraries(test_reentrancy PRIVATE mnet_static) +target_include_directories(test_reentrancy PRIVATE include src test) +add_test(NAME mnet_reentrancy_tests COMMAND test_reentrancy) + # Install install(TARGETS mnet_static mnet_shared ARCHIVE DESTINATION lib diff --git a/Makefile b/Makefile index 3ec0b38..d41ebb7 100644 --- a/Makefile +++ b/Makefile @@ -38,10 +38,11 @@ SRCS = src/mnet.c src/mnet_app.c src/mnet_response.c \ TEST_SRCS = test/test_mnet.c test/test_http.c test/test_mnet_parser.c \ test/test_stress.c test/test_features.c test/test_security.c \ - test/test_client.c test/test_welcome.c test/test_starvation.c + test/test_client.c test/test_welcome.c test/test_starvation.c \ + test/test_reentrancy.c TEST_BINS = test_mnet test_http test_parser test_stress test_features \ - test_security test_client test_welcome test_starvation + test_security test_client test_welcome test_starvation test_reentrancy # Default target: build everything all: lib libmnet.a libmnet.so examples @@ -156,6 +157,9 @@ test_welcome: $(SRCS) test/test_welcome.c test_starvation: $(SRCS) test/test_starvation.c $(CC) $(CFLAGS) -Isrc $(SRCS) test/test_starvation.c -o $@ $(LDFLAGS) +test_reentrancy: $(SRCS) test/test_reentrancy.c + $(CC) $(CFLAGS) -Isrc $(SRCS) test/test_reentrancy.c -o $@ $(LDFLAGS) + # ---- Run the examples (build + run) ---- # Usage: make run EXAMPLE=example_http_server PORT=8080 # EXAMPLE Example binary to build and run (default: example_http_server) diff --git a/meson.build b/meson.build index 5b7dc36..add42e5 100644 --- a/meson.build +++ b/meson.build @@ -148,6 +148,14 @@ test_starvation = executable('test_starvation', test('mnet_starvation_tests', test_starvation) +test_reentrancy = executable('test_reentrancy', + 'test/test_reentrancy.c', + link_with: mnet_static, + include_directories: [mnet_inc, include_directories('src'), include_directories('test')], +) + +test('mnet_reentrancy_tests', test_reentrancy) + # Install headers (libraries are installed by default) install_headers('include/mnet/mnet.h', 'include/mnet/mnet_app.h', 'include/mnet/mnet_request.h', 'include/mnet/mnet_response.h', 'include/mnet/mnet_router.h', 'include/mnet/mnet_socket.h', diff --git a/src/mnet_app.c b/src/mnet_app.c index 1353c60..8a71b9f 100644 --- a/src/mnet_app.c +++ b/src/mnet_app.c @@ -1132,11 +1132,20 @@ static mnet_response_t welcome_response(mnet_app_t *app) return mnet_error(500, "internal server error"); } - /* Get current time */ + /* Get current time. localtime() returns a pointer to a process-wide + static struct tm, which is not safe to use from several worker + threads at once; use the reentrant variant instead. */ time_t now = time(NULL); - struct tm *tm = localtime(&now); - char time_str[64]; - strftime(time_str, sizeof(time_str), "%Y-%m-%d %H:%M:%S %Z", tm); + char time_str[64] = "unknown"; + struct tm tmbuf; +#ifdef _WIN32 + if (localtime_s(&tmbuf, &now) == 0) +#else + if (localtime_r(&now, &tmbuf) != NULL) +#endif + { + strftime(time_str, sizeof(time_str), "%Y-%m-%d %H:%M:%S %Z", &tmbuf); + } /* Get git commit hash */ char commit_hash[13] = "unknown"; diff --git a/test/test_reentrancy.c b/test/test_reentrancy.c new file mode 100644 index 0000000..4939bfd --- /dev/null +++ b/test/test_reentrancy.c @@ -0,0 +1,202 @@ +/* test/test_reentrancy.c + * + * Concurrency test for the dev-mode welcome page. + * + * The welcome-page handler formats the current time. It runs on worker + * threads, so the time conversion must be reentrant: localtime() returns + * a pointer to a process-wide static struct tm and corrupts under + * concurrent use. This test drives the welcome page from many clients at + * once and requires every response to carry a well-formed timestamp. + * + * Build (POSIX): + * gcc -std=c17 -Wall -Wextra -Wpedantic -Werror -D_POSIX_C_SOURCE=200112L + * -Iinclude -Isrc -pthread src/mnet.c src/mnet_app.c src/mnet_response.c + * src/mnet_request.c src/mnet_router.c test/test_reentrancy.c -o test_reentrancy + * + * The socket parts are POSIX-only; on Windows this compiles to a no-op. + */ +#define _GNU_SOURCE +#include +#include + +#include +#include +#include + +#ifndef _WIN32 + +#include +#include +#include +#include +#include +#include +#include +#include + +#define REENTRANCY_PORT 18780 +#define REENTRANCY_CLIENTS 32 + +static int failures = 0; +static int passed = 0; + +#define CHECK(cond, name) \ + do { \ + if (cond) { \ + passed++; \ + printf(" PASS %s\n", (name)); \ + } else { \ + failures++; \ + printf(" FAIL %s (line %d)\n", (name), __LINE__); \ + } \ + } while (0) + +typedef struct { + int port; + int bad_responses; +} client_arg_t; + +static int connect_to(int port) +{ + int fd = socket(AF_INET, SOCK_STREAM, 0); + struct sockaddr_in a; + struct timeval tv; + + if (fd < 0) return -1; + + tv.tv_sec = 30; + tv.tv_usec = 0; + setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)); + + memset(&a, 0, sizeof(a)); + a.sin_family = AF_INET; + a.sin_port = htons((uint16_t)port); + a.sin_addr.s_addr = inet_addr("127.0.0.1"); + if (connect(fd, (struct sockaddr *)&a, sizeof(a)) != 0) { + close(fd); + return -1; + } + return fd; +} + +/* True when s matches "YYYY-MM-DD HH:MM:SS" exactly. */ +static int timestamp_well_formed(const char *s) +{ + static const char *pattern = "dddd-dd-dd dd:dd:dd"; + for (const char *p = pattern; *p; p++, s++) { + if (*p == 'd') { + if (*s < '0' || *s > '9') return 0; + } else if (*s != *p) { + return 0; + } + } + return 1; +} + +static void *client_thread(void *arg) +{ + client_arg_t *ca = (client_arg_t *)arg; + int fd = connect_to(ca->port); + + if (fd < 0) { + ca->bad_responses++; + return NULL; + } + + const char *req = + "GET / HTTP/1.1\r\n" + "Host: localhost\r\n" + "\r\n"; + if (send(fd, req, strlen(req), 0) < 0) { + ca->bad_responses++; + close(fd); + return NULL; + } + + char reply[8192]; + size_t used = 0; + for (;;) { + ssize_t n = recv(fd, reply + used, sizeof(reply) - 1 - used, 0); + if (n <= 0) break; + used += (size_t)n; + reply[used] = '\0'; + if (strstr(reply, "") != NULL) break; + if (used >= sizeof(reply) - 1) break; + } + close(fd); + + const char *marker = "Build time: "; + const char *ts = strstr(reply, marker); + if (ts == NULL) { + ca->bad_responses++; + return NULL; + } + ts += strlen(marker); + if (!timestamp_well_formed(ts)) { + ca->bad_responses++; + } + return NULL; +} + +int main(void) +{ + printf("Running welcome-page reentrancy tests...\n"); + + pid_t pid = fork(); + if (pid < 0) { + printf(" SKIP (fork failed)\n"); + return 0; + } + if (pid == 0) { + mnet_app_t *app = mnet_create(); + if (app == NULL) _exit(1); + mnet_set_dev_mode(app, 1); + mnet_run(app, REENTRANCY_PORT); + mnet_destroy(app); + _exit(0); + } + usleep(300000); + + pthread_t threads[REENTRANCY_CLIENTS]; + client_arg_t args[REENTRANCY_CLIENTS]; + int created[REENTRANCY_CLIENTS]; + int started = 0; + + for (int i = 0; i < REENTRANCY_CLIENTS; i++) { + args[i].port = REENTRANCY_PORT; + args[i].bad_responses = 0; + created[i] = 0; + if (pthread_create(&threads[i], NULL, client_thread, &args[i]) == 0) { + created[i] = 1; + started++; + } + } + + int bad = 0; + for (int i = 0; i < REENTRANCY_CLIENTS; i++) { + if (created[i]) pthread_join(threads[i], NULL); + } + for (int i = 0; i < started; i++) bad += args[i].bad_responses; + + CHECK(started == REENTRANCY_CLIENTS, + "all concurrent clients ran"); + CHECK(bad == 0, + "every concurrent welcome-page response has a well-formed " + "timestamp (localtime is reentrant)"); + + kill(pid, SIGTERM); + waitpid(pid, NULL, 0); + + printf("\n%d passed, %d failed\n", passed, failures); + return failures == 0 ? 0 : 1; +} + +#else + +int main(void) +{ + printf("Reentrancy tests are not run on Windows.\n"); + return 0; +} + +#endif