agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedIntroduce psystem() to replace system()
6+ messages / 5 participants
[nested] [flat]
* Introduce psystem() to replace system()
@ 2026-08-10 19:27 Jonathan Gonzalez V. <jonathan@abdiel.eu>
0 siblings, 2 replies; 6+ messages in thread
From: Jonathan Gonzalez V. @ 2026-08-10 19:27 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
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%40ma...
[2] https://www.postgresql.org/message-id/21292.1358698487%40sss.pgh.pa.us
--
Jonathan Gonzalez V.
EDB https://enterprisedb.com
Attachments:
[text/x-diff] 0001-replace-calls-to-system-with-PostgreSQL-own-implemen.patch (13.7K, ../../87zeyt93ns.fsf@abdiel.eu/2-0001-replace-calls-to-system-with-PostgreSQL-own-implemen.patch)
download | inline diff:
From 22707f3f0c5a160d9483136a198b64dc1fbb9d82 Mon Sep 17 00:00:00 2001
From: "Jonathan Gonzalez V." <jonathan@abdiel.eu>
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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Introduce psystem() to replace system()
@ 2026-08-10 23:00 Tom Lane <tgl@sss.pgh.pa.us>
parent: Jonathan Gonzalez V. <jonathan@abdiel.eu>
1 sibling, 2 replies; 6+ messages in thread
From: Tom Lane @ 2026-08-10 23:00 UTC (permalink / raw)
To: Jonathan Gonzalez V. <jonathan@abdiel.eu>; +Cc: pgsql-hackers@lists.postgresql.org
"Jonathan Gonzalez V." <jonathan@abdiel.eu> writes:
> 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.
There's a lot to be said for not going through system() if we don't
have to, and I think you are right that there are many places where
we don't have to, if we're willing to write our own stdio-redirection
code (but that might be a bigger can of worms than it seems). That'd
improve security and also performance, though I'm not very sure how
big the latter win would be.
However, I think your apparent ambition to have *zero* use of system()
is a bridge too far. In particular:
> 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.
I think breaking the existing definition of archive_command and
similar GUCs is a nonstarter. You're going to make many users
unhappy and only a tiny minority happier.
There might be some way to compromise, along the lines of "if the
string contains no shell metacharacters then parse it ourselves and
use execv(), else use system()". The devil's in the details there;
but if it could work then it'd satisfy people who'd like to not
have a shell available and are willing to deal with the ensuing
restrictions. But if you think that description covers all or even
most of our users, I'm here to tell you you're wrong.
As for details ... doesn't this pcommand_count_args thing break
instantly on cases like pathnames containing spaces? I think
you need a much clearer concept (and, um, some documentation)
about the semantics of these functions. I don't think we're
really going to move the goalposts far unless we can get away
from assumptions like that.
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Introduce psystem() to replace system()
@ 2026-08-11 01:11 Haibo Yan <tristan.yim@gmail.com>
parent: Jonathan Gonzalez V. <jonathan@abdiel.eu>
1 sibling, 0 replies; 6+ messages in thread
From: Haibo Yan @ 2026-08-11 01:11 UTC (permalink / raw)
To: Jonathan Gonzalez V. <jonathan@abdiel.eu>; +Cc: pgsql-hackers@lists.postgresql.org
On Mon, Aug 10, 2026 at 12:28 PM Jonathan Gonzalez V. <jonathan@abdiel.eu>
wrote:
>
> 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.
>
> Hi Jonathan
Thanks for working on this. I think avoiding the shell for cases where
PostgreSQL is just
invoking a known executable makes sense.
There is some related work here which may be worth looking at:
https://www.postgresql.org/message-id/flat/CAGECzQQh6VSy3KG4pN1d%3Dh9J%3DD1rStFCMR%2Bt7yh_Kwj-g87aLQ...
That effort ran into some similar issues around replacing system()/popen()
and handling the
fork/exec boundary.
A few things I noticed in this patch:
1. system() and execv() do not have the same semantics. For internal
commands such as pg_ctl
invoking initdb, that is probably fine and preferable. But for things like
archive_command,
shell features such as PATH lookup, &&, pipes and redirection are part of
the existing
interface, so changing those would be a compatibility decision rather than
just an
implementation change.
2. For pipelines, pcommand_exec() only knows the stdin/stdout/stderr FDs.
After fork(),
however, a child may inherit other pipe endpoints as well. Those need to be
closed before
exec(), otherwise an unused write end can keep a pipe alive and prevent EOF
from being seen.
3. I also wonder about returning errno from pcommand_exec(). An execv()
failure should
probably remain distinguishable from a program that successfully execs and
later exits with
the same numeric status. An error pipe from child to parent may be useful
here.
Overall I like the direction, but I think it would be useful to reuse some
of the lessons
from the earlier pg_system() / pg_popen() work before this grows into a
more general process
/pipeline API.
Regards,
Haibo
> [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%40ma...
> [2] https://www.postgresql.org/message-id/21292.1358698487%40sss.pgh.pa.us
>
> --
> Jonathan Gonzalez V.
> EDB https://enterprisedb.com
>
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Introduce psystem() to replace system()
@ 2026-08-12 17:05 Peter Eisentraut <peter@eisentraut.org>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 6+ messages in thread
From: Peter Eisentraut @ 2026-08-12 17:05 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Jonathan Gonzalez V. <jonathan@abdiel.eu>; +Cc: pgsql-hackers@lists.postgresql.org
On 11.08.26 01:00, Tom Lane wrote:
> However, I think your apparent ambition to have*zero* use of system()
> is a bridge too far. In particular:
>
>> 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.
> I think breaking the existing definition of archive_command and
> similar GUCs is a nonstarter. You're going to make many users
> unhappy and only a tiny minority happier.
Right. The direction should be, you don't have to use archive_command.
There is already an alternative in archive_library, or indirectly by
using something like pg_receivewal. Similar alternatives could be
designed for other cases where a shell command appears in a public API.
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Introduce psystem() to replace system()
@ 2026-08-12 17:12 Nathan Bossart <nathandbossart@gmail.com>
parent: Peter Eisentraut <peter@eisentraut.org>
0 siblings, 0 replies; 6+ messages in thread
From: Nathan Bossart @ 2026-08-12 17:12 UTC (permalink / raw)
To: Peter Eisentraut <peter@eisentraut.org>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Jonathan Gonzalez V. <jonathan@abdiel.eu>; pgsql-hackers@lists.postgresql.org
On Wed, Aug 12, 2026 at 07:05:36PM +0200, Peter Eisentraut wrote:
> On 11.08.26 01:00, Tom Lane wrote:
>> I think breaking the existing definition of archive_command and
>> similar GUCs is a nonstarter. You're going to make many users
>> unhappy and only a tiny minority happier.
>
> Right. The direction should be, you don't have to use archive_command.
> There is already an alternative in archive_library, or indirectly by using
> something like pg_receivewal. Similar alternatives could be designed for
> other cases where a shell command appears in a public API.
+1. This conversation makes me wonder about reviving the "restore modules"
work [0].
[0] https://postgr.es/m/flat/20221227192449.GA3672473%40nathanxps13
--
nathan
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Introduce psystem() to replace system()
@ 2026-08-19 11:53 Jonathan Gonzalez V. <jonathan@abdiel.eu>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 6+ messages in thread
From: Jonathan Gonzalez V. @ 2026-08-19 11:53 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers@lists.postgresql.org
Hello!
Tom Lane <tgl@sss.pgh.pa.us> writes:
> "Jonathan Gonzalez V." <jonathan@abdiel.eu> writes:
>> 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.
>
> There's a lot to be said for not going through system() if we don't
> have to, and I think you are right that there are many places where
> we don't have to, if we're willing to write our own stdio-redirection
> code (but that might be a bigger can of worms than it seems). That'd
> improve security and also performance, though I'm not very sure how
> big the latter win would be.
I found a lot of discussions in the past, that's why I thought the idea
will be of interest. The stdio-redirection, it's for now, just something
for send stuff to DEVNULL, probably it can evolve in future patches, but
for now, I think that for the silent stuff it's enough, because the
redirection stuff will open the door to manage some piping and the idea
it's to remove those kind of needs too.
In terms of security, there is an improvement, but for performance, I wasn't
able to establish a base line or even what to measure, but I can imagine
that people here may have more ideas about what and how to measure it.
> However, I think your apparent ambition to have *zero* use of system()
> is a bridge too far. In particular:
>
>> 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.
>
> I think breaking the existing definition of archive_command and
> similar GUCs is a nonstarter. You're going to make many users
> unhappy and only a tiny minority happier.
>
> There might be some way to compromise, along the lines of "if the
> string contains no shell metacharacters then parse it ourselves and
> use execv(), else use system()". The devil's in the details there;
> but if it could work then it'd satisfy people who'd like to not
> have a shell available and are willing to deal with the ensuing
> restrictions. But if you think that description covers all or even
> most of our users, I'm here to tell you you're wrong.
I'm pretty sure it will not, but I open the door to think about it and
keep I'm trying to keep in mind those situations when thinking about the
design of the patches.
> As for details ... doesn't this pcommand_count_args thing break
> instantly on cases like pathnames containing spaces? I think
> you need a much clearer concept (and, um, some documentation)
> about the semantics of these functions. I don't think we're
> really going to move the goalposts far unless we can get away
> from assumptions like that.
Well, that function it's designed in case we should keep compatibility
with system(), I'm not happy with that idea but it may be required, but
if we can avoid having that from the beginning I'm more than happy with
this.
With all that said, I'll start putting some documentation and clarify
the concepts for this and have a better v2 version.
Regards!
--
Jonathan Gonzalez V.
EDB
https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2026-08-19 11:53 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-08-10 19:27 Introduce psystem() to replace system() Jonathan Gonzalez V. <jonathan@abdiel.eu>
2026-08-10 23:00 ` Tom Lane <tgl@sss.pgh.pa.us>
2026-08-12 17:05 ` Peter Eisentraut <peter@eisentraut.org>
2026-08-12 17:12 ` Nathan Bossart <nathandbossart@gmail.com>
2026-08-19 11:53 ` Jonathan Gonzalez V. <jonathan@abdiel.eu>
2026-08-11 01:11 ` Haibo Yan <tristan.yim@gmail.com>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox