Hello!! For some time I've been wondering why PostgreSQL needs system() calls, which use a shell that can lead to many problems, and also why it requires a shell to run a command.
I first started thinking about this when I was trying to run a full distroless PostgreSQL container. It turns out that isn't possible since the shell is a requirement, and distroless containers are secure exactly _because_ there's no shell to execute any command other than the ones that are meant to be executed. After some research I found out that using system() has other problems, like issues related to quoting that are really painful to solve [0][1], and also the exit codes control[2]. Both topics have already been discussed on the list. But the main argument now for me is security. Not having a shell avoids any possible PATH injection, missing quoting to escape a command, or new lines that the shell interprets differently from what you'd expect. After some thinking I came up with a small interface, which only purpose is to replace system() calls in a more smooth way using execv() under the hood. I suppose you could use execl() but I've decided to keep it simple, leaving the opportunity to expand in the future. I already implemented one call with `pg_ctl initdb` as an example. There's an important topic related to using shell versus not a shell. In some places like `archive_command` people may use `&&`, but this idea aims to avoid this kind of behavior since it's not secure. Probably we can implement a way to run commands in sequence, or simply tell the users that this isn't allowed anymore, but it's possible to trigger commands in sequence since the interface allows to manipulate the STDIN and STDOUT. I would like to open the discussion here if this is the right direction. There's a lot to do and this still a work in progress, the current patch is small and simple, but already provides building blocks in this direction. [0] https://www.postgresql.org/message-id/7606.1153326421%40sss.pgh.pa.us [1] https://www.postgresql.org/message-id/CA%2BTgmobBmWWCgPUd04NGoQ%3D_XvcidV%2BsE2F7KChEXfs8KBPg6w%40mail.gmail.com [2] https://www.postgresql.org/message-id/21292.1358698487%40sss.pgh.pa.us -- Jonathan Gonzalez V. EDB https://enterprisedb.com
>From 22707f3f0c5a160d9483136a198b64dc1fbb9d82 Mon Sep 17 00:00:00 2001 From: "Jonathan Gonzalez V." <[email protected]> Date: Mon, 3 Aug 2026 15:21:45 -0400 Subject: [PATCH 1/1] replace calls to system() with PostgreSQL own implementation Introduce PCommand and psystem() to execute commands from argument arrays, avoiding the shell on Unix. Windows implementation still on a work in progress --- src/backend/utils/init/postinit.c | 58 --------- src/bin/pg_ctl/pg_ctl.c | 30 ++--- src/common/Makefile | 1 + src/common/exec.c | 59 +++++++++ src/common/meson.build | 2 + src/common/pg_exec.c | 206 ++++++++++++++++++++++++++++++ src/include/common/pg_exec.h | 39 ++++++ src/include/miscadmin.h | 1 - src/include/port.h | 2 + 9 files changed, 324 insertions(+), 74 deletions(-) create mode 100644 src/common/pg_exec.c create mode 100644 src/include/common/pg_exec.h diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c index 3d8c9bdebd5..78ea7d56022 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -492,64 +492,6 @@ CheckMyDatabase(const char *name, bool am_superuser, bool override_allow_connect ReleaseSysCache(tup); } - -/* - * pg_split_opts -- split a string of options and append it to an argv array - * - * The caller is responsible for ensuring the argv array is large enough. The - * maximum possible number of arguments added by this routine is - * (strlen(optstr) + 1) / 2. - * - * Because some option values can contain spaces we allow escaping using - * backslashes, with \\ representing a literal backslash. - */ -void -pg_split_opts(char **argv, int *argcp, const char *optstr) -{ - StringInfoData s; - - initStringInfo(&s); - - while (*optstr) - { - bool last_was_escape = false; - - resetStringInfo(&s); - - /* skip over leading space */ - while (isspace((unsigned char) *optstr)) - optstr++; - - if (*optstr == '\0') - break; - - /* - * Parse a single option, stopping at the first space, unless it's - * escaped. - */ - while (*optstr) - { - if (isspace((unsigned char) *optstr) && !last_was_escape) - break; - - if (!last_was_escape && *optstr == '\\') - last_was_escape = true; - else - { - last_was_escape = false; - appendStringInfoChar(&s, *optstr); - } - - optstr++; - } - - /* now store the option in the next argv[] position */ - argv[(*argcp)++] = pstrdup(s.data); - } - - pfree(s.data); -} - /* * Initialize MaxBackends value from config options. * diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c index 6c604e2d962..19e884bac1f 100644 --- a/src/bin/pg_ctl/pg_ctl.c +++ b/src/bin/pg_ctl/pg_ctl.c @@ -25,6 +25,7 @@ #include "common/controldata_utils.h" #include "common/file_perm.h" #include "common/logging.h" +#include "common/pg_exec.h" #include "common/string.h" #include "datatype/timestamp.h" #include "getopt_long.h" @@ -42,6 +43,7 @@ typedef enum IMMEDIATE_MODE, } ShutdownMode; + typedef enum { POSTMASTER_READY, @@ -904,26 +906,18 @@ find_other_exec_or_die(const char *argv0, const char *target, const char *versio static void do_init(void) { - char *cmd; + PCommand *cmd; if (exec_path == NULL) exec_path = find_other_exec_or_die(argv0, "initdb", "initdb (PostgreSQL) " PG_VERSION "\n"); - if (pgdata_opt == NULL) - pgdata_opt = ""; - - if (post_opts == NULL) - post_opts = ""; - - if (!silent_mode) - cmd = psprintf("\"%s\" %s%s", - exec_path, pgdata_opt, post_opts); - else - cmd = psprintf("\"%s\" %s%s > \"%s\"", - exec_path, pgdata_opt, post_opts, DEVNULL); + cmd = pcommand_init(exec_path, "initdb"); + pcommand_append_arg(cmd, pgdata_opt); + pcommand_append_arg(cmd, post_opts); + cmd->silent = silent_mode; fflush(NULL); - if (system(cmd) != 0) + if (psystem(cmd) != 0) { write_stderr(_("%s: database system initialization failed\n"), progname); exit(1); @@ -2155,6 +2149,12 @@ adjust_data_dir(void) else my_exec_path = pg_strdup(exec_path); +/* + * command = pcommand_init(my_exec_path, "postgres"); + * pcommand_append_arg(command, "-C data_directory"); + * pcommand_append_arg(command, pgdata_opt); + * pcommand_append_arg(command, post_opts); + */ /* it's important for -C to be the first option, see main.c */ cmd = psprintf("\"%s\" -C data_directory %s%s", my_exec_path, @@ -2287,7 +2287,7 @@ main(int argc, char **argv) * We could pass PGDATA just in an environment variable * but we do -D too for clearer postmaster 'ps' display */ - pgdata_opt = psprintf("-D \"%s\" ", pgdata_D); + pgdata_opt = psprintf("-D %s ", pgdata_D); pg_free(pgdata_D); break; } diff --git a/src/common/Makefile b/src/common/Makefile index 1a2fbbe887f..753399f023a 100644 --- a/src/common/Makefile +++ b/src/common/Makefile @@ -68,6 +68,7 @@ OBJS_COMMON = \ md5_common.o \ parse_manifest.o \ percentrepl.o \ + pg_exec.o \ pg_get_line.o \ pg_lzcompress.o \ pg_prng.o \ diff --git a/src/common/exec.c b/src/common/exec.c index 2881aa92ca6..c5a31c17a95 100644 --- a/src/common/exec.c +++ b/src/common/exec.c @@ -33,6 +33,7 @@ #include <sys/stat.h> #include <sys/wait.h> #include <unistd.h> +#include <fcntl.h> #ifdef EXEC_BACKEND #if defined(HAVE_SYS_PERSONALITY_H) @@ -43,6 +44,7 @@ #endif #include "common/string.h" +#include "lib/stringinfo.h" /* Inhibit mingw CRT's auto-globbing of command line arguments */ #if defined(WIN32) && !defined(_MSC_VER) @@ -711,3 +713,60 @@ GetTokenUser(HANDLE hToken, PTOKEN_USER *ppTokenUser) } #endif + +/* + * pg_split_opts -- split a string of options and append it to an argv array + * + * The caller is responsible for ensuring the argv array is large enough. The + * maximum possible number of arguments added by this routine is + * (strlen(optstr) + 1) / 2. + * + * Because some option values can contain spaces we allow escaping using + * backslashes, with \\ representing a literal backslash. + */ +void +pg_split_opts(char **argv, int *argcp, const char *optstr) +{ + StringInfoData s; + + initStringInfo(&s); + + while (*optstr) + { + bool last_was_escape = false; + + resetStringInfo(&s); + + /* skip over leading space */ + while (isspace((unsigned char) *optstr)) + optstr++; + + if (*optstr == '\0') + break; + + /* + * Parse a single option, stopping at the first space, unless it's + * escaped. + */ + while (*optstr) + { + if (isspace((unsigned char) *optstr) && !last_was_escape) + break; + + if (!last_was_escape && *optstr == '\\') + last_was_escape = true; + else + { + last_was_escape = false; + appendStringInfoChar(&s, *optstr); + } + + optstr++; + } + + /* now store the option in the next argv[] position */ + argv[(*argcp)++] = pstrdup(s.data); + } + + pfree(s.data); +} diff --git a/src/common/meson.build b/src/common/meson.build index 9bd55cda95b..06ebb4dd874 100644 --- a/src/common/meson.build +++ b/src/common/meson.build @@ -22,6 +22,7 @@ common_sources = files( 'md5_common.c', 'parse_manifest.c', 'percentrepl.c', + 'pg_exec.c', 'pg_get_line.c', 'pg_lzcompress.c', 'pg_prng.c', @@ -190,6 +191,7 @@ foreach name, opts : pgcommon_variants kwargs: opts + { 'include_directories': [ include_directories('.'), + include_directories('../interfaces/libpq'), opts.get('include_directories', []), ], 'dependencies': opts['dependencies'] + [ssl], diff --git a/src/common/pg_exec.c b/src/common/pg_exec.c new file mode 100644 index 00000000000..bb5613216f2 --- /dev/null +++ b/src/common/pg_exec.c @@ -0,0 +1,206 @@ +/*------------------------------------------------------------------------- + * + * pg_exec.c + * Functions fo execute and manage the input/output of commands + * + * + * Portions Copyright (c) 1996-2026, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * + * IDENTIFICATION + * src/common/pg_exec.c + * + *------------------------------------------------------------------------- + */ + +#include <unistd.h> +#include <sys/wait.h> +#include <fcntl.h> + +#include "c.h" + +#include "postgres.h" +#include "common/pg_exec.h" +#include "utils/palloc.h" + +#ifdef WIN32 +#include "fe_utils/string_utils.h" +#endif + +static int +pcommand_count_args(const char *arg) +{ + int count = 0; + bool in_word = false; + + Assert(arg != NULL); + + while (*arg) + { + if (isspace((unsigned char) *arg)) + in_word = false; + else if (!in_word) + { + count++; + in_word = true; + } + + arg++; + } + + return count; +} + +void +pcommand_append_arg(PCommand * cmd, const char *arg) +{ + int args_count; + + if (arg == NULL) + return; + + args_count = pcommand_count_args(arg); + + if (args_count == 0) + return; + + if (cmd->argc + args_count >= cmd->nalloc) + { + cmd->nalloc += args_count + 1; + cmd->argv = repalloc_array(cmd->argv, char *, cmd->nalloc); + } + + if (args_count > 1) + { + pg_split_opts(cmd->argv, &cmd->argc, arg); + cmd->argv[cmd->argc] = NULL; + } + else + { + + cmd->argv[cmd->argc++] = pstrdup(arg); + cmd->argv[cmd->argc] = NULL; + } +} + + +#ifdef WIN32 +int +pcommand_win(PCommand * cmd) +{ + PQExpBufferData cmd_str; + int i; + + initPQExpBuffer(&cmd_str); + + appendShellString(&cmd_str, cmd->path); + + for (i = 1; i < cmd->argc; i++) + { + appendPQExpBufferChar(&cmd_str, ' '); + appendShellString(&cmd_str, cmd->argv[i]); + } + + if (cmd->silent) + { + appendPQExpBufferStr(&cmd_str, " > "); + appendShellString(&cmd_str, DEVNULL); + } + + return system(cmd_str.data); +} +#endif + +PCommand * +pcommand_init(char *path, char *command) +{ + PCommand *cmd = palloc(sizeof(PCommand)); + + cmd->path = path; + cmd->command = command; + + cmd->argc = 0; + cmd->nalloc = 2; + cmd->argv = palloc_array(char *, cmd->nalloc); + + pcommand_append_arg(cmd, command); + + cmd->stdin_fd = -1; + cmd->stdout_fd = -1; + cmd->stderr_fd = -1; + + return cmd; +} + +int +pcommand_exec(PCommand * cmd) +{ + if (cmd->stdin_fd >= 0 && dup2(cmd->stdin_fd, STDIN_FILENO) < 0) + return errno; + if (cmd->stdout_fd >= 0 && dup2(cmd->stdout_fd, STDOUT_FILENO) < 0) + return errno; + if (cmd->stderr_fd >= 0 && dup2(cmd->stderr_fd, STDERR_FILENO) < 0) + return errno; + + execv(cmd->path, cmd->argv); + return errno; +} + + +#ifndef WIN32 +int +pcommand_wait(PCommand * cmd) +{ + int status = -1; + + /* This should have some kind of timeout... do we have that somewhere else */ + while (true) + { + if (waitpid(cmd->pid, &status, 0) < 0) + { + if (errno == EINTR) + continue; + return status; + } + else + { + return status; + } + } +} +#endif + +/* + * psystem() is a replacement for system(3) that avoids the use of a shell + * It's not a full replacement yet since it doesn't support pipes. For now it + * should be used only in places where is known that no pipe is requried and the + * commands being call are known commands. + * This funciton shouldn't be used as a replacement for system(3) call in places + * like RestoreArchivedFile() or shell_archive_file() + * + * The return value is always the return of the executed command + */ +int +psystem(PCommand * cmd) +{ +#ifdef WIN32 + return pcommand_win(cmd); +#else + + cmd->pid = fork(); + if (cmd->pid < 0) + return -1; + + if (cmd->pid == 0) + { + if (cmd->silent) + cmd->stdout_fd = open(DEVNULL, O_WRONLY); + + errno = pcommand_exec(cmd); + _exit(127); + } + + return pcommand_wait(cmd); +#endif +} diff --git a/src/include/common/pg_exec.h b/src/include/common/pg_exec.h new file mode 100644 index 00000000000..d987449c615 --- /dev/null +++ b/src/include/common/pg_exec.h @@ -0,0 +1,39 @@ +/*------------------------------------------------------------------------- + * Exec commands for backend/frontend programs + * + * Copyright (c) 2018-2026, PostgreSQL Global Development Group + * + * src/include/common/pg_exec.h + * + *------------------------------------------------------------------------- + */ +#ifndef COMMON_PG_EXEC_H +#define COMMON_PG_EXEC_H + +typedef struct PCommand +{ + pid_t pid; + char *path; + char *command; + char *command_win; + char **argv; + int argc; + int nalloc; + bool silent; + int stdin_fd; + int stdout_fd; + int stderr_fd; +} PCommand; + +extern PCommand * pcommand_init(char *path, char *command); +extern void pcommand_append_arg(PCommand * cmd, const char *arg); +#ifdef WIN32 +extern int pcommand_win(PCommand *cmd); +#endif +#ifndef WIN32 +extern int pcommand_wait(PCommand *cmd); +#endif +extern int pcommand_exec(PCommand * cmd); +extern int psystem(PCommand * command); + +#endif diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 0fc59af02b9..41d4c62c531 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -512,7 +512,6 @@ extern PGDLLIMPORT ProcessingMode Mode; #define INIT_PG_LOAD_SESSION_LIBS 0x0001 #define INIT_PG_OVERRIDE_ALLOW_CONNS 0x0002 #define INIT_PG_OVERRIDE_ROLE_LOGIN 0x0004 -extern void pg_split_opts(char **argv, int *argcp, const char *optstr); extern void InitializeMaxBackends(void); extern void InitializeFastPathLocks(void); extern void InitPostgres(const char *in_dbname, Oid dboid, diff --git a/src/include/port.h b/src/include/port.h index 172acf7d02f..82d900fcbc4 100644 --- a/src/include/port.h +++ b/src/include/port.h @@ -140,6 +140,8 @@ extern int find_my_exec(const char *argv0, char *retpath); extern int find_other_exec(const char *argv0, const char *target, const char *versionstr, char *retpath); extern char *pipe_read_line(char *cmd); +extern void pg_split_opts(char **argv, int *argcp, const char *optstr); + /* Doesn't belong here, but this is used with find_other_exec(), so... */ #define PG_BACKEND_VERSIONSTR "postgres (PostgreSQL) " PG_VERSION "\n" -- 2.53.0
