From 67ef21fd5bc2e470ab4faa103906b612fdc77209 Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Thu, 23 Jul 2026 13:59:54 -0400 Subject: [PATCH v1 1/8] Fix improper shell escaping in various frontend utilities. Various frontend tools, including but not limited to those touched by this commit, do not perform robust shell-escaping of all values passed to the shell. This commit addresses some of the more straightforward deficiencies, but is not a complete fix. Although shell injection is possible in the cases fixed here and in other cases to be fixed in later commits, we view these as robustness fixes rather than security fixes. For any currently-known shell-injection opportunity to constitute a security vulnerability, an attacker would need to lack direct access to the shell but be able to control either (A) some but not all of the arguments being passed to frontend utilities, or (B) pathnames to important filesystem locations such as the PostgreSQL installation directory or the locations of database clusters, or (C) the contents of environment variables passed to affected PostgreSQL binaries. While such scenarios can't be completely ruled out, they seem unlikely in practice. Users who may have such scenarios are advised to carefully consider how inputs to PostgreSQL-provided binaries are sanitized. This commit fixes the following problems: (1) initdb ran postgres without properly escaping the path to the postgres binary; (2) pg_dumpall ran pg_dump without properly escaping the path to the pg_dump binary; (3) pg_upgrade ran initdb, pg_controldata, pg_ctl, pg_dump, pg_dumpall, pg_resetwal, pg_restore, postgres, psql, and vacuumdb without properly escaping the path to the corresponding binaries, the paths to the relevant data directories, the paths to log files to which output was redirected, or the socket directory passed to the server via pg_ctl's -o option; and (4) psql ran the user's choice of editor without properly escaping the path to the file to be edited. Note that pg_upgrade also passes user-provided options specified via -o/--old-options or -O/--new-options through to pg_ctl, and the quoting here is also improper: we just put double quotes around the user-provided string rather than escaping it. That should also be fixed, but this commit does not do so, because it is possible that there are users relying on the existing behavior. Instead, I propose to fix this part of the problem only in the master branch. Note also that when psql invokes an editor or a shell, the configured value is treated as a shell fragment, and this commit does not change that treatment, even on Windows, where the value is also surrounded by double quotes. Backpatch-through: 14 --- src/bin/initdb/initdb.c | 15 ++-- src/bin/pg_dump/pg_dumpall.c | 4 +- src/bin/pg_upgrade/controldata.c | 14 ++-- src/bin/pg_upgrade/dump.c | 21 ++--- src/bin/pg_upgrade/exec.c | 54 +++++++++++- src/bin/pg_upgrade/option.c | 5 +- src/bin/pg_upgrade/pg_upgrade.c | 124 ++++++++++++++------------- src/bin/pg_upgrade/pg_upgrade.h | 2 + src/bin/pg_upgrade/server.c | 138 ++++++++++++++++++++++--------- src/bin/psql/command.c | 28 ++++--- 10 files changed, 268 insertions(+), 137 deletions(-) diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index b3d496372ad..7234a347279 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -1244,13 +1244,14 @@ test_specific_config_settings(int test_conns, int test_av_slots, int test_buffs) initPQExpBuffer(&cmd); /* Set up the test postmaster invocation */ - printfPQExpBuffer(&cmd, - "\"%s\" --check %s %s " + appendShellString(&cmd, backend_exec); + appendPQExpBuffer(&cmd, + " --check %s %s " "-c max_connections=%d " "-c autovacuum_worker_slots=%d " "-c shared_buffers=%d " "-c dynamic_shared_memory_type=%s", - backend_exec, boot_options, extra_options, + boot_options, extra_options, test_conns, test_av_slots, test_buffs, dynamic_shared_memory_type); @@ -1632,7 +1633,8 @@ bootstrap_template1(void) initPQExpBuffer(&cmd); - printfPQExpBuffer(&cmd, "\"%s\" --boot %s %s", backend_exec, boot_options, extra_options); + appendShellString(&cmd, backend_exec); + appendPQExpBuffer(&cmd, " --boot %s %s", boot_options, extra_options); appendPQExpBuffer(&cmd, " -X %d", wal_segment_size_mb * (1024 * 1024)); if (data_checksums) appendPQExpBufferStr(&cmd, " -k"); @@ -3139,8 +3141,9 @@ initialize_data_directory(void) fflush(stdout); initPQExpBuffer(&cmd); - printfPQExpBuffer(&cmd, "\"%s\" %s %s template1 >%s", - backend_exec, backend_options, extra_options, DEVNULL); + appendShellString(&cmd, backend_exec); + appendPQExpBuffer(&cmd, " %s %s template1 >%s", + backend_options, extra_options, DEVNULL); PG_CMD_OPEN(cmd.data); diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c index c53e77c2878..71cfe1afb68 100644 --- a/src/bin/pg_dump/pg_dumpall.c +++ b/src/bin/pg_dump/pg_dumpall.c @@ -1707,8 +1707,8 @@ runPgDump(const char *dbname, const char *create_opts) initPQExpBuffer(&connstrbuf); initPQExpBuffer(&cmd); - printfPQExpBuffer(&cmd, "\"%s\" %s %s", pg_dump_bin, - pgdumpopts->data, create_opts); + appendShellString(&cmd, pg_dump_bin); + appendPQExpBuffer(&cmd, " %s %s", pgdumpopts->data, create_opts); /* * If we have a filename, use the undocumented plain-append pg_dump diff --git a/src/bin/pg_upgrade/controldata.c b/src/bin/pg_upgrade/controldata.c index b3bd4ccde83..61f83355d1e 100644 --- a/src/bin/pg_upgrade/controldata.c +++ b/src/bin/pg_upgrade/controldata.c @@ -119,8 +119,9 @@ get_control_data(ClusterInfo *cluster) if (!live_check || cluster == &new_cluster) { /* only pg_controldata outputs the cluster state */ - snprintf(cmd, sizeof(cmd), "\"%s/pg_controldata\" \"%s\"", - cluster->bindir, cluster->pgdata); + snprintf(cmd, sizeof(cmd), "%s %s", + quote_shell_path_arg(cluster->bindir, "pg_controldata"), + quote_shell_arg(cluster->pgdata)); fflush(NULL); if ((output = popen(cmd, "r")) == NULL) @@ -182,10 +183,11 @@ get_control_data(ClusterInfo *cluster) } } - snprintf(cmd, sizeof(cmd), "\"%s/%s \"%s\"", - cluster->bindir, - live_check ? "pg_controldata\"" : "pg_resetwal\" -n", - cluster->pgdata); + snprintf(cmd, sizeof(cmd), "%s%s %s", + quote_shell_path_arg(cluster->bindir, + live_check ? "pg_controldata" : "pg_resetwal"), + live_check ? "" : " -n", + quote_shell_arg(cluster->pgdata)); fflush(NULL); if ((output = popen(cmd, "r")) == NULL) diff --git a/src/bin/pg_upgrade/dump.c b/src/bin/pg_upgrade/dump.c index f47c8d06211..cf0cd6c4c63 100644 --- a/src/bin/pg_upgrade/dump.c +++ b/src/bin/pg_upgrade/dump.c @@ -21,13 +21,13 @@ generate_old_dump(void) /* run new pg_dumpall binary for globals */ exec_prog(UTILITY_LOG_FILE, NULL, true, true, - "\"%s/pg_dumpall\" %s%s --globals-only --quote-all-identifiers " - "--binary-upgrade %s --no-sync -f \"%s/%s\"", - new_cluster.bindir, cluster_conn_opts(&old_cluster), + "%s %s%s --globals-only --quote-all-identifiers " + "--binary-upgrade %s --no-sync -f %s", + quote_shell_path_arg(new_cluster.bindir, "pg_dumpall"), + cluster_conn_opts(&old_cluster), protocol_negotiation_supported(&old_cluster) ? "" : " -d \"max_protocol_version=3.0\"", log_opts.verbose ? "--verbose" : "", - log_opts.dumpdir, - GLOBALS_DUMP_FILE); + quote_shell_path_arg(log_opts.dumpdir, GLOBALS_DUMP_FILE)); check_ok(); prep_status_progress("Creating dump of database schemas"); @@ -56,15 +56,16 @@ generate_old_dump(void) snprintf(log_file_name, sizeof(log_file_name), DB_DUMP_LOG_FILE_MASK, old_db->db_oid); parallel_exec_prog(log_file_name, NULL, - "\"%s/pg_dump\" %s --no-data %s %s --quote-all-identifiers " - "--binary-upgrade --format=custom %s --no-sync --file=\"%s/%s\" %s", - new_cluster.bindir, cluster_conn_opts(&old_cluster), + "%s %s --no-data %s %s --quote-all-identifiers " + "--binary-upgrade --format=custom %s --no-sync --file=%s %s", + quote_shell_path_arg(new_cluster.bindir, "pg_dump"), + cluster_conn_opts(&old_cluster), (user_opts.transfer_mode == TRANSFER_MODE_SWAP) ? "" : "--sequence-data", log_opts.verbose ? "--verbose" : "", user_opts.do_statistics ? "--statistics" : "--no-statistics", - log_opts.dumpdir, - sql_file_name, escaped_connstr.data); + quote_shell_path_arg(log_opts.dumpdir, sql_file_name), + escaped_connstr.data); termPQExpBuffer(&escaped_connstr); } diff --git a/src/bin/pg_upgrade/exec.c b/src/bin/pg_upgrade/exec.c index a1bdbf373e3..81c473d6c45 100644 --- a/src/bin/pg_upgrade/exec.c +++ b/src/bin/pg_upgrade/exec.c @@ -12,6 +12,7 @@ #include #include "common/string.h" +#include "fe_utils/string_utils.h" #include "fe_utils/version.h" #include "pg_upgrade.h" @@ -40,7 +41,8 @@ get_bin_version(ClusterInfo *cluster) int v1 = 0, v2 = 0; - snprintf(cmd, sizeof(cmd), "\"%s/pg_ctl\" --version", cluster->bindir); + snprintf(cmd, sizeof(cmd), "%s --version", + quote_shell_path_arg(cluster->bindir, "pg_ctl")); fflush(NULL); if ((output = popen(cmd, "r")) == NULL || @@ -103,7 +105,7 @@ exec_prog(const char *log_filename, const char *opt_log_file, if (written >= MAXCMDLEN) pg_fatal("command too long"); written += snprintf(cmd + written, MAXCMDLEN - written, - " >> \"%s\" 2>&1", log_file); + " >> %s 2>&1", quote_shell_arg(log_file)); if (written >= MAXCMDLEN) pg_fatal("command too long"); @@ -416,7 +418,7 @@ check_exec(const char *dir, const char *program, bool check_version) if (validate_exec(path) != 0) pg_fatal("check for \"%s\" failed: %m", path); - snprintf(cmd, sizeof(cmd), "\"%s\" -V", path); + snprintf(cmd, sizeof(cmd), "%s -V", quote_shell_arg(path)); if ((line = pipe_read_line(cmd)) == NULL) pg_fatal("check for \"%s\" failed: cannot execute", @@ -435,3 +437,49 @@ check_exec(const char *dir, const char *program, bool check_version) pg_free(line); } + +/* + * quote_shell_arg + * + * Returns a palloc'd string that has been quoted for use as a shell argument. + */ +char * +quote_shell_arg(const char *arg) +{ + PQExpBufferData buf; + + initPQExpBuffer(&buf); + appendShellString(&buf, arg); + + if (PQExpBufferBroken(&buf)) + pg_fatal("out of memory"); + + return buf.data; +} + +/* + * quote_shell_path_arg + * + * As quote_shell_arg, but the string is created by joining path and filename + * with a slash. + */ +char * +quote_shell_path_arg(const char *path, const char *filename) +{ + PQExpBufferData buf; + char *result; + + initPQExpBuffer(&buf); + appendPQExpBufferStr(&buf, path); + appendPQExpBufferChar(&buf, '/'); + appendPQExpBufferStr(&buf, filename); + + if (PQExpBufferBroken(&buf)) + pg_fatal("out of memory"); + + result = quote_shell_arg(buf.data); + + termPQExpBuffer(&buf); + + return result; +} diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c index f01d2f92d95..d9662407d48 100644 --- a/src/bin/pg_upgrade/option.c +++ b/src/bin/pg_upgrade/option.c @@ -465,8 +465,9 @@ adjust_data_dir(ClusterInfo *cluster) * so this might fail --- only works for PG 9.2+. If this fails, * pg_upgrade will fail anyway because the data files will not be found. */ - snprintf(cmd, sizeof(cmd), "\"%s/postgres\" -D \"%s\" -C data_directory", - cluster->bindir, cluster->pgconfig); + snprintf(cmd, sizeof(cmd), "%s -D %s -C data_directory", + quote_shell_path_arg(cluster->bindir, "postgres"), + quote_shell_arg(cluster->pgconfig)); fflush(NULL); if ((output = popen(cmd, "r")) == NULL || diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c index 7366fd4627c..46446f2dab7 100644 --- a/src/bin/pg_upgrade/pg_upgrade.c +++ b/src/bin/pg_upgrade/pg_upgrade.c @@ -196,9 +196,10 @@ main(int argc, char **argv) */ prep_status("Setting next OID for new cluster"); exec_prog(UTILITY_LOG_FILE, NULL, true, true, - "\"%s/pg_resetwal\" -o %u \"%s\"", - new_cluster.bindir, old_cluster.controldata.chkpnt_nxtoid, - new_cluster.pgdata); + "%s -o %u %s", + quote_shell_path_arg(new_cluster.bindir, "pg_resetwal"), + old_cluster.controldata.chkpnt_nxtoid, + quote_shell_arg(new_cluster.pgdata)); check_ok(); migrate_logical_slots = count_old_cluster_logical_slots(); @@ -241,11 +242,11 @@ main(int argc, char **argv) { prep_status("Sync data directory to disk"); exec_prog(UTILITY_LOG_FILE, NULL, true, true, - "\"%s/initdb\" --sync-only %s \"%s\" --sync-method %s", - new_cluster.bindir, + "%s --sync-only %s %s --sync-method %s", + quote_shell_path_arg(new_cluster.bindir, "initdb"), (user_opts.transfer_mode == TRANSFER_MODE_SWAP) ? "--no-sync-data-files" : "", - new_cluster.pgdata, + quote_shell_arg(new_cluster.pgdata), user_opts.sync_method); check_ok(); } @@ -445,10 +446,10 @@ set_new_cluster_char_signedness(void) prep_status("Setting the default char signedness for new cluster"); exec_prog(UTILITY_LOG_FILE, NULL, true, true, - "\"%s/pg_resetwal\" --char-signedness %s \"%s\"", - new_cluster.bindir, + "%s --char-signedness %s %s", + quote_shell_path_arg(new_cluster.bindir, "pg_resetwal"), new_char_signedness ? "signed" : "unsigned", - new_cluster.pgdata); + quote_shell_arg(new_cluster.pgdata)); check_ok(); } @@ -550,8 +551,9 @@ prepare_new_cluster(void) */ prep_status("Analyzing all rows in the new cluster"); exec_prog(UTILITY_LOG_FILE, NULL, true, true, - "\"%s/vacuumdb\" %s --all --analyze %s", - new_cluster.bindir, cluster_conn_opts(&new_cluster), + "%s %s --all --analyze %s", + quote_shell_path_arg(new_cluster.bindir, "vacuumdb"), + cluster_conn_opts(&new_cluster), log_opts.verbose ? "--verbose" : ""); check_ok(); @@ -563,8 +565,9 @@ prepare_new_cluster(void) */ prep_status("Freezing all rows in the new cluster"); exec_prog(UTILITY_LOG_FILE, NULL, true, true, - "\"%s/vacuumdb\" %s --all --freeze %s", - new_cluster.bindir, cluster_conn_opts(&new_cluster), + "%s %s --all --freeze %s", + quote_shell_path_arg(new_cluster.bindir, "vacuumdb"), + cluster_conn_opts(&new_cluster), log_opts.verbose ? "--verbose" : ""); check_ok(); } @@ -584,10 +587,10 @@ prepare_new_globals(void) prep_status("Restoring global objects in the new cluster"); exec_prog(UTILITY_LOG_FILE, NULL, true, true, - "\"%s/psql\" " EXEC_PSQL_ARGS " %s -f \"%s/%s\"", - new_cluster.bindir, cluster_conn_opts(&new_cluster), - log_opts.dumpdir, - GLOBALS_DUMP_FILE); + "%s " EXEC_PSQL_ARGS " %s -f %s", + quote_shell_path_arg(new_cluster.bindir, "psql"), + cluster_conn_opts(&new_cluster), + quote_shell_path_arg(log_opts.dumpdir, GLOBALS_DUMP_FILE)); check_ok(); } @@ -638,19 +641,15 @@ create_new_objects(void) */ create_opts = "--clean --create"; - exec_prog(log_file_name, - NULL, - true, - true, - "\"%s/pg_restore\" %s %s --exit-on-error --verbose " + exec_prog(log_file_name, NULL, true, true, + "%s %s %s --exit-on-error --verbose " "--transaction-size=%d " - "--dbname postgres \"%s/%s\"", - new_cluster.bindir, + "--dbname postgres %s", + quote_shell_path_arg(new_cluster.bindir, "pg_restore"), cluster_conn_opts(&new_cluster), create_opts, RESTORE_TRANSACTION_SIZE, - log_opts.dumpdir, - sql_file_name); + quote_shell_path_arg(log_opts.dumpdir, sql_file_name)); break; /* done once we've processed template1 */ } @@ -694,17 +693,15 @@ create_new_objects(void) txn_size = Max(txn_size, 10); } - parallel_exec_prog(log_file_name, - NULL, - "\"%s/pg_restore\" %s %s --exit-on-error --verbose " + parallel_exec_prog(log_file_name, NULL, + "%s %s %s --exit-on-error --verbose " "--transaction-size=%d " - "--dbname template1 \"%s/%s\"", - new_cluster.bindir, + "--dbname template1 %s", + quote_shell_path_arg(new_cluster.bindir, "pg_restore"), cluster_conn_opts(&new_cluster), create_opts, txn_size, - log_opts.dumpdir, - sql_file_name); + quote_shell_path_arg(log_opts.dumpdir, sql_file_name)); } /* reap all children */ @@ -749,16 +746,21 @@ copy_subdir_files(const char *old_subdir, const char *new_subdir) snprintf(old_path, sizeof(old_path), "%s/%s", old_cluster.pgdata, old_subdir); snprintf(new_path, sizeof(new_path), "%s/%s", new_cluster.pgdata, new_subdir); +#ifdef WIN32 + /* Trailing backslash tells xcopy the destination is a directory. */ + strlcat(new_path, "\\", sizeof(new_path)); +#endif + prep_status("Copying old %s to new server", old_subdir); exec_prog(UTILITY_LOG_FILE, NULL, true, true, #ifndef WIN32 - "cp -Rf \"%s\" \"%s\"", + "cp -Rf %s %s", #else /* flags: everything, no confirm, quiet, overwrite read-only */ - "xcopy /e /y /q /r \"%s\" \"%s\\\"", + "xcopy /e /y /q /r %s %s", #endif - old_path, new_path); + quote_shell_arg(old_path), quote_shell_arg(new_path)); check_ok(); } @@ -766,6 +768,17 @@ copy_subdir_files(const char *old_subdir, const char *new_subdir) static void copy_xact_xlog_xid(void) { + char *pg_resetwal_path; + char *pgdata; + + /* + * Perform shell quoting on values this function will use repeatedly. + * + * XXX: Some of these pg_resetwal calls could probably be combined. + */ + pg_resetwal_path = quote_shell_path_arg(new_cluster.bindir, "pg_resetwal"); + pgdata = quote_shell_arg(new_cluster.pgdata); + /* * Copy old commit logs to new data dir. pg_clog has been renamed to * pg_xact in post-10 clusters. @@ -774,28 +787,25 @@ copy_xact_xlog_xid(void) prep_status("Setting oldest XID for new cluster"); exec_prog(UTILITY_LOG_FILE, NULL, true, true, - "\"%s/pg_resetwal\" -f -u %u \"%s\"", - new_cluster.bindir, old_cluster.controldata.chkpnt_oldstxid, - new_cluster.pgdata); + "%s -f -u %u %s", + pg_resetwal_path, old_cluster.controldata.chkpnt_oldstxid, pgdata); check_ok(); /* set the next transaction id and epoch of the new cluster */ prep_status("Setting next transaction ID and epoch for new cluster"); exec_prog(UTILITY_LOG_FILE, NULL, true, true, - "\"%s/pg_resetwal\" -f -x %u \"%s\"", - new_cluster.bindir, old_cluster.controldata.chkpnt_nxtxid, - new_cluster.pgdata); + "%s -f -x %u %s", + pg_resetwal_path, old_cluster.controldata.chkpnt_nxtxid, pgdata); exec_prog(UTILITY_LOG_FILE, NULL, true, true, - "\"%s/pg_resetwal\" -f -e %u \"%s\"", - new_cluster.bindir, old_cluster.controldata.chkpnt_nxtepoch, - new_cluster.pgdata); + "%s -f -e %u %s", + pg_resetwal_path, old_cluster.controldata.chkpnt_nxtepoch, pgdata); /* must reset commit timestamp limits also */ exec_prog(UTILITY_LOG_FILE, NULL, true, true, - "\"%s/pg_resetwal\" -f -c %u,%u \"%s\"", - new_cluster.bindir, + "%s -f -c %u,%u %s", + pg_resetwal_path, old_cluster.controldata.chkpnt_nxtxid, old_cluster.controldata.chkpnt_nxtxid, - new_cluster.pgdata); + pgdata); check_ok(); /* Copy or convert pg_multixact files */ @@ -816,10 +826,9 @@ copy_xact_xlog_xid(void) * counters here and the oldest multi present on system. */ exec_prog(UTILITY_LOG_FILE, NULL, true, true, - "\"%s/pg_resetwal\" -O %" PRIu64 " -m %u,%u \"%s\"", - new_cluster.bindir, new_nxtmxoff, new_nxtmulti, - old_cluster.controldata.chkpnt_oldstMulti, - new_cluster.pgdata); + "%s -O %" PRIu64 " -m %u,%u %s", + pg_resetwal_path, new_nxtmxoff, new_nxtmulti, + old_cluster.controldata.chkpnt_oldstMulti, pgdata); check_ok(); } else @@ -856,10 +865,8 @@ copy_xact_xlog_xid(void) prep_status("Setting next multixact ID and offset for new cluster"); exec_prog(UTILITY_LOG_FILE, NULL, true, true, - "\"%s/pg_resetwal\" -O %" PRIu64 " -m %u,%u \"%s\"", - new_cluster.bindir, - nxtmxoff, nxtmulti, oldstMulti, - new_cluster.pgdata); + "%s -O %" PRIu64 " -m %u,%u %s", + pg_resetwal_path, nxtmxoff, nxtmulti, oldstMulti, pgdata); check_ok(); } @@ -867,9 +874,8 @@ copy_xact_xlog_xid(void) prep_status("Resetting WAL archives"); exec_prog(UTILITY_LOG_FILE, NULL, true, true, /* use timeline 1 to match controldata and no WAL history file */ - "\"%s/pg_resetwal\" -l 00000001%s \"%s\"", new_cluster.bindir, - old_cluster.controldata.nextxlogfile + 8, - new_cluster.pgdata); + "%s -l 00000001%s %s", + pg_resetwal_path, old_cluster.controldata.nextxlogfile + 8, pgdata); check_ok(); } diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h index d6e5bca5792..892161f3119 100644 --- a/src/bin/pg_upgrade/pg_upgrade.h +++ b/src/bin/pg_upgrade/pg_upgrade.h @@ -393,6 +393,8 @@ bool exec_prog(const char *log_filename, const char *opt_log_file, bool report_error, bool exit_on_error, const char *fmt, ...) pg_attribute_printf(5, 6); void verify_directories(void); bool pid_lock_file_exists(const char *datadir); +char *quote_shell_arg(const char *arg); +char *quote_shell_path_arg(const char *path, const char *filename); /* file.c */ diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c index 7da9dffe585..31b993b43a9 100644 --- a/src/bin/pg_upgrade/server.c +++ b/src/bin/pg_upgrade/server.c @@ -157,14 +157,34 @@ stop_postmaster_atexit(void) } +/* + * To start postgres with a particular value for a particular GUC, we can + * specify -c guc_name=guc_value on the command-line, but we need to + * shell-escape the string to avoid misbehavior in the case where, for + * example, guc_value contains spaces or double quotes. + */ +static void +add_pg_config_option(PQExpBuffer postgres_opts, + const char *guc_name, const char *guc_value) +{ + char *guc_string = psprintf("%s=%s", guc_name, guc_value); + + if (postgres_opts->len > 0) + appendPQExpBufferChar(postgres_opts, ' '); + appendPQExpBufferStr(postgres_opts, "-c "); + appendShellString(postgres_opts, guc_string); + pfree(guc_string); +} + + bool start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error) { - char cmd[MAXPGPATH * 4 + 1000]; + PQExpBufferData cmd; PGconn *conn; bool pg_ctl_return = false; - char socket_string[MAXPGPATH + 200]; - PQExpBufferData pgoptions; + PQExpBufferData postgres_opts; + PQExpBufferData socket_opts; static bool exit_hook_registered = false; @@ -174,48 +194,85 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error) exit_hook_registered = true; } - socket_string[0] = '\0'; - -#if !defined(WIN32) - /* prevent TCP/IP connections, restrict socket access */ - strcat(socket_string, - " -c listen_addresses='' -c unix_socket_permissions=0700"); - - /* Have a sockdir? Tell the postmaster. */ - if (cluster->sockdir) - snprintf(socket_string + strlen(socket_string), - sizeof(socket_string) - strlen(socket_string), - " -c %s='%s'", - "unix_socket_directories", - cluster->sockdir); -#endif - - initPQExpBuffer(&pgoptions); - /* - * Construct a parameter string which is passed to the server process. + * Construct options to be passed to the server process. * + * Use -b to disable autovacuum and logical replication launcher + * (effective in PG17 or later for the latter). + */ + initPQExpBuffer(&postgres_opts); + appendPQExpBuffer(&postgres_opts, "-p %d -b", cluster->port); + + /* * Turn off durability requirements to improve object creation speed, and * we only modify the new cluster, so only use it there. If there is a * crash, the new cluster has to be recreated anyway. fsync=off is a big * win on ext4. */ if (cluster == &new_cluster) - appendPQExpBufferStr(&pgoptions, " -c synchronous_commit=off -c fsync=off -c full_page_writes=off"); + { + add_pg_config_option(&postgres_opts, "synchronous_commit", "off"); + add_pg_config_option(&postgres_opts, "fsync", "off"); + add_pg_config_option(&postgres_opts, "full_page_writes", "off"); + } + + initPQExpBuffer(&socket_opts); + +#if !defined(WIN32) + /* prevent TCP/IP connections, restrict socket access */ + add_pg_config_option(&socket_opts, "listen_addresses", ""); + add_pg_config_option(&socket_opts, "unix_socket_permissions", "0700"); + + /* Have a sockdir? Tell the postmaster. */ + if (cluster->sockdir) + add_pg_config_option(&socket_opts, "unix_socket_directories", + cluster->sockdir); +#endif /* - * Use -b to disable autovacuum and logical replication launcher - * (effective in PG17 or later for the latter). + * Construct the pg_ctl command. + * + * -o/--old-options or -O/--new-options are documented as allowing the + * user to pass through options to the server. To deliver that behavior, + * we should shell-escape them before passing them to pg_ctl -o, since we + * will use the shell to run pg_ctl. However, the historical behavior of + * these flags is actually that they simply wrap the values of the options + * in double-quotes, and it's possible that there are users including + * shell metacharacters in the values passed to those options and relying + * on the faulty escaping for correct operation. Hence, preserve that + * behavior for now. + * + * We do, however, want to escape the other values that we're passing to + * pg_ctl -o, so that if, for example, the socket directory contains shell + * metacharacters, we nevertheless interpret the value as a literal + * pathname. Since appendShellString can only be applied to an entire + * option value as a unit, we specify -o three times: once for the options + * that precede the user-specified options, once for the user-specified + * options, and once for the options that follow the user-specified + * options. The order matters, since later options override earlier ones. */ - snprintf(cmd, sizeof(cmd), - "\"%s/pg_ctl\" -w -l \"%s/%s\" -D \"%s\" -o \"-p %d -b%s %s%s\" start", - cluster->bindir, - log_opts.logdir, - SERVER_LOG_FILE, cluster->pgconfig, cluster->port, - pgoptions.data, - cluster->pgopts ? cluster->pgopts : "", socket_string); + initPQExpBuffer(&cmd); + appendPQExpBufferStr(&cmd, quote_shell_path_arg(cluster->bindir, "pg_ctl")); + appendPQExpBufferStr(&cmd, " -w -l "); + appendPQExpBufferStr(&cmd, + quote_shell_path_arg(log_opts.logdir, + SERVER_LOG_FILE)); + appendPQExpBufferStr(&cmd, " -D "); + appendPQExpBufferStr(&cmd, quote_shell_arg(cluster->pgconfig)); + + appendPQExpBufferStr(&cmd, " -o "); + appendShellString(&cmd, postgres_opts.data); + if (cluster->pgopts) + appendPQExpBuffer(&cmd, " -o \"%s\"", cluster->pgopts); + if (socket_opts.len > 0) + { + appendPQExpBufferStr(&cmd, " -o "); + appendShellString(&cmd, socket_opts.data); + } + appendPQExpBufferStr(&cmd, " start"); - termPQExpBuffer(&pgoptions); + termPQExpBuffer(&postgres_opts); + termPQExpBuffer(&socket_opts); /* * Don't throw an error right away, let connecting throw the error because @@ -227,11 +284,14 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error) SERVER_START_LOG_FILE) != 0) ? SERVER_LOG_FILE : NULL, report_and_exit_on_error, false, - "%s", cmd); + "%s", cmd.data); /* Did it fail and we are just testing if the server could be started? */ if (!pg_ctl_return && !report_and_exit_on_error) + { + termPQExpBuffer(&cmd); return false; + } /* * We set this here to make sure atexit() shuts down the server, but only @@ -264,13 +324,14 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error) if (cluster == &old_cluster) pg_fatal("could not connect to source postmaster started with the command:\n" "%s", - cmd); + cmd.data); else pg_fatal("could not connect to target postmaster started with the command:\n" "%s", - cmd); + cmd.data); } PQfinish(conn); + termPQExpBuffer(&cmd); /* * If pg_ctl failed, and the connection didn't fail, and @@ -302,8 +363,9 @@ stop_postmaster(bool in_atexit) return; /* no cluster running */ exec_prog(SERVER_STOP_LOG_FILE, NULL, !in_atexit, !in_atexit, - "\"%s/pg_ctl\" -w -D \"%s\" -o \"%s\" %s stop", - cluster->bindir, cluster->pgconfig, + "%s -w -D %s -o \"%s\" %s stop", + quote_shell_path_arg(cluster->bindir, "pg_ctl"), + quote_shell_arg(cluster->pgconfig), cluster->pgopts ? cluster->pgopts : "", in_atexit ? "-m fast" : "-m smart"); diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index ee85c05a00d..d5e21c7e35f 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -4647,7 +4647,7 @@ editFile(const char *fname, int lineno) { const char *editorName; const char *editor_lineno_arg = NULL; - char *sys; + PQExpBufferData buf; int result; Assert(fname != NULL); @@ -4683,28 +4683,34 @@ editFile(const char *fname, int lineno) * severe brain damage in their command shell plus the fact that standard * program paths include spaces. */ + initPQExpBuffer(&buf); #ifndef WIN32 if (lineno > 0) - sys = psprintf("exec %s %s%d '%s'", - editorName, editor_lineno_arg, lineno, fname); + appendPQExpBuffer(&buf, "exec %s %s%d ", + editorName, editor_lineno_arg, lineno); else - sys = psprintf("exec %s '%s'", - editorName, fname); + appendPQExpBuffer(&buf, "exec %s ", editorName); #else if (lineno > 0) - sys = psprintf("\"%s\" %s%d \"%s\"", - editorName, editor_lineno_arg, lineno, fname); + appendPQExpBuffer(&buf, "\"%s\" %s%d ", + editorName, editor_lineno_arg, lineno); else - sys = psprintf("\"%s\" \"%s\"", - editorName, fname); + appendPQExpBuffer(&buf, "\"%s\" ", editorName); #endif + if (!appendShellStringNoError(&buf, fname)) + { + pg_log_error("shell command argument contains a newline or carriage return: \"%s\"", + fname); + termPQExpBuffer(&buf); + return false; + } fflush(NULL); - result = system(sys); + result = system(buf.data); if (result == -1) pg_log_error("could not start editor \"%s\"", editorName); else if (result == 127) pg_log_error("could not start /bin/sh"); - pfree(sys); + termPQExpBuffer(&buf); return result == 0; } -- 2.50.1 (Apple Git-155)