From fa1894ab72cb493f4adda5af036fc73f6ecbf187 Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Mon, 27 Jul 2026 09:01:16 -0400 Subject: [PATCH v1 6/8] Rationalize argv escaping on Windows. In most places where we build command lines to be executed on Windows, we execute them via the shell, but there are three places where we build a command-line to be passed to CreateProcess(), which requires different treatment. The backend's internal_forkexec() and pg_ctl's start_postmaster() already get this right, but add comments to make it clearer. pg_ctl's pgwin32_CommandLine() does not get this right. To fix, this commit (1) ports the function to use StringInfo rather than PQExpBuffer; (2) adds StringInfo helpers for the type of escaping needed in these cases; and (3) properly escapes the arguments to pg_ctl register -D, -N, and -e. This fixes a bug where including a double-quote in the argument to pg_ctl register -N or -e would cause the service to get registered incorrectly, so that it would not actually start. It is difficult to construct a realistic failure scenario for pg_ctl register -D because canonicalize_path() is applied to the value, and Windows paths can't contain double quotes anyway. The rationale for using StringInfo here rather than PQExpBuffer is that StringInfo is widely preferred in new code and is available in code that does not depend on libpq; a side effect of this commit is that pg_ctl's libpq dependency is removed. This commit doesn't touch the behavior of pg_ctl -o, which is defined as a shell fragment, so just escaping it wouldn't be correct. Improving this case is left as future work. While the changes to pg_ctl register -N and pg_ctl register -e seem like legitimate bug fixes, they are so low-impact that it does not seem worth the risk of back-patching. --- src/backend/postmaster/launch_backend.c | 9 +- src/bin/pg_ctl/Makefile | 10 +-- src/bin/pg_ctl/meson.build | 2 +- src/bin/pg_ctl/pg_ctl.c | 46 ++++++---- src/common/stringinfo.c | 110 ++++++++++++++++++++++++ src/include/lib/stringinfo.h | 17 ++++ 6 files changed, 166 insertions(+), 28 deletions(-) diff --git a/src/backend/postmaster/launch_backend.c b/src/backend/postmaster/launch_backend.c index 8f3cfea880c..56e2f1a3d02 100644 --- a/src/backend/postmaster/launch_backend.c +++ b/src/backend/postmaster/launch_backend.c @@ -438,7 +438,14 @@ retry: return -1; } - /* Format the cmd line */ + /* + * Format the cmd line. + * + * Note that it is correct that the program name is simply surrounded by + * double quotes, without any escaping. The other arguments can't contain + * any characters that need escaping. For more details, see the comments + * for appendStringInfoWin32Argv(). + */ #ifdef _WIN64 sprintf(paramHandleStr, "%llu", (LONG_PTR) paramHandle); #else diff --git a/src/bin/pg_ctl/Makefile b/src/bin/pg_ctl/Makefile index 5c2d4180980..e8ee3379db7 100644 --- a/src/bin/pg_ctl/Makefile +++ b/src/bin/pg_ctl/Makefile @@ -16,21 +16,13 @@ subdir = src/bin/pg_ctl top_builddir = ../../.. include $(top_builddir)/src/Makefile.global -# On Windows, we need to link with libpq, just for use of pqexpbuffer; -# but let's not pull that in on platforms where we don't need it. -ifeq ($(PORTNAME), win32) -override CPPFLAGS := -I$(libpq_srcdir) $(CPPFLAGS) -LDFLAGS_INTERNAL += $(libpq_pgport) -SUBMAKE_LIBPQ := submake-libpq -endif - OBJS = \ $(WIN32RES) \ pg_ctl.o all: pg_ctl -pg_ctl: $(OBJS) | submake-libpgport $(SUBMAKE_LIBPQ) +pg_ctl: $(OBJS) | submake-libpgport $(CC) $(CFLAGS) $(OBJS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X) install: all installdirs diff --git a/src/bin/pg_ctl/meson.build b/src/bin/pg_ctl/meson.build index 69fa7a28427..90c62c8d65d 100644 --- a/src/bin/pg_ctl/meson.build +++ b/src/bin/pg_ctl/meson.build @@ -12,7 +12,7 @@ endif pg_ctl = executable('pg_ctl', pg_ctl_sources, - dependencies: [frontend_code, libpq], + dependencies: [frontend_code], kwargs: default_bin_args, ) bin_targets += pg_ctl diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c index d99f3c93ca1..aee765bf985 100644 --- a/src/bin/pg_ctl/pg_ctl.c +++ b/src/bin/pg_ctl/pg_ctl.c @@ -31,10 +31,6 @@ #include "lib/stringinfo.h" #include "utils/pidfile.h" -#ifdef WIN32 /* on Unix, we don't need libpq */ -#include "pqexpbuffer.h" -#endif - typedef enum { @@ -561,6 +557,11 @@ start_postmaster(void) close(fd); } + /* + * Note that it is correct that the program name is simply surrounded by + * double quotes, without any escaping. For more details, see the comments + * for appendStringInfoWin32Argv(). + */ initStringInfo(&cmd); appendStringInfo(&cmd, "\"%s\" /C ", comspec); appendStringInfoShell(&cmd, exec_path); @@ -1439,7 +1440,7 @@ pgwin32_IsInstalled(SC_HANDLE hSCM) static char * pgwin32_CommandLine(bool registration) { - PQExpBuffer cmdLine = createPQExpBuffer(); + StringInfoData cmdLine; char cmdPath[MAXPGPATH]; int ret; @@ -1472,14 +1473,21 @@ pgwin32_CommandLine(bool registration) /* use backslashes in path to avoid problems with some third-party tools */ make_native_path(cmdPath); - /* be sure to double-quote the executable's name in the command */ - appendPQExpBuffer(cmdLine, "\"%s\"", cmdPath); + /* + * Note that it is correct that the program name is simply surrounded by + * double quotes, without any escaping. For more details, see the comments + * for appendStringInfoWin32Argv(). + */ + initStringInfo(&cmdLine); + appendStringInfo(&cmdLine, "\"%s\"", cmdPath); /* append assorted switches to the command line, as needed */ if (registration) - appendPQExpBuffer(cmdLine, " runservice -N \"%s\"", - register_servicename); + { + appendStringInfoString(&cmdLine, " runservice -N "); + appendStringInfoWin32Argv(&cmdLine, register_servicename); + } if (pg_config) { @@ -1492,32 +1500,36 @@ pgwin32_CommandLine(bool registration) exit(1); } make_native_path(dataDir); - appendPQExpBuffer(cmdLine, " -D \"%s\"", dataDir); + appendStringInfoString(&cmdLine, " -D "); + appendStringInfoWin32Argv(&cmdLine, dataDir); free(dataDir); } if (registration && event_source != NULL) - appendPQExpBuffer(cmdLine, " -e \"%s\"", event_source); + { + appendStringInfoString(&cmdLine, " -e "); + appendStringInfoWin32Argv(&cmdLine, event_source); + } if (registration && do_wait) - appendPQExpBufferStr(cmdLine, " -w"); + appendStringInfoString(&cmdLine, " -w"); /* Don't propagate a value from an environment variable. */ if (registration && wait_seconds_arg && wait_seconds != DEFAULT_WAIT) - appendPQExpBuffer(cmdLine, " -t %d", wait_seconds); + appendStringInfo(&cmdLine, " -t %d", wait_seconds); if (registration && silent_mode) - appendPQExpBufferStr(cmdLine, " -s"); + appendStringInfoString(&cmdLine, " -s"); if (post_opts) { if (registration) - appendPQExpBuffer(cmdLine, " -o \"%s\"", post_opts); + appendStringInfo(&cmdLine, " -o \"%s\"", post_opts); else - appendPQExpBuffer(cmdLine, " %s", post_opts); + appendStringInfo(&cmdLine, " %s", post_opts); } - return cmdLine->data; + return cmdLine.data; } static void diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c index 9d9dc3000ee..b88fdb28aee 100644 --- a/src/common/stringinfo.c +++ b/src/common/stringinfo.c @@ -405,6 +405,116 @@ appendStringInfoShellNoError(StringInfo str, const char *s) return ok; } +#ifdef WIN32 +/* + * Append the given string to the command line being built in str, quoted so + * that command-line-to-argv parsing on Windows will reconstruct it as exactly + * one argument. This is for use when building a command line to be passed + * to CreateProcess() without the involvement of cmd.exe; in contrast, + * appendStringInfoShell() quotes for both of the layers of interpretation + * that a system() argument experiences. + * + * As with appendStringInfoShell, LF and CR characters are forbidden. + * appendStringInfoWin32Argv() reports an error and does not return if LF or + * CR appears; in the backend it does ereport(ERROR), while in frontend code + * it prints a message and exits. appendStringInfoWin32ArgvNoError() omits + * those characters from the result, and returns false if there were any. + * + * NB: Don't apply this function to the name of the executable, only to + * arguments being passed to it! Special rules apply to argv[0] as opposed + * to later command-line arguments: in secure usage, it should start with + * a double quote; provided that it does, it will continue until the next + * double quote. No escape characters are recognized in between. This works + * because Windows filenames cannot contain a double quote. Hence, while + * command-line argument values should be escaped using this function (except + * when they are also passed through the shell), the right way to escape the + * name of the executable is to use a format string like "\"%s\"", which + * looks wrong but isn't. + */ +void +appendStringInfoWin32Argv(StringInfo str, const char *s) +{ + if (!appendStringInfoWin32ArgvNoError(str, s)) + { +#ifndef FRONTEND + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("command line argument contains a newline or carriage return: \"%s\"", + s))); +#else + fprintf(stderr, + _("command line argument contains a newline or carriage return: \"%s\"\n"), + s); + exit(EXIT_FAILURE); +#endif + } +} + +bool +appendStringInfoWin32ArgvNoError(StringInfo str, const char *s) +{ + int backslash_run_length = 0; + bool ok = true; + const char *p; + + /* + * Don't bother with adding quotes if the string is nonempty and clearly + * contains only safe characters. + */ + if (*s != '\0' && + strspn(s, "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_./:") == strlen(s)) + { + appendStringInfoString(str, s); + return ok; + } + + /* + * https://msdn.microsoft.com/en-us/library/17w5ykft.aspx describes the + * rules that the Microsoft C runtime will follow when parsing the string + * we construct here. + */ + appendStringInfoChar(str, '"'); + for (p = s; *p; p++) + { + if (*p == '\n' || *p == '\r') + { + ok = false; + continue; + } + + /* Change N backslashes before a double quote to 2N+1 backslashes. */ + if (*p == '"') + { + while (backslash_run_length) + { + appendStringInfoChar(str, '\\'); + backslash_run_length--; + } + appendStringInfoChar(str, '\\'); + } + else if (*p == '\\') + backslash_run_length++; + else + backslash_run_length = 0; + + appendStringInfoChar(str, *p); + } + + /* + * Change N backslashes at end of argument to 2N backslashes, because they + * precede the double quote that terminates the argument. + */ + while (backslash_run_length) + { + appendStringInfoChar(str, '\\'); + backslash_run_length--; + } + appendStringInfoChar(str, '"'); + + return ok; +} +#endif /* WIN32 */ + /* * appendBinaryStringInfo * diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h index a5a16021552..06a538425b5 100644 --- a/src/include/lib/stringinfo.h +++ b/src/include/lib/stringinfo.h @@ -253,6 +253,23 @@ extern void appendStringInfoSpaces(StringInfo str, int count); extern void appendStringInfoShell(StringInfo str, const char *s); extern bool appendStringInfoShellNoError(StringInfo str, const char *s); +#ifdef WIN32 +/*------------------------ + * appendStringInfoWin32Argv + * appendStringInfoWin32ArgvNoError + * + * Append a string to str, quoted so that command-line-to-argv parsing on + * Windows will reconstruct it as exactly one argument. Use this for + * command-line arguments being passed directly via CreateProcess() without + * the involvement of cmd.exe; do NOT use it for the name of the executable + * itself (see the function header comments for appendStringInfoWin32Argv + * for full details). Other than the different quoting behavior, these + * functions behave like appendStringInfoShell/appendStringInfoShellNoError. + */ +extern void appendStringInfoWin32Argv(StringInfo str, const char *s); +extern bool appendStringInfoWin32ArgvNoError(StringInfo str, const char *s); +#endif + /*------------------------ * appendBinaryStringInfo * Append arbitrary binary data to a StringInfo, allocating more space -- 2.50.1 (Apple Git-155)