[PATCH] pg_ctl should not truncate command lines at 1024 characters
Hello,
Lacking a tool to edit postgresql.conf programmatically, people resort
to passing cluster options on the command line. While passing all
non-default options in this way may sound like an abuse of the feature,
IMHO pg_ctl should not blindly truncate generated command lines at
MAXPGPATH (1024 characters) and then run that, resulting in:
/bin/sh: Syntax error: end of file unexpected (expecting word)
pg_ctl: could not start server
Examine the log output.
The attached patch tries to fix it in the least intrusive way.
While we're at it, is it supposed that pg_ctl is a very short-lived
process and is therefore allowed to leak memory? I've noticed some
places where I would like to add a free() call.
-- Ph.
Attachments:
0001-pg_ctl-should-not-truncate-command-lines-at-1024-cha.patchtext/x-diff; name=0001-pg_ctl-should-not-truncate-command-lines-at-1024-cha.patchDownload
From 6f2e70025208fa4d0034ddbe5254e2cb9759dd24 Mon Sep 17 00:00:00 2001
From: Phil Krylov <phil@krylov.eu>
Date: Thu, 2 Sep 2021 21:39:58 +0200
Subject: [PATCH] pg_ctl should not truncate command lines at 1024 characters
---
src/bin/pg_ctl/pg_ctl.c | 47 ++++++++++++++++++++++----------------
src/bin/pg_ctl/t/001_start_stop.pl | 41 ++++++++++++++++++++++++++++++++-
2 files changed, 67 insertions(+), 21 deletions(-)
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 7985da0a94..199d13c1fb 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -442,7 +442,7 @@ free_readfile(char **optlines)
static pgpid_t
start_postmaster(void)
{
- char cmd[MAXPGPATH];
+ char *cmd;
#ifndef WIN32
pgpid_t pm_pid;
@@ -487,14 +487,15 @@ start_postmaster(void)
* has the same PID as the current child process.
*/
if (log_file != NULL)
- snprintf(cmd, MAXPGPATH, "exec \"%s\" %s%s < \"%s\" >> \"%s\" 2>&1",
- exec_path, pgdata_opt, post_opts,
- DEVNULL, log_file);
+ cmd = psprintf("exec \"%s\" %s%s < \"%s\" >> \"%s\" 2>&1",
+ exec_path, pgdata_opt, post_opts,
+ DEVNULL, log_file);
else
- snprintf(cmd, MAXPGPATH, "exec \"%s\" %s%s < \"%s\" 2>&1",
- exec_path, pgdata_opt, post_opts, DEVNULL);
+ cmd = psprintf("exec \"%s\" %s%s < \"%s\" 2>&1",
+ exec_path, pgdata_opt, post_opts, DEVNULL);
(void) execl("/bin/sh", "/bin/sh", "-c", cmd, (char *) NULL);
+ free(cmd);
/* exec failed */
write_stderr(_("%s: could not start server: %s\n"),
@@ -553,19 +554,21 @@ start_postmaster(void)
else
close(fd);
- snprintf(cmd, MAXPGPATH, "\"%s\" /C \"\"%s\" %s%s < \"%s\" >> \"%s\" 2>&1\"",
- comspec, exec_path, pgdata_opt, post_opts, DEVNULL, log_file);
+ cmd = psprintf("\"%s\" /C \"\"%s\" %s%s < \"%s\" >> \"%s\" 2>&1\"",
+ comspec, exec_path, pgdata_opt, post_opts, DEVNULL, log_file);
}
else
- snprintf(cmd, MAXPGPATH, "\"%s\" /C \"\"%s\" %s%s < \"%s\" 2>&1\"",
- comspec, exec_path, pgdata_opt, post_opts, DEVNULL);
+ cmd = psprintf("\"%s\" /C \"\"%s\" %s%s < \"%s\" 2>&1\"",
+ comspec, exec_path, pgdata_opt, post_opts, DEVNULL);
if (!CreateRestrictedProcess(cmd, &pi, false))
{
write_stderr(_("%s: could not start server: error code %lu\n"),
progname, (unsigned long) GetLastError());
+ free(cmd);
exit(1);
}
+ free(cmd);
/* Don't close command process handle here; caller must do so */
postmasterProcess = pi.hProcess;
CloseHandle(pi.hThread);
@@ -828,7 +831,7 @@ find_other_exec_or_die(const char *argv0, const char *target, const char *versio
static void
do_init(void)
{
- char cmd[MAXPGPATH];
+ char *cmd;
if (exec_path == NULL)
exec_path = find_other_exec_or_die(argv0, "initdb", "initdb (PostgreSQL) " PG_VERSION "\n");
@@ -840,17 +843,19 @@ do_init(void)
post_opts = "";
if (!silent_mode)
- snprintf(cmd, MAXPGPATH, "\"%s\" %s%s",
- exec_path, pgdata_opt, post_opts);
+ cmd = psprintf("\"%s\" %s%s",
+ exec_path, pgdata_opt, post_opts);
else
- snprintf(cmd, MAXPGPATH, "\"%s\" %s%s > \"%s\"",
- exec_path, pgdata_opt, post_opts, DEVNULL);
+ cmd = psprintf("\"%s\" %s%s > \"%s\"",
+ exec_path, pgdata_opt, post_opts, DEVNULL);
if (system(cmd) != 0)
{
write_stderr(_("%s: database system initialization failed\n"), progname);
+ free(cmd);
exit(1);
}
+ free(cmd);
}
static void
@@ -2175,7 +2180,7 @@ set_starttype(char *starttypeopt)
static void
adjust_data_dir(void)
{
- char cmd[MAXPGPATH],
+ char *cmd,
filename[MAXPGPATH],
*my_exec_path;
FILE *fd;
@@ -2207,18 +2212,20 @@ adjust_data_dir(void)
my_exec_path = pg_strdup(exec_path);
/* it's important for -C to be the first option, see main.c */
- snprintf(cmd, MAXPGPATH, "\"%s\" -C data_directory %s%s",
- my_exec_path,
- pgdata_opt ? pgdata_opt : "",
- post_opts ? post_opts : "");
+ cmd = psprintf("\"%s\" -C data_directory %s%s",
+ my_exec_path,
+ pgdata_opt ? pgdata_opt : "",
+ post_opts ? post_opts : "");
fd = popen(cmd, "r");
if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL)
{
write_stderr(_("%s: could not determine the data directory using command \"%s\"\n"), progname, cmd);
+ free(cmd);
exit(1);
}
pclose(fd);
+ free(cmd);
free(my_exec_path);
/* strip trailing newline and carriage return */
diff --git a/src/bin/pg_ctl/t/001_start_stop.pl b/src/bin/pg_ctl/t/001_start_stop.pl
index 1d8d6bbb70..da05cc8f8b 100644
--- a/src/bin/pg_ctl/t/001_start_stop.pl
+++ b/src/bin/pg_ctl/t/001_start_stop.pl
@@ -9,7 +9,7 @@ use Fcntl ':mode';
use File::stat qw{lstat};
use PostgresNode;
use TestLib;
-use Test::More tests => 24;
+use Test::More tests => 26;
my $tempdir = TestLib::tempdir;
my $tempdir_short = TestLib::tempdir_short;
@@ -68,6 +68,45 @@ command_ok([ 'pg_ctl', 'stop', '-D', "$tempdir/data" ], 'pg_ctl stop');
command_fails([ 'pg_ctl', 'stop', '-D', "$tempdir/data" ],
'second pg_ctl stop fails');
+command_ok([ 'pg_ctl', 'start', '-D', "$tempdir/data",
+ '-w', '-s', '-m', 'fast',
+ '-o', '-c', '-o', 'update_process_title=on',
+ '-o', '-c', '-o', 'effective_io_concurrency=0',
+ '-o', '-c', '-o', 'synchronous_commit=off',
+ '-o', '-c', '-o', 'full_page_writes=off',
+ '-o', '-c', '-o', 'wal_init_zero=off',
+ '-o', '-c', '-o', 'wal_recycle=off',
+ '-o', '-c', '-o', 'effective_cache_size=3200MB',
+ '-o', '-c', '-o', 'checkpoint_completion_target=0.9',
+ '-o', '-c', '-o', 'random_page_cost=1.1',
+ '-o', '-c', '-o', 'parallel_setup_cost=200.0',
+ '-o', '-c', '-o', 'log_min_duration_statement=100',
+ '-o', '-c', '-o', 'log_checkpoints=on',
+ '-o', '-c', '-o', 'log_temp_files=0',
+ '-o', '-c', '-o', 'temp_buffers=32MB',
+ '-o', '-c', '-o', 'work_mem=50MB',
+ '-o', '-c', '-o', 'maintenance_work_mem=50MB',
+ '-o', '-c', '-o', 'max_wal_size=500MB',
+ '-o', '-c', '-o', 'default_statistics_target=10000',
+ '-o', '-c', '-o', 'autovacuum_max_workers=8',
+ '-o', '-c', '-o', 'autovacuum_naptime=15min',
+ '-o', '-c', '-o', 'autovacuum_vacuum_threshold=1000',
+ '-o', '-c', '-o', 'autovacuum_analyze_threshold=1000',
+ '-o', '-c', '-o', 'autovacuum_freeze_max_age=1000000000',
+ '-o', '-c', '-o', 'vacuum_freeze_min_age=10000000',
+ '-o', '-c', '-o', 'vacuum_freeze_table_age=800000000',
+ '-o', '-c', '-o', 'jit_above_cost=-1',
+ '-o', '-c', '-o', 'jit_optimize_above_cost=10000000',
+ '-o', '-c', '-o', 'max_locks_per_transaction=128',
+ '-o', '-c', '-o', 'default_text_search_config=pg_catalog.english',
+ '-o', '-c', '-o', 'timezone=Europe/London',
+ '-o', '-c', '-o', 'shared_buffers=400MB',
+ '-o', '-c', '-o', 'port=5433',
+ '-o', '-c', '-o', 'cluster_name=13/mytestcluster',
+ '-o', '-c', '-o', 'max_connections=200' ],
+ 'pg_ctl start with lots of parameters');
+command_ok([ 'pg_ctl', 'stop', '-D', "$tempdir/data" ], 'pg_ctl stop');
+
# Log file for default permission test. The permissions won't be checked on
# Windows but we still want to do the restart test.
my $logFileName = "$tempdir/data/perm-test-600.log";
--
2.15.2 (Apple Git-101.1)
Em qui., 2 de set. de 2021 às 18:36, Phil Krylov <phil@krylov.eu> escreveu:
Hello,
Lacking a tool to edit postgresql.conf programmatically, people resort
to passing cluster options on the command line. While passing all
non-default options in this way may sound like an abuse of the feature,
IMHO pg_ctl should not blindly truncate generated command lines at
MAXPGPATH (1024 characters) and then run that, resulting in:
The msvc docs says that limit for the command line is 32,767 characters,
while ok for me, I think if not it would be better to check this limit?
/bin/sh: Syntax error: end of file unexpected (expecting word)
pg_ctl: could not start server
Examine the log output.The attached patch tries to fix it in the least intrusive way.
While we're at it, is it supposed that pg_ctl is a very short-lived
process and is therefore allowed to leak memory? I've noticed some
places where I would like to add a free() call.
+1 to add free.
regards,
Ranier Vilela
On 2021-09-03 00:36, Ranier Vilela wrote:
The msvc docs says that limit for the command line is 32,767
characters,
while ok for me, I think if not it would be better to check this limit?
Well, it's ARG_MAX in POSIX, and ARG_MAX is defined as 256K in Darwin,
512K in FreeBSD, 128K in Linux; _POSIX_ARG_MAX is defined as 4096 on all
three platforms. Windows may differ too. Anyways, allocating even 128K
in precious stack space is too much, that's why I suggest to use
psprintf(). As for checking any hard limit, I don't think it would have
much value - somehow we got the original command line, thus it is
supported by the system, so we can just pass it on.
-- Ph.
Phil Krylov <phil@krylov.eu> writes:
IMHO pg_ctl should not blindly truncate generated command lines at
MAXPGPATH (1024 characters) and then run that, resulting in:
Fair enough.
The attached patch tries to fix it in the least intrusive way.
Seems reasonable. We didn't have psprintf when this code was written,
but now that we do, it's hardly any more complicated to do it without
the length restriction.
While we're at it, is it supposed that pg_ctl is a very short-lived
process and is therefore allowed to leak memory? I've noticed some
places where I would like to add a free() call.
I think that these free() calls you propose to add are a complete
waste of code space. Certainly a free() right before an exit() call
is that; if anything, it's *delaying* recycling the memory space for
some useful purpose. But no part of pg_ctl runs long enough for it
to be worth worrying about small leaks.
I do not find your proposed test case to be a useful expenditure
of test cycles, either. If it ever fails, we'd learn nothing,
except that that particular platform has a surprisingly small
command line length limit.
regards, tom lane
On 2021-09-03 02:09, Tom Lane wrote:
I think that these free() calls you propose to add are a complete
waste of code space. Certainly a free() right before an exit() call
is that; if anything, it's *delaying* recycling the memory space for
some useful purpose. But no part of pg_ctl runs long enough for it
to be worth worrying about small leaks.
OK, I have removed the free() before exit().
I do not find your proposed test case to be a useful expenditure
of test cycles, either. If it ever fails, we'd learn nothing,
except that that particular platform has a surprisingly small
command line length limit.
Hmm, it's a test case that fails with the current code and stops failing
with my fix, so I've put it there to show the problem. But, truly, it
does not bring much value after the fix is applied.
Attaching the new version, with the test case and free-before-exit
removed.
-- Ph.
Attachments:
0001-pg_ctl-should-not-truncate-command-lines-at-1024-cha.patchtext/x-diff; name=0001-pg_ctl-should-not-truncate-command-lines-at-1024-cha.patchDownload
From 4f8e39f3c5e39b5ec507ec4b07ad5d33c6610524 Mon Sep 17 00:00:00 2001
From: Phil Krylov <phil@krylov.eu>
Date: Thu, 2 Sep 2021 21:39:58 +0200
Subject: [PATCH] pg_ctl should not truncate command lines at 1024 characters
---
src/bin/pg_ctl/pg_ctl.c | 43 +++++++++++++++++++++++--------------------
1 file changed, 23 insertions(+), 20 deletions(-)
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 7985da0a94..e07b5f02d7 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -442,7 +442,7 @@ free_readfile(char **optlines)
static pgpid_t
start_postmaster(void)
{
- char cmd[MAXPGPATH];
+ char *cmd;
#ifndef WIN32
pgpid_t pm_pid;
@@ -487,12 +487,12 @@ start_postmaster(void)
* has the same PID as the current child process.
*/
if (log_file != NULL)
- snprintf(cmd, MAXPGPATH, "exec \"%s\" %s%s < \"%s\" >> \"%s\" 2>&1",
- exec_path, pgdata_opt, post_opts,
- DEVNULL, log_file);
+ cmd = psprintf("exec \"%s\" %s%s < \"%s\" >> \"%s\" 2>&1",
+ exec_path, pgdata_opt, post_opts,
+ DEVNULL, log_file);
else
- snprintf(cmd, MAXPGPATH, "exec \"%s\" %s%s < \"%s\" 2>&1",
- exec_path, pgdata_opt, post_opts, DEVNULL);
+ cmd = psprintf("exec \"%s\" %s%s < \"%s\" 2>&1",
+ exec_path, pgdata_opt, post_opts, DEVNULL);
(void) execl("/bin/sh", "/bin/sh", "-c", cmd, (char *) NULL);
@@ -553,12 +553,12 @@ start_postmaster(void)
else
close(fd);
- snprintf(cmd, MAXPGPATH, "\"%s\" /C \"\"%s\" %s%s < \"%s\" >> \"%s\" 2>&1\"",
- comspec, exec_path, pgdata_opt, post_opts, DEVNULL, log_file);
+ cmd = psprintf("\"%s\" /C \"\"%s\" %s%s < \"%s\" >> \"%s\" 2>&1\"",
+ comspec, exec_path, pgdata_opt, post_opts, DEVNULL, log_file);
}
else
- snprintf(cmd, MAXPGPATH, "\"%s\" /C \"\"%s\" %s%s < \"%s\" 2>&1\"",
- comspec, exec_path, pgdata_opt, post_opts, DEVNULL);
+ cmd = psprintf("\"%s\" /C \"\"%s\" %s%s < \"%s\" 2>&1\"",
+ comspec, exec_path, pgdata_opt, post_opts, DEVNULL);
if (!CreateRestrictedProcess(cmd, &pi, false))
{
@@ -566,6 +566,7 @@ start_postmaster(void)
progname, (unsigned long) GetLastError());
exit(1);
}
+ free(cmd);
/* Don't close command process handle here; caller must do so */
postmasterProcess = pi.hProcess;
CloseHandle(pi.hThread);
@@ -828,7 +829,7 @@ find_other_exec_or_die(const char *argv0, const char *target, const char *versio
static void
do_init(void)
{
- char cmd[MAXPGPATH];
+ char *cmd;
if (exec_path == NULL)
exec_path = find_other_exec_or_die(argv0, "initdb", "initdb (PostgreSQL) " PG_VERSION "\n");
@@ -840,17 +841,18 @@ do_init(void)
post_opts = "";
if (!silent_mode)
- snprintf(cmd, MAXPGPATH, "\"%s\" %s%s",
- exec_path, pgdata_opt, post_opts);
+ cmd = psprintf("\"%s\" %s%s",
+ exec_path, pgdata_opt, post_opts);
else
- snprintf(cmd, MAXPGPATH, "\"%s\" %s%s > \"%s\"",
- exec_path, pgdata_opt, post_opts, DEVNULL);
+ cmd = psprintf("\"%s\" %s%s > \"%s\"",
+ exec_path, pgdata_opt, post_opts, DEVNULL);
if (system(cmd) != 0)
{
write_stderr(_("%s: database system initialization failed\n"), progname);
exit(1);
}
+ free(cmd);
}
static void
@@ -2175,7 +2177,7 @@ set_starttype(char *starttypeopt)
static void
adjust_data_dir(void)
{
- char cmd[MAXPGPATH],
+ char *cmd,
filename[MAXPGPATH],
*my_exec_path;
FILE *fd;
@@ -2207,10 +2209,10 @@ adjust_data_dir(void)
my_exec_path = pg_strdup(exec_path);
/* it's important for -C to be the first option, see main.c */
- snprintf(cmd, MAXPGPATH, "\"%s\" -C data_directory %s%s",
- my_exec_path,
- pgdata_opt ? pgdata_opt : "",
- post_opts ? post_opts : "");
+ cmd = psprintf("\"%s\" -C data_directory %s%s",
+ my_exec_path,
+ pgdata_opt ? pgdata_opt : "",
+ post_opts ? post_opts : "");
fd = popen(cmd, "r");
if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL)
@@ -2219,6 +2221,7 @@ adjust_data_dir(void)
exit(1);
}
pclose(fd);
+ free(cmd);
free(my_exec_path);
/* strip trailing newline and carriage return */
--
2.15.2 (Apple Git-101.1)