From 4017378beed72021ccb92165cae22653ecac27fa Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio Date: Sat, 13 Dec 2025 13:05:50 +0100 Subject: [PATCH v3 2/3] Don't use deprecated and insecure PQcancel psql and other tools anymore All of our frontend tools that used our fe_utils to cancel queries, including psql, still used PQcancel to send cancel requests to the server. That function is insecure, because it does not use encryption to send the cancel request. This starts using the new cancellation APIs (introduced in 61461a300) for all these frontend tools. These APIs use the same encryption settings as the connection that's being cancelled. Since these APIs are not signal-safe this required a refactor to not send the cancel request in a signal handler anymore, but instead using a dedicated thread. Similar logic was already used for Windows anyway, so this also has the benefit that it makes the cancel logic more uniform across our supported platforms. The calls to PQcancel in pg_dump are still kept and will be removed in a later commit. The reason for that is that that code does not use the helpers from fe_utils to cancel queries, and instead implements its own logic. --- meson.build | 2 +- src/fe_utils/Makefile | 2 +- src/fe_utils/cancel.c | 317 ++++++++++++++++++++++++++---------------- 3 files changed, 202 insertions(+), 119 deletions(-) diff --git a/meson.build b/meson.build index df907b62da3..b14bc155a31 100644 --- a/meson.build +++ b/meson.build @@ -3381,7 +3381,7 @@ frontend_code = declare_dependency( include_directories: [postgres_inc], link_with: [fe_utils, common_static, pgport_static], sources: generated_headers_stamp, - dependencies: [os_deps, libintl], + dependencies: [os_deps, libintl, thread_dep], ) backend_both_deps += [ diff --git a/src/fe_utils/Makefile b/src/fe_utils/Makefile index cbfbf93ac69..809ab21cc0c 100644 --- a/src/fe_utils/Makefile +++ b/src/fe_utils/Makefile @@ -17,7 +17,7 @@ subdir = src/fe_utils top_builddir = ../.. include $(top_builddir)/src/Makefile.global -override CPPFLAGS := -DFRONTEND -I$(libpq_srcdir) $(CPPFLAGS) +override CPPFLAGS := -DFRONTEND -I$(libpq_srcdir) -I$(top_srcdir)/src/port $(CPPFLAGS) OBJS = \ archive.o \ diff --git a/src/fe_utils/cancel.c b/src/fe_utils/cancel.c index e6b75439f56..4c5933e2513 100644 --- a/src/fe_utils/cancel.c +++ b/src/fe_utils/cancel.c @@ -16,9 +16,21 @@ #include "postgres_fe.h" +#include #include +#ifndef WIN32 +#include +#endif + +#ifdef WIN32 +#include "pthread-win32.h" +#else +#include +#endif + #include "common/connect.h" +#include "common/logging.h" #include "fe_utils/cancel.h" #include "fe_utils/string_utils.h" @@ -36,11 +48,22 @@ (void) rc_; \ } while (0) + +/* + * Cancel connection that should be used to send cancel requests. + */ +static PGcancelConn *cancelConn = NULL; + /* - * Contains all the information needed to cancel a query issued from - * a database connection to the backend. + * Generation counter for cancelConn. Incremented each time cancelConn is + * changed. Used to detect if cancelConn was replaced while we were using it. */ -static PGcancel *volatile cancelConn = NULL; +static uint64 cancelConnGeneration = 0; + +/* + * Mutex protecting cancelConn and cancelConnGeneration. + */ +static pthread_mutex_t cancelConnLock = PTHREAD_MUTEX_INITIALIZER; /* * Predetermined localized error strings --- needed to avoid trying @@ -58,186 +81,246 @@ static const char *cancel_not_sent_msg = NULL; */ volatile sig_atomic_t CancelRequested = false; -#ifdef WIN32 -static CRITICAL_SECTION cancelConnLock; -#endif - /* * Additional callback for cancellations. */ static void (*cancel_callback) (void) = NULL; +#ifndef WIN32 +/* + * On Unix, we use the self-pipe trick to wake up the cancel thread from the + * signal handler. + */ +static int cancel_pipe[2] = {-1, -1}; +static pthread_t cancel_thread; +#endif + /* - * SetCancelConn - * - * Set cancelConn to point to the current database connection. + * Send a cancel request to the connection, if one is set. */ -void -SetCancelConn(PGconn *conn) +static void +SendCancelRequest(void) { - PGcancel *oldCancelConn; + PGcancelConn *cc; + uint64 generation; + bool putConnectionBack = false; + + /* + * We take the cancel connection out of the global. This ensures that + * ResetCancelConn or SetCancelConn won't free it while we're using it. + */ + pthread_mutex_lock(&cancelConnLock); + cc = cancelConn; + generation = cancelConnGeneration; + cancelConn = NULL; + pthread_mutex_unlock(&cancelConnLock); -#ifdef WIN32 - EnterCriticalSection(&cancelConnLock); -#endif + if (cc == NULL) + return; - /* Free the old one if we have one */ - oldCancelConn = cancelConn; + write_stderr(cancel_sent_msg); - /* be sure handle_sigint doesn't use pointer while freeing */ - cancelConn = NULL; + if (!PQcancelBlocking(cc)) + { + char *errmsg = PQcancelErrorMessage(cc); - if (oldCancelConn != NULL) - PQfreeCancel(oldCancelConn); + write_stderr(cancel_not_sent_msg); + if (errmsg) + write_stderr(errmsg); + } + /* Reset for possible reuse */ + PQcancelReset(cc); + + /* + * Put the cancel connection back if it wasn't replaced while we were + * using it. + */ + pthread_mutex_lock(&cancelConnLock); + if (cancelConnGeneration == generation) + { + /* Generation unchanged, put it back for reuse */ + cancelConn = cc; + putConnectionBack = true; + } + pthread_mutex_unlock(&cancelConnLock); - cancelConn = PQgetCancel(conn); + /* If it was replaced, we free it, because we were the last user */ + if (!putConnectionBack) + PQcancelFinish(cc); +} -#ifdef WIN32 - LeaveCriticalSection(&cancelConnLock); -#endif + +/* + * Helper to replace cancelConn with a new value. + */ +static void +SetCancelConnInternal(PGcancelConn *newCancelConn) +{ + PGcancelConn *oldCancelConn; + + pthread_mutex_lock(&cancelConnLock); + oldCancelConn = cancelConn; + cancelConn = newCancelConn; + cancelConnGeneration++; + pthread_mutex_unlock(&cancelConnLock); + + if (oldCancelConn != NULL) + PQcancelFinish(oldCancelConn); +} + +/* + * SetCancelConn + * + * Set cancelConn to point to a cancel connection for the given database + * connection. This creates a new PGcancelConn that can be used to send + * cancel requests. + */ +void +SetCancelConn(PGconn *conn) +{ + SetCancelConnInternal(PQcancelCreate(conn)); } /* * ResetCancelConn * - * Free the current cancel connection, if any, and set to NULL. + * Clear cancelConn, preventing any pending cancel from being sent. */ void ResetCancelConn(void) { - PGcancel *oldCancelConn; + SetCancelConnInternal(NULL); +} -#ifdef WIN32 - EnterCriticalSection(&cancelConnLock); -#endif - oldCancelConn = cancelConn; +#ifdef WIN32 +/* + * Console control handler for Windows. + * + * This runs in a separate thread created by the OS, so we can safely call + * the blocking cancel API directly. + */ +static BOOL WINAPI +consoleHandler(DWORD dwCtrlType) +{ + if (dwCtrlType == CTRL_C_EVENT || + dwCtrlType == CTRL_BREAK_EVENT) + { + CancelRequested = true; - /* be sure handle_sigint doesn't use pointer while freeing */ - cancelConn = NULL; + if (cancel_callback != NULL) + cancel_callback(); - if (oldCancelConn != NULL) - PQfreeCancel(oldCancelConn); + SendCancelRequest(); -#ifdef WIN32 - LeaveCriticalSection(&cancelConnLock); -#endif + return TRUE; + } + else + /* Return FALSE for any signals not being handled */ + return FALSE; } +#else /* !WIN32 */ /* - * Code to support query cancellation - * - * Note that sending the cancel directly from the signal handler is safe - * because PQcancel() is written to make it so. We use write() to report - * to stderr because it's better to use simple facilities in a signal - * handler. - * - * On Windows, the signal canceling happens on a separate thread, because - * that's how SetConsoleCtrlHandler works. The PQcancel function is safe - * for this (unlike PQrequestCancel). However, a CRITICAL_SECTION is required - * to protect the PGcancel structure against being changed while the signal - * thread is using it. + * Cancel thread main function. Waits for the signal handler to write to the + * pipe, then sends a cancel request. */ +static void * +cancel_thread_main(void *arg) +{ + for (;;) + { + char buf[16]; + ssize_t rc; -#ifndef WIN32 + /* Wait for signal handler to wake us up */ + rc = read(cancel_pipe[0], buf, sizeof(buf)); + if (rc <= 0) + { + if (errno == EINTR) + continue; + /* Pipe closed or error - exit thread */ + break; + } + + SendCancelRequest(); + } + + return NULL; +} /* - * handle_sigint - * - * Handle interrupt signals by canceling the current command, if cancelConn - * is set. + * Signal handler for SIGINT. Sets CancelRequested and wakes up the cancel + * thread by writing to the pipe. */ static void handle_sigint(SIGNAL_ARGS) { - char errbuf[256]; + int save_errno = errno; + char c = 1; CancelRequested = true; if (cancel_callback != NULL) cancel_callback(); - /* Send QueryCancel if we are processing a database query */ - if (cancelConn != NULL) - { - if (PQcancel(cancelConn, errbuf, sizeof(errbuf))) - { - write_stderr(cancel_sent_msg); - } - else - { - write_stderr(cancel_not_sent_msg); - write_stderr(errbuf); - } - } + /* Wake up the cancel thread - write() is async-signal-safe */ + if (cancel_pipe[1] >= 0) + (void) write(cancel_pipe[1], &c, 1); + + errno = save_errno; } +#endif /* WIN32 */ + + /* * setup_cancel_handler * - * Register query cancellation callback for SIGINT. + * Set up handler for SIGINT (Unix) or console events (Windows) to send a + * cancel request to the server. Also sets up the infrastructure to send + * cancel requests asynchronously. */ void setup_cancel_handler(void (*query_cancel_callback) (void)) { cancel_callback = query_cancel_callback; - cancel_sent_msg = _("Cancel request sent\n"); + cancel_sent_msg = _("Sending cancel request\n"); cancel_not_sent_msg = _("Could not send cancel request: "); - pqsignal(SIGINT, handle_sigint); -} +#ifdef WIN32 + SetConsoleCtrlHandler(consoleHandler, TRUE); +#else -#else /* WIN32 */ + /* + * Set up the self-pipe for communication between signal handler and + * cancel thread. We use a pipe because write() is async-signal-safe. + */ + if (pipe(cancel_pipe) < 0) + { + pg_log_error("could not create pipe for cancel: %m"); + exit(1); + } -static BOOL WINAPI -consoleHandler(DWORD dwCtrlType) -{ - char errbuf[256]; + /* + * Make the write end non-blocking, so that the signal handler won't block + * if the pipe buffer is full (which is very unlikely in practice but + * possible in theory). + */ + fcntl(cancel_pipe[1], F_SETFL, O_NONBLOCK); - if (dwCtrlType == CTRL_C_EVENT || - dwCtrlType == CTRL_BREAK_EVENT) { - CancelRequested = true; - - if (cancel_callback != NULL) - cancel_callback(); + int rc = pthread_create(&cancel_thread, NULL, cancel_thread_main, NULL); - /* Send QueryCancel if we are processing a database query */ - EnterCriticalSection(&cancelConnLock); - if (cancelConn != NULL) + if (rc != 0) { - if (PQcancel(cancelConn, errbuf, sizeof(errbuf))) - { - write_stderr(cancel_sent_msg); - } - else - { - write_stderr(cancel_not_sent_msg); - write_stderr(errbuf); - } + pg_log_error("could not create cancel thread: %s", strerror(rc)); + exit(1); } - - LeaveCriticalSection(&cancelConnLock); - - return TRUE; } - else - /* Return FALSE for any signals not being handled */ - return FALSE; -} -void -setup_cancel_handler(void (*callback) (void)) -{ - cancel_callback = callback; - cancel_sent_msg = _("Cancel request sent\n"); - cancel_not_sent_msg = _("Could not send cancel request: "); - - InitializeCriticalSection(&cancelConnLock); - - SetConsoleCtrlHandler(consoleHandler, TRUE); + pqsignal(SIGINT, handle_sigint); +#endif } - -#endif /* WIN32 */ -- 2.52.0