diff --git a/.gitignore b/.gitignore index dd55ee93bb..61433d248d 100644 --- a/.gitignore +++ b/.gitignore @@ -240,3 +240,5 @@ deps/protobuf/protobuf-*/ # symlink at runtime under test-scripts/deps/; never commit this one (it was # accidentally committed once with an absolute /home path). test/scripts/deps/mysqlbinlog + +# Temporary ASAN CI end-to-end validation trigger; this branch will not merge. diff --git a/docs/ci-asan-e2e-probe.md b/docs/ci-asan-e2e-probe.md new file mode 100644 index 0000000000..7eba4d260a --- /dev/null +++ b/docs/ci-asan-e2e-probe.md @@ -0,0 +1,5 @@ +# Temporary CI ASAN validation probe + +This disposable file opens a label-controlled CI validation pull request. +Markdown-only changes are ignored by `CI-trigger`; a subsequent non-ignored +no-op change will start the selected run after the `ci:asan` label is applied. diff --git a/docs/superpowers/plans/2026-08-15-pgsql-poisoned-query-uaf.md b/docs/superpowers/plans/2026-08-15-pgsql-poisoned-query-uaf.md new file mode 100644 index 0000000000..929aec9335 --- /dev/null +++ b/docs/superpowers/plans/2026-08-15-pgsql-poisoned-query-uaf.md @@ -0,0 +1,19 @@ +# PgSQL Poisoned-Query ASAN UAF Implementation Plan + +**Goal:** Keep the incoming simple-query packet alive until request logging and +parser cleanup finish, then verify the complete ASAN TAP fan-out on PR #6083. + +**Design:** `CurrentQuery` borrows its SQL pointer from the packet handled by +`handler_poisoned_simple_query()`. Preserve the existing ownership model and +move `RequestEnd()` before `l_free()` in both exits; do not add a copy or change +mirror-session behavior. + +## Implementation + +1. Use the existing failing ASAN run of + `pgsql-retry_guard_in_txn_on_broken_backend-t` as the regression's red state. +2. Reorder finalization and packet release in both malformed and normal exits. +3. Run formatting/diff checks and focused source checks, then commit and push + `ci/verify-asan-label` so the label-selected ASAN workflow reruns. +4. Inspect all prior failed PR #6083 jobs, group failures by sanitizer signature, + and compare them against the fresh run before making any additional fix. diff --git a/include/proxysql_admin.h b/include/proxysql_admin.h index d642b2756e..9d7c68ea69 100644 --- a/include/proxysql_admin.h +++ b/include/proxysql_admin.h @@ -334,6 +334,7 @@ class ProxySQL_Admin { int main_poll_nfds; struct pollfd *main_poll_fds; int *main_callback_func; + bool admin_threads_shutdown; bool registered_prometheus_collectable; @@ -683,6 +684,7 @@ class ProxySQL_Admin { */ void save_mysql_servers_runtime_to_database(bool _runtime); void admin_shutdown(); + void shutdown_threads(); bool is_command(std::string); template diff --git a/lib/PgSQL_Session.cpp b/lib/PgSQL_Session.cpp index 4e102bda26..e4ad4932bf 100644 --- a/lib/PgSQL_Session.cpp +++ b/lib/PgSQL_Session.cpp @@ -478,8 +478,9 @@ bool PgSQL_Session::handler_poisoned_simple_query(PtrSize_t* pkt) { auto buff = pgpkt.detach(); client_myds->PSarrayOUT->add((void*)buff.first, buff.second); client_myds->DSS = STATE_SLEEP; - l_free(pkt->size, pkt->ptr); + // CurrentQuery borrows pkt->ptr; finish logging/parser cleanup first. if (mirror == false) RequestEnd(NULL, false); + l_free(pkt->size, pkt->ptr); return true; } @@ -605,8 +606,9 @@ bool PgSQL_Session::handler_poisoned_simple_query(PtrSize_t* pkt) { auto buff = pgpkt.detach(); client_myds->PSarrayOUT->add((void*)buff.first, buff.second); client_myds->DSS = STATE_SLEEP; - l_free(pkt->size, pkt->ptr); + // CurrentQuery borrows pkt->ptr; finish logging/parser cleanup first. if (mirror == false) RequestEnd(NULL, false); + l_free(pkt->size, pkt->ptr); return true; } diff --git a/lib/ProxySQL_Admin.cpp b/lib/ProxySQL_Admin.cpp index b7a9ce5562..48e127abcf 100644 --- a/lib/ProxySQL_Admin.cpp +++ b/lib/ProxySQL_Admin.cpp @@ -312,6 +312,32 @@ int admin_load_main_=0; bool admin_nostart_=false; static volatile int admin_client_threads_active = 0; +class Admin_Client_Thread_Guard { +public: + explicit Admin_Client_Thread_Guard(size_t stack_size) : stack_size_(stack_size) { + __sync_fetch_and_add(&admin_client_threads_active, 1); + __sync_fetch_and_add(&GloVars.statuses.stack_memory_admin_threads, stack_size_); + } + + ~Admin_Client_Thread_Guard() { + __sync_fetch_and_sub(&GloVars.statuses.stack_memory_admin_threads, stack_size_); + __sync_fetch_and_sub(&admin_client_threads_active, 1); + } + + Admin_Client_Thread_Guard(const Admin_Client_Thread_Guard&) = delete; + Admin_Client_Thread_Guard& operator=(const Admin_Client_Thread_Guard&) = delete; + +private: + size_t stack_size_; +}; + +static void close_pending_admin_client(arg_proxysql_adm* client_arg) { + ::shutdown(client_arg->client_t, SHUT_RDWR); + close(client_arg->client_t); + free(client_arg->addr); + free(client_arg); +} + int __admin_refresh_interval=0; bool admin_proxysql_mysql_paused = false; @@ -2156,16 +2182,13 @@ void ProxySQL_Admin::vacuum_stats(bool is_admin) { void *child_mysql(void *arg) { - if (GloMTH == nullptr) { return NULL; } - pthread_attr_t thread_attr; size_t tmp_stack_size=0; if (!pthread_attr_init(&thread_attr)) { - if (!pthread_attr_getstacksize(&thread_attr , &tmp_stack_size )) { - __sync_fetch_and_add(&GloVars.statuses.stack_memory_admin_threads,tmp_stack_size); - } + pthread_attr_getstacksize(&thread_attr, &tmp_stack_size); + pthread_attr_destroy(&thread_attr); } - __sync_fetch_and_add(&admin_client_threads_active, 1); + Admin_Client_Thread_Guard thread_guard(tmp_stack_size); arg_proxysql_adm*myarg = (arg_proxysql_adm*)arg; int client = myarg->client_t; @@ -2173,6 +2196,18 @@ void *child_mysql(void *arg) { //struct sockaddr *addr = arg->addr; //socklen_t addr_size; + struct pollfd fds[1]; + nfds_t nfds=1; + int rc; + // The acceptor holds this mutex until the child has taken ownership of + // its argument. Every exit below must happen after releasing it. + pthread_mutex_unlock(&sock_mutex); + + if (__sync_fetch_and_add(&glovars.shutdown, 0) != 0 || !wait_for_glo_mth() || !GloMTH) { + close_pending_admin_client(myarg); + return NULL; + } + GloMTH->wrlock(); { char *s=GloMTH->get_variable((char *)"server_capabilities"); @@ -2181,13 +2216,6 @@ void *child_mysql(void *arg) { } GloMTH->wrunlock(); - struct pollfd fds[1]; - nfds_t nfds=1; - int rc; - pthread_mutex_unlock(&sock_mutex); - // Wait for GloMTH to be initialized - if (!wait_for_glo_mth()) return NULL; // quick exit during shutdown/restart - if (!GloMTH) return NULL; MySQL_Thread *mysql_thr=new MySQL_Thread(); mysql_thr->curtime=monotonic_time(); GloMyQPro->init_thread(); @@ -2277,23 +2305,17 @@ void *child_mysql(void *arg) { __exit_child_mysql: delete mysql_thr; - __sync_fetch_and_sub(&admin_client_threads_active, 1); - __sync_fetch_and_sub(&GloVars.statuses.stack_memory_admin_threads,tmp_stack_size); - return NULL; } void* child_postgres(void* arg) { - if (GloPTH == nullptr) { return NULL; } - pthread_attr_t thread_attr; size_t tmp_stack_size = 0; if (!pthread_attr_init(&thread_attr)) { - if (!pthread_attr_getstacksize(&thread_attr, &tmp_stack_size)) { - __sync_fetch_and_add(&GloVars.statuses.stack_memory_admin_threads, tmp_stack_size); - } + pthread_attr_getstacksize(&thread_attr, &tmp_stack_size); + pthread_attr_destroy(&thread_attr); } - __sync_fetch_and_add(&admin_client_threads_active, 1); + Admin_Client_Thread_Guard thread_guard(tmp_stack_size); arg_proxysql_adm* myarg = (arg_proxysql_adm*)arg; int client = myarg->client_t; @@ -2302,6 +2324,10 @@ void* child_postgres(void* arg) { nfds_t nfds = 1; int rc; pthread_mutex_unlock(&sock_mutex); + if (__sync_fetch_and_add(&glovars.shutdown, 0) != 0 || GloPTH == nullptr) { + close_pending_admin_client(myarg); + return NULL; + } PgSQL_Thread* pgsql_thr = new PgSQL_Thread(); pgsql_thr->curtime = monotonic_time(); GloPgQPro->init_thread(); @@ -2399,9 +2425,6 @@ void* child_postgres(void* arg) { __exit_child_postgres: delete pgsql_thr; - __sync_fetch_and_sub(&admin_client_threads_active, 1); - __sync_fetch_and_sub(&GloVars.statuses.stack_memory_admin_threads, tmp_stack_size); - return NULL; } @@ -2816,6 +2839,7 @@ void update_modules_metrics() { ProxySQL_Admin::ProxySQL_Admin() : serial_exposer(std::function { update_modules_metrics }) { + admin_threads_shutdown = false; #ifdef DEBUG debugdb_disk = NULL; if (glovars.has_debug==false) { @@ -3205,9 +3229,12 @@ void ProxySQL_Admin::flush_tsdb_variables___runtime_to_database(SQLite3DB *db, b } #endif -void ProxySQL_Admin::admin_shutdown() { - int i; -// do { usleep(50); } while (main_shutdown==0); +void ProxySQL_Admin::shutdown_threads() { + if (admin_threads_shutdown) { + return; + } + admin_threads_shutdown = true; + if (Admin_HTTP_Server) { if (variables.web_enabled) { MHD_stop_daemon(Admin_HTTP_Server); @@ -3224,6 +3251,12 @@ void ProxySQL_Admin::admin_shutdown() { while (__sync_fetch_and_add(&admin_client_threads_active, 0) != 0) { usleep(1000); } +} + +void ProxySQL_Admin::admin_shutdown() { + int i; +// do { usleep(50); } while (main_shutdown==0); + shutdown_threads(); delete admindb; delete statsdb; delete configdb; diff --git a/lib/proxysql_utils.cpp b/lib/proxysql_utils.cpp index c27defdba2..c68d754c59 100644 --- a/lib/proxysql_utils.cpp +++ b/lib/proxysql_utils.cpp @@ -264,6 +264,14 @@ int wexecvp( if (opts.waitpid_delay_us != 0) to_opts.waitpid_delay_us = opts.waitpid_delay_us; if (opts.sigkill_to_us != 0) to_opts.sigkill_to_us = opts.sigkill_to_us; + // Prepare argv before fork(). The child may run in a multi-threaded process + // where another thread held an allocator lock at the time of fork. + std::vector child_argv {}; + child_argv.reserve(argv.size() + 2); + child_argv.push_back(file.c_str()); + child_argv.insert(child_argv.end(), argv.begin(), argv.end()); + child_argv.push_back(nullptr); + // Pipes for parent to write and read int read_p_err = pipe(pipes[PARENT_READ_PIPE]); int write_p_err = pipe(pipes[PARENT_WRITE_PIPE]); @@ -279,44 +287,22 @@ int wexecvp( } if(child_pid == 0) { - int child_err = 0; - std::vector _argv = argv; - - // Append null to end of _argv for extra safety - _argv.push_back(nullptr); - // Duplicate file argument to avoid manual duplication - _argv.insert(_argv.begin(), file.c_str()); - - // close all files , with the exception of the pipes - close_all_non_term_fd({ CHILD_READ_FD, CHILD_WRITE_FD, CHILD_WRITE_ERR, PARENT_READ_FD, PARENT_READ_ERR, PARENT_WRITE_FD}); - // Copy the pipe descriptors int dup_read_err = dup2(CHILD_READ_FD, STDIN_FILENO); int dup_write_err = dup2(CHILD_WRITE_FD, STDOUT_FILENO); int dup_err_err = dup2(CHILD_WRITE_ERR, STDERR_FILENO); if (dup_read_err == -1 || dup_write_err == -1 || dup_err_err == -1) { - exit(errno); + _exit(errno); } - // Close no longer needed pipes - close(CHILD_READ_FD); - close(CHILD_WRITE_FD); - close(CHILD_WRITE_ERR); - - close(PARENT_READ_FD); - close(PARENT_READ_ERR); - close(PARENT_WRITE_FD); - - - char** args = const_cast(_argv.data()); - child_err = execvp(file.c_str(), args); + // The pipe endpoints now live on stdin/stdout/stderr. With no exclusions, + // descriptor cleanup can use close_range() (or its allocation-free fallback). + close_all_non_term_fd({}); - if (child_err) { - exit(errno); - } else { - exit(0); - } + char** args = const_cast(child_argv.data()); + execvp(file.c_str(), args); + _exit(errno); } else { std::string stdout_ {}; std::string stderr_ {}; @@ -531,7 +517,7 @@ std::string get_checksum_from_hash(uint64_t hash) { * - This method is O(1) and the most efficient * - ONLY used when excludeFDs is empty (otherwise would close excluded fds) * - * 2. **Secondary Method:** Iterate through /proc/self/fd + * 2. **Secondary Method (non-empty excludeFDs only):** Iterate through /proc/self/fd * - Uses opendir("/proc/self/fd") to get a directory stream of open file descriptors * - Uses dirfd() to get the directory's own fd and skips closing it (prevents self-referential closure bug) * - Reads each entry and uses atoi() to convert to fd (no heap allocation) @@ -539,8 +525,9 @@ std::string get_checksum_from_hash(uint64_t hash) { * - This method is O(n) where n is the number of open file descriptors * * 3. **Fallback Method:** Iterate through rlimit - * - If /proc/self/fd is not available (e.g., on non-Linux systems or chroot environments), - * falls back to getrlimit(RLIMIT_NOFILE) + * - For an empty excludeFDs list, this is used directly when close_range() is unavailable, + * because opendir() may allocate and is unsafe after a multi-threaded fork + * - For a non-empty list, this is used if /proc/self/fd is unavailable * - Iterates from 3 to rlim_cur-1, attempting to close each descriptor * - Ignores EBADF errors for descriptors that aren't actually open * - This method is O(rlim_cur) which can be much slower if rlim_max is large (e.g., 1048576) @@ -558,10 +545,10 @@ std::string get_checksum_from_hash(uint64_t hash) { * - This prevents undefined behavior from closing the fd while iterating * * **Thread Safety Considerations:** - * - This function IS safe to call in the child process between fork() and execve() - * - By avoiding heap allocations (using atoi() and simple loops), it prevents deadlocks - * on malloc locks that may be held by other threads in the parent at fork time - * - For optimal safety, call with an empty excludeFDs initializer list: close_all_non_term_fd({}) + * - With an empty excludeFDs list, this function is safe to call in the child process + * between fork() and execve(): both close_range() and the rlimit fallback avoid allocation + * - A non-empty excludeFDs list may use opendir() and should not be used after a + * multi-threaded fork * * **Parameters:** * @param excludeFDs A vector of file descriptor numbers to keep open (in addition to 0, 1, 2) @@ -603,8 +590,13 @@ void close_all_non_term_fd(const std::vector& excludeFDs) { static int close_range_available = -1; // -1 = unknown, 0 = not available, 1 = available if (close_range_available == 1) { // close_range is available, use it to close all fds >= 3 - syscall(__NR_close_range, 3, ~0U, 0); - return; + long ret = syscall(__NR_close_range, 3, ~0U, 0); + if (ret == 0) { + return; + } + if (errno == ENOSYS) { + close_range_available = 0; + } } if (close_range_available == -1) { // First call: check if close_range is available @@ -622,6 +614,19 @@ void close_all_non_term_fd(const std::vector& excludeFDs) { } #endif + // For an empty exclusion list, callers can be in the child of a + // multi-threaded fork. Avoid opendir(), which may allocate internally. + if (excludeFDs.empty()) { + struct rlimit nlimit; + int rc = getrlimit(RLIMIT_NOFILE, &nlimit); + if (rc == 0) { + for (rlim_t fd_rlim = 3; fd_rlim < nlimit.rlim_cur && fd_rlim <= INT_MAX; fd_rlim++) { + close(static_cast(fd_rlim)); + } + } + return; + } + // Fallback: iterate through /proc/self/fd DIR *d; struct dirent *dir; diff --git a/src/main.cpp b/src/main.cpp index 0cf8737bf0..e6b1eae565 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1361,9 +1361,6 @@ void ProxySQL_Main_shutdown_all_modules() { } { -#ifdef TEST_WITHASAN - pthread_mutex_lock(&GloAdmin->sql_query_global_mutex); -#endif cpu_timer t; delete GloAdmin; #ifdef DEBUG @@ -1830,6 +1827,9 @@ bool ProxySQL_Main_init_phase3___start_all() { void ProxySQL_Main_init_phase4___shutdown() { cpu_timer t; + // Stop accepting admin work and wait for all detached admin clients before + // the modules used by admin queries are joined or destroyed. + GloAdmin->shutdown_threads(); ProxySQL_Main_join_all_threads(); //write(GloAdmin->pipefd[1], &GloAdmin->pipefd[1], 1); // write a random byte diff --git a/test/infra/control/asan-detection.bash b/test/infra/control/asan-detection.bash new file mode 100755 index 0000000000..de3f4e472c --- /dev/null +++ b/test/infra/control/asan-detection.bash @@ -0,0 +1,20 @@ +#!/bin/bash + +# Return success only when the supplied ELF binary has a dynamic dependency on +# libasan. The central build hands binaries to separate TAP workflows, so build +# flags are no longer available when the test infrastructure starts. +proxysql_binary_uses_asan() { + local binary="${1:-}" + local dynamic_section + + [ -r "${binary}" ] || return 1 + + if command -v readelf >/dev/null 2>&1; then + dynamic_section="$(LC_ALL=C readelf --dynamic "${binary}" 2>/dev/null)" || return 1 + grep -Eq '\(NEEDED\).*\[libasan\.so(\.[^]]*)?\]' <<< "${dynamic_section}" + else + # proxysql's GCC ASAN build links libasan dynamically. Keep a + # dependency-free fallback for minimal runner hosts without binutils. + LC_ALL=C grep -aEq 'libasan\.so(\.[0-9]+)+' "${binary}" + fi +} diff --git a/test/infra/control/env-isolated.bash b/test/infra/control/env-isolated.bash index 880f1d040e..7b0adb8022 100755 --- a/test/infra/control/env-isolated.bash +++ b/test/infra/control/env-isolated.bash @@ -104,14 +104,16 @@ export TEST_PY_TAP_REPEAT="${TEST_PY_TAP_REPEAT:-1}" export TEST_PY_TAP_SHUFFLE_LIMIT="${TEST_PY_TAP_SHUFFLE_LIMIT:-0}" export TEST_PY_TAP_DUMP_RUNTIME="${TEST_PY_TAP_DUMP_RUNTIME:-1}" export TEST_PY_TAP_DUMP_STATS="${TEST_PY_TAP_DUMP_STATS:-1}" -# Per-test wall-clock budget, in seconds. 0 disables it entirely, which was -# the previous default: a hung TAP test then ran until the CI job itself was -# killed. 1800 is ~2.4x the slowest single test measured across 47 groups -# (reg_test_3765_ssl_pollout-t, 12.5 min; then test_cluster_sync-t 10.5, -# set_testing-240-t 7.8, test_auth_methods-t 7.7 -- only 4 tests exceed 5 -# minutes at all), so it cannot fire on a merely slow test while still -# catching a hang long before the 90-minute step budget. -export TEST_TAP_TIMEOUT="${TEST_TAP_TIMEOUT:-1800}" +# Per-test wall-clock budget, in seconds. 0 disables it entirely. Normal TAP +# runs retain the measured 30-minute ceiling. The exhaustive authentication +# matrix takes just over 30 minutes with ASAN instrumentation, so ASAN gets a +# bounded 60-minute ceiling while remaining below the 90-minute job budget. +DEFAULT_TEST_TAP_TIMEOUT=1800 +if [ "${WITHASAN}" = "1" ]; then + DEFAULT_TEST_TAP_TIMEOUT=3600 +fi +export TEST_TAP_TIMEOUT="${TEST_TAP_TIMEOUT:-${DEFAULT_TEST_TAP_TIMEOUT}}" +unset DEFAULT_TEST_TAP_TIMEOUT # Cluster sync test support — expose first cluster node admin port for replica validation if [ "${NUM_CLUSTER_NODES}" -gt 0 ]; then diff --git a/test/infra/control/run-tests-isolated.bash b/test/infra/control/run-tests-isolated.bash index 12fcff6c13..88f570e5f1 100755 --- a/test/infra/control/run-tests-isolated.bash +++ b/test/infra/control/run-tests-isolated.bash @@ -31,6 +31,18 @@ SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" REPO_ROOT="$(cd "${SCRIPT_DIR}/../../.." && pwd)" export WORKSPACE="${REPO_ROOT}" +source "${SCRIPT_DIR}/asan-detection.bash" + +TEST_SANITIZER_ENV=(-e WITHASAN=0) +if proxysql_binary_uses_asan "${WORKSPACE}/src/proxysql"; then + export WITHASAN=1 + export ASAN_OPTIONS="${ASAN_OPTIONS:-detect_leaks=0}" + TEST_SANITIZER_ENV=(-e WITHASAN=1 -e ASAN_OPTIONS="${ASAN_OPTIONS}") + echo ">>> Detected ASAN-instrumented ProxySQL; enabling ASAN-aware TAP behavior" +else + export WITHASAN=0 +fi + # Default INFRA_ID if not provided export INFRA_ID="${INFRA_ID:-dev-$USER}" export INFRA="${INFRA:-${INFRA_TYPE}}" @@ -312,6 +324,7 @@ docker run \ -e MULTI_GROUP="${MULTI_GROUP:-0}" \ -e GCOV_PREFIX="/gcov/tap" \ -e GCOV_PREFIX_STRIP="2" \ + "${TEST_SANITIZER_ENV[@]}" \ proxysql-ci-base:latest \ /bin/bash -c " set -e diff --git a/test/infra/control/start-proxysql-isolated.bash b/test/infra/control/start-proxysql-isolated.bash index f5828c7e48..31ca65dd5e 100755 --- a/test/infra/control/start-proxysql-isolated.bash +++ b/test/infra/control/start-proxysql-isolated.bash @@ -8,6 +8,17 @@ REPO_ROOT="$(cd "${SCRIPT_DIR}/../../.." && pwd)" export WORKSPACE="${REPO_ROOT}" source "${SCRIPT_DIR}/docker-fs-helper.bash" +source "${SCRIPT_DIR}/asan-detection.bash" + +PROXYSQL_SANITIZER_ENV=() +if proxysql_binary_uses_asan "${WORKSPACE}/src/proxysql"; then + export WITHASAN=1 + export ASAN_OPTIONS="${ASAN_OPTIONS:-detect_leaks=0}" + PROXYSQL_SANITIZER_ENV=(-e WITHASAN=1 -e ASAN_OPTIONS="${ASAN_OPTIONS}") + echo ">>> Detected ASAN-instrumented ProxySQL; LeakSanitizer disabled for daemon processes" +else + export WITHASAN=0 +fi if [ -z "${INFRA_ID}" ]; then echo "Error: INFRA_ID is not set."; exit 1; fi @@ -238,6 +249,7 @@ docker run -d \ ${PGSQL_SOCKET_MOUNT} \ -e GCOV_PREFIX="/gcov" \ -e GCOV_PREFIX_STRIP="2" \ + "${PROXYSQL_SANITIZER_ENV[@]}" \ proxysql-ci-base:latest \ /bin/bash -c "${STARTUP_CMD}" diff --git a/test/infra/control/test-asan-detection.bash b/test/infra/control/test-asan-detection.bash new file mode 100755 index 0000000000..408feedd9e --- /dev/null +++ b/test/infra/control/test-asan-detection.bash @@ -0,0 +1,28 @@ +#!/bin/bash +set -euo pipefail + +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +subject="${script_dir}/asan-detection.bash" +tmp_dir="$(mktemp -d)" +trap 'rm -rf "${tmp_dir}"' EXIT + +printf '%s\n' 'int main() { return 0; }' > "${tmp_dir}/main.cpp" +"${CXX:-c++}" "${tmp_dir}/main.cpp" -o "${tmp_dir}/plain" +"${CXX:-c++}" -fsanitize=address "${tmp_dir}/main.cpp" -o "${tmp_dir}/asan" + +source "${subject}" + +if proxysql_binary_uses_asan "${tmp_dir}/plain"; then + echo "plain binary incorrectly detected as ASAN" >&2 + exit 1 +fi +if ! proxysql_binary_uses_asan "${tmp_dir}/asan"; then + echo "ASAN binary was not detected" >&2 + exit 1 +fi +if proxysql_binary_uses_asan "${tmp_dir}/missing"; then + echo "missing binary incorrectly detected as ASAN" >&2 + exit 1 +fi + +echo "ASAN binary detection tests passed" diff --git a/test/tap/tests/Makefile b/test/tap/tests/Makefile index 849f2abaaf..77f8865271 100644 --- a/test/tap/tests/Makefile +++ b/test/tap/tests/Makefile @@ -131,7 +131,16 @@ ifneq ($(wildcard $(SQLITE3_LDIR)/vec.o),) endif endif -OPT := $(STDCPP) -O2 -ggdb $(WGCOV) $(WASAN) -DGITVERSION=\"$(GIT_VERSION)\" +# Keep public class layouts identical to libproxysql.a. In particular, +# PROXYSQL40 adds members to ProxySQL_GlobalVariables; omitting the define in +# TAP translation units makes their GloVars allocation smaller than the +# constructor linked from the archive. +PSQL40 := +ifeq ($(PROXYSQL40),1) + PSQL40 := -DPROXYSQL40 +endif + +OPT := $(STDCPP) -O2 -ggdb $(PSQL40) $(WGCOV) $(WASAN) -DGITVERSION=\"$(GIT_VERSION)\" ifneq ($(UNAME_S),Darwin) OPT += -Wl,--no-as-needed endif @@ -159,7 +168,7 @@ tests_no_infra: admin_set_credentials_logging-t listener_conflicts_validation-t ./admin_set_credentials_logging-t ./listener_conflicts_validation-t -DEBUG_OPT := $(STDCPP) -O0 -DDEBUG -ggdb $(WGCOV) $(WASAN) -DGITVERSION=\"$(GIT_VERSION)\" +DEBUG_OPT := $(STDCPP) -O0 -DDEBUG -ggdb $(PSQL40) $(WGCOV) $(WASAN) -DGITVERSION=\"$(GIT_VERSION)\" ifneq ($(UNAME_S),Darwin) DEBUG_OPT += -Wl,--no-as-needed endif @@ -416,7 +425,7 @@ prepare_statement_err3024_async-t: prepare_statement_err3024-t.cpp $(TAP_LDIR)/l ifneq ($(UNAME_S),Darwin) test_wexecvp_syscall_failures-t: test_wexecvp_syscall_failures-t.cpp $(TAP_LDIR)/libtap$(SHLIB_EXT) - $(CXX) $< $(IDIRS) $(LDIRS) $(OPT) $(MYLIBS) -Wl,--wrap=pipe,--wrap=fcntl,--wrap=read,--wrap=poll $(STATIC_LIBS) -o $@ + $(CXX) $< $(IDIRS) $(LDIRS) $(OPT) $(MYLIBS) -Wl,--wrap=pipe,--wrap=fcntl,--wrap=read,--wrap=poll,--wrap=fork,--wrap=_Znwm,--wrap=opendir $(STATIC_LIBS) -o $@ endif # Every test that links pg_lite_client.cpp shares one link line. pg_lite_client diff --git a/test/tap/tests/reg_test_3223-restapi_return_codes-t.cpp b/test/tap/tests/reg_test_3223-restapi_return_codes-t.cpp index 94ac2fe887..6a58dac68a 100644 --- a/test/tap/tests/reg_test_3223-restapi_return_codes-t.cpp +++ b/test/tap/tests/reg_test_3223-restapi_return_codes-t.cpp @@ -310,9 +310,9 @@ int main(int argc, char** argv) { vector i_epts_info {}; const auto ext_i_epts_info = [] (const faulty_req_t& req) { return req.ept_info; }; - // Failed scripts may require to read the full output from ASAN leaks report. This in combination with the - // forked process shutdown slowdown can take a considerable amount of time. A cleaner solution would be - // to disable 'detect_leaks' at runtime, but doesn't look feasible at the moment. + // ASAN makes forked child shutdown slower even though the isolated runner + // disables LeakSanitizer for daemon processes. Preserve extra headroom for + // these intentionally failing script executions. int wasan = get_env_int("WITHASAN", 0); if (wasan) { for (auto& req : invalid_requests) { diff --git a/test/tap/tests/reg_test_4001-restapi_scripts_num_fds-t.cpp b/test/tap/tests/reg_test_4001-restapi_scripts_num_fds-t.cpp index e7c028ceb2..ada8c7a08f 100755 --- a/test/tap/tests/reg_test_4001-restapi_scripts_num_fds-t.cpp +++ b/test/tap/tests/reg_test_4001-restapi_scripts_num_fds-t.cpp @@ -100,10 +100,16 @@ int main(int argc, char** argv) { diag("RESTAPI enabled."); string script_base_path = script_dst; - const ept_info_t dummy_ept { "dummy_ept_script", "%s.py", "POST", 5000 }; + const bool with_asan = get_env_int("WITHASAN", 0) != 0; + const uint64_t endpoint_timeout = with_asan ? 15000 : 5000; + const ept_info_t dummy_ept { "dummy_ept_script", "%s.py", "POST", endpoint_timeout }; vector v_epts_info {}; - for (const auto& req : honest_requests) v_epts_info.push_back(req.ept_info); + for (const auto& req : honest_requests) { + ept_info_t ept_info = req.ept_info; + ept_info.timeout = endpoint_timeout; + v_epts_info.push_back(ept_info); + } diag("Configuring RESTAPI endpoints using scripts in: %s", script_base_path.c_str()); int ept_conf_res = configure_endpoints(admin, script_base_path, v_epts_info, dummy_ept, true); diff --git a/test/tap/tests/test_cluster_sync-t.cpp b/test/tap/tests/test_cluster_sync-t.cpp index be3fdcf6c6..cf1a9e9657 100644 --- a/test/tap/tests/test_cluster_sync-t.cpp +++ b/test/tap/tests/test_cluster_sync-t.cpp @@ -62,6 +62,7 @@ */ #include +#include #include #include #include @@ -1288,31 +1289,53 @@ int main(int, char**) { // Launch proxysql with cluster config via fork/exec so we can track the PID std::atomic replica_pid { 0 }; + std::atomic replica_exited { false }; - std::thread proxy_replica_th([&save_proxy_stderr, &replica_pid, &cl] () { + std::thread proxy_replica_th([&save_proxy_stderr, &replica_pid, &replica_exited, &cl] () { const string replica_stderr { string(cl.workdir) + "test_cluster_sync_config/cluster_sync_node_stderr.txt" }; const std::string proxysql_db = std::string(cl.workdir) + "test_cluster_sync_config/proxysql.db"; const std::string stats_db = std::string(cl.workdir) + "test_cluster_sync_config/proxysql_stats.db"; const std::string fmt_config_file = std::string(cl.workdir) + "test_cluster_sync_config/test_cluster_sync.cnf"; const string proxy_binary_path { string { cl.workdir } + "../../../src/proxysql" }; - const string proxy_command { - proxy_binary_path + " -f -M -c " + fmt_config_file + " > " + replica_stderr + " 2>&1" - }; + diag("Launching replica ProxySQL via fork/exec: `%s -f -M -c %s`", proxy_binary_path.c_str(), fmt_config_file.c_str()); - diag("Launching replica ProxySQL via fork/exec with command: `%s`", proxy_command.c_str()); + int stderr_fd = open(replica_stderr.c_str(), O_WRONLY | O_CREAT | O_TRUNC, 0644); + if (stderr_fd == -1) { + diag("Failed to open replica stderr file '%s': %s", replica_stderr.c_str(), strerror(errno)); + ok(false, "proxysql cluster node should execute and shutdown nicely. Failed to open stderr file"); + replica_exited.store(true); + return; + } pid_t pid = fork(); if (pid == 0) { - execl("/bin/sh", "sh", "-c", proxy_command.c_str(), nullptr); - _exit(127); + if (dup2(stderr_fd, STDOUT_FILENO) == -1 || dup2(stderr_fd, STDERR_FILENO) == -1) { + _exit(errno); + } + close_all_non_term_fd({}); + execl(proxy_binary_path.c_str(), proxy_binary_path.c_str(), "-f", "-M", "-c", fmt_config_file.c_str(), nullptr); + _exit(errno); + } + + close(stderr_fd); + if (pid == -1) { + diag("Failed to fork replica ProxySQL: %s", strerror(errno)); + ok(false, "proxysql cluster node should execute and shutdown nicely. Failed to fork"); + replica_exited.store(true); + return; } replica_pid.store(pid); int status = 0; - waitpid(pid, &status, 0); - int exec_res = WIFEXITED(status) ? WEXITSTATUS(status) : -1; + pid_t wait_res = 0; + do { + wait_res = waitpid(pid, &status, 0); + } while (wait_res == -1 && errno == EINTR); + + int exec_res = wait_res == pid && WIFEXITED(status) ? WEXITSTATUS(status) : -1; + replica_exited.store(true); ok(exec_res == 0, "proxysql cluster node should execute and shutdown nicely. Exit status was: %d", exec_res); @@ -1322,6 +1345,23 @@ int main(int, char**) { } else { diag("LOG: One of the tests failed to pass, logging stderr 'test_cluster_sync_config/cluster_sync_node_stderr.txt'"); } + + std::ifstream stderr_stream { replica_stderr }; + constexpr std::streamoff max_stderr_bytes = 64 * 1024; + stderr_stream.seekg(0, std::ios::end); + std::streamoff stderr_size = stderr_stream.tellg(); + if (stderr_size > max_stderr_bytes) { + stderr_stream.seekg(stderr_size - max_stderr_bytes); + std::string partial_line {}; + std::getline(stderr_stream, partial_line); + diag("REPLICA STDERR: [showing final 64 KiB]"); + } else { + stderr_stream.seekg(0); + } + std::string stderr_line {}; + while (std::getline(stderr_stream, stderr_line)) { + diag("REPLICA STDERR: %s", stderr_line.c_str()); + } } remove(proxysql_db.c_str()); @@ -2787,27 +2827,29 @@ int main(int, char**) { mysql_options(r_proxy_admin, MYSQL_OPT_CONNECT_TIMEOUT, &mysql_timeout); mysql_options(r_proxy_admin, MYSQL_OPT_READ_TIMEOUT, &mysql_timeout); mysql_options(r_proxy_admin, MYSQL_OPT_WRITE_TIMEOUT, &mysql_timeout); - mysql_query(r_proxy_admin, "PROXYSQL SHUTDOWN"); + int shutdown_rc = mysql_query(r_proxy_admin, "PROXYSQL SHUTDOWN"); + if (shutdown_rc != 0) { + diag("PROXYSQL SHUTDOWN returned %d: %s", shutdown_rc, mysql_error(r_proxy_admin)); + } mysql_close(r_proxy_admin); } - // Ensure the replica process is dead before joining the thread. + // Ensure the replica process is dead before joining the thread. The worker + // is the only thread that reaps it; polling waitpid() here would race with it. // If PROXYSQL SHUTDOWN failed or r_proxy_admin was NULL, the process // launched via fork() would block the thread forever. { pid_t pid = replica_pid.load(); if (pid > 0) { - int wait_secs = 5; - bool exited = false; - for (int i = 0; i < wait_secs; i++) { - if (waitpid(pid, nullptr, WNOHANG) != 0) { - exited = true; - break; - } - sleep(1); + const int wait_secs = get_env_int("WITHASAN", 0) ? 90 : 5; + const uint64_t wait_us = wait_secs * 1000 * 1000ULL; + const uint64_t wait_started = get_timestamp_us(); + while (!replica_exited.load() && get_timestamp_us() - wait_started < wait_us) { + usleep(100 * 1000); } - if (!exited) { - diag("Replica ProxySQL (pid=%d) did not exit after SHUTDOWN, sending SIGKILL", pid); + if (!replica_exited.load()) { + diag("Replica ProxySQL (pid=%d) did not exit within %d seconds after SHUTDOWN, sending SIGKILL", pid, wait_secs); + save_proxy_stderr.store(true); kill(pid, SIGKILL); } } diff --git a/test/tap/tests/test_wexecvp_syscall_failures-t.cpp b/test/tap/tests/test_wexecvp_syscall_failures-t.cpp index 68156b9472..b80ed59ba6 100644 --- a/test/tap/tests/test_wexecvp_syscall_failures-t.cpp +++ b/test/tap/tests/test_wexecvp_syscall_failures-t.cpp @@ -8,7 +8,9 @@ #include #include +#include #include +#include #include #include "proxysql_utils.h" @@ -19,6 +21,40 @@ using std::string; using std::vector; using std::pair; +bool g_reject_child_unsafe_calls = false; +volatile sig_atomic_t g_in_fork_child = 0; + +extern "C" pid_t __real_fork(void); +extern "C" pid_t __wrap_fork(void); + +pid_t __wrap_fork(void) { + pid_t pid = __real_fork(); + if (pid == 0) { + g_in_fork_child = 1; + } + return pid; +} + +extern "C" void* __real__Znwm(size_t size); +extern "C" void* __wrap__Znwm(size_t size); + +void* __wrap__Znwm(size_t size) { + if (g_reject_child_unsafe_calls && g_in_fork_child) { + _exit(ECANCELED); + } + return __real__Znwm(size); +} + +extern "C" DIR* __real_opendir(const char* name); +extern "C" DIR* __wrap_opendir(const char* name); + +DIR* __wrap_opendir(const char* name) { + if (g_reject_child_unsafe_calls && g_in_fork_child) { + _exit(ECANCELED); + } + return __real_opendir(name); +} + bool g_read_use_real = false; int g_read_ret = -1; int g_read_errno = EINVAL; @@ -173,7 +209,7 @@ int main(int argc, char** argv) { } } - plan(planned_tests); + plan(planned_tests + 1); for (const test_pl_t& pl : test_pls) { enable_reals(test_pls); @@ -185,5 +221,18 @@ int main(int argc, char** argv) { check_read_failure(base_path, pl, ""); } + { + string child_stdout {}; + string child_stderr {}; + to_opts_t opts {}; + opts.timeout_us = 1000 * 1000; + + g_reject_child_unsafe_calls = true; + int err = wexecvp("/bin/true", {}, opts, child_stdout, child_stderr); + g_reject_child_unsafe_calls = false; + + ok(err == 0, "wexecvp should not allocate or open directories between fork and exec"); + } + return exit_status(); }