On 14.12.22 17:09, Robert Haas wrote:
Well, OK, I'll tentatively cast a vote in favor of adopting basebackup_to_shell's approach elsewhere. Or to put that in plain English: I think that if the input appears to be malformed, it's better to throw an error than to guess what the user meant. In the case of basebackup_to_shell there are potentially security ramifications to the setting involved so it seemed like a bad idea to take a laissez faire approach. But also, just in general, if somebody supplies an ssl_passphrase_command or archive_command with %<something unexpected>, I don't really see why we should treat that differently than trying to start the server with work_mem=banana.
I agree. Here is an updated patch with the error checking included.
From 8bb85ad7ed99d0255ea82306b08ef7d343ab8edb Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <pe...@eisentraut.org> Date: Mon, 19 Dec 2022 09:04:43 +0100 Subject: [PATCH v2] Common function for percent placeholder replacement There are a number of places where a shell command is constructed with percent-placeholders (like %x). It's cumbersome to have to open-code this several times. This factors out this logic into a separate function. This also allows us to ensure consistency for and document some subtle behaviors, such as what to do with unrecognized placeholders. Discussion: https://www.postgresql.org/message-id/flat/5238bbed-0b01-83a6-d4b2-7eb0562a054e%40enterprisedb.com --- .../basebackup_to_shell/basebackup_to_shell.c | 55 +------- src/backend/access/transam/xlogarchive.c | 45 +------ src/backend/libpq/be-secure-common.c | 38 ++---- src/backend/postmaster/shell_archive.c | 59 ++------- src/common/Makefile | 1 + src/common/archive.c | 81 +----------- src/common/meson.build | 1 + src/common/percentrepl.c | 120 ++++++++++++++++++ src/include/common/percentrepl.h | 18 +++ 9 files changed, 173 insertions(+), 245 deletions(-) create mode 100644 src/common/percentrepl.c create mode 100644 src/include/common/percentrepl.h diff --git a/contrib/basebackup_to_shell/basebackup_to_shell.c b/contrib/basebackup_to_shell/basebackup_to_shell.c index bdaf67a4c8..9a3d57c64b 100644 --- a/contrib/basebackup_to_shell/basebackup_to_shell.c +++ b/contrib/basebackup_to_shell/basebackup_to_shell.c @@ -12,6 +12,7 @@ #include "access/xact.h" #include "backup/basebackup_target.h" +#include "common/percentrepl.h" #include "miscadmin.h" #include "storage/fd.h" #include "utils/acl.h" @@ -208,59 +209,7 @@ static char * shell_construct_command(const char *base_command, const char *filename, const char *target_detail) { - StringInfoData buf; - const char *c; - - initStringInfo(&buf); - for (c = base_command; *c != '\0'; ++c) - { - /* Anything other than '%' is copied verbatim. */ - if (*c != '%') - { - appendStringInfoChar(&buf, *c); - continue; - } - - /* Any time we see '%' we eat the following character as well. */ - ++c; - - /* - * The following character determines what we insert here, or may - * cause us to throw an error. - */ - if (*c == '%') - { - /* '%%' is replaced by a single '%' */ - appendStringInfoChar(&buf, '%'); - } - else if (*c == 'f') - { - /* '%f' is replaced by the filename */ - appendStringInfoString(&buf, filename); - } - else if (*c == 'd') - { - /* '%d' is replaced by the target detail */ - appendStringInfoString(&buf, target_detail); - } - else if (*c == '\0') - { - /* Incomplete escape sequence, expected a character afterward */ - ereport(ERROR, - errcode(ERRCODE_SYNTAX_ERROR), - errmsg("shell command ends unexpectedly after escape character \"%%\"")); - } - else - { - /* Unknown escape sequence */ - ereport(ERROR, - errcode(ERRCODE_SYNTAX_ERROR), - errmsg("shell command contains unexpected escape sequence \"%c\"", - *c)); - } - } - - return buf.data; + return replace_percent_placeholders(base_command, "df", (const char *[]){target_detail, filename}); } /* diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c index e2b7176f2f..0f74ccb49c 100644 --- a/src/backend/access/transam/xlogarchive.c +++ b/src/backend/access/transam/xlogarchive.c @@ -23,6 +23,7 @@ #include "access/xlog_internal.h" #include "access/xlogarchive.h" #include "common/archive.h" +#include "common/percentrepl.h" #include "miscadmin.h" #include "pgstat.h" #include "postmaster/startup.h" @@ -291,11 +292,8 @@ void ExecuteRecoveryCommand(const char *command, const char *commandName, bool failOnSignal, uint32 wait_event_info) { - char xlogRecoveryCmd[MAXPGPATH]; + char *xlogRecoveryCmd; char lastRestartPointFname[MAXPGPATH]; - char *dp; - char *endp; - const char *sp; int rc; XLogSegNo restartSegNo; XLogRecPtr restartRedoPtr; @@ -316,42 +314,7 @@ ExecuteRecoveryCommand(const char *command, const char *commandName, /* * construct the command to be executed */ - dp = xlogRecoveryCmd; - endp = xlogRecoveryCmd + MAXPGPATH - 1; - *endp = '\0'; - - for (sp = command; *sp; sp++) - { - if (*sp == '%') - { - switch (sp[1]) - { - case 'r': - /* %r: filename of last restartpoint */ - sp++; - strlcpy(dp, lastRestartPointFname, endp - dp); - dp += strlen(dp); - break; - case '%': - /* convert %% to a single % */ - sp++; - if (dp < endp) - *dp++ = *sp; - break; - default: - /* otherwise treat the % as not special */ - if (dp < endp) - *dp++ = *sp; - break; - } - } - else - { - if (dp < endp) - *dp++ = *sp; - } - } - *dp = '\0'; + xlogRecoveryCmd = replace_percent_placeholders(command, "r", (const char *[]){lastRestartPointFname}); ereport(DEBUG3, (errmsg_internal("executing %s \"%s\"", commandName, command))); @@ -364,6 +327,8 @@ ExecuteRecoveryCommand(const char *command, const char *commandName, rc = system(xlogRecoveryCmd); pgstat_report_wait_end(); + pfree(xlogRecoveryCmd); + if (rc != 0) { /* diff --git a/src/backend/libpq/be-secure-common.c b/src/backend/libpq/be-secure-common.c index 2719ce1403..912f12d13e 100644 --- a/src/backend/libpq/be-secure-common.c +++ b/src/backend/libpq/be-secure-common.c @@ -22,6 +22,7 @@ #include <sys/stat.h> #include <unistd.h> +#include "common/percentrepl.h" #include "common/string.h" #include "libpq/libpq.h" #include "storage/fd.h" @@ -39,8 +40,7 @@ int run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf, int size) { int loglevel = is_server_start ? ERROR : LOG; - StringInfoData command; - char *p; + char *command; FILE *fh; int pclose_rc; size_t len = 0; @@ -49,37 +49,15 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf, Assert(size > 0); buf[0] = '\0'; - initStringInfo(&command); + command = replace_percent_placeholders(ssl_passphrase_command, "p", (const char *[]){prompt}); - for (p = ssl_passphrase_command; *p; p++) - { - if (p[0] == '%') - { - switch (p[1]) - { - case 'p': - appendStringInfoString(&command, prompt); - p++; - break; - case '%': - appendStringInfoChar(&command, '%'); - p++; - break; - default: - appendStringInfoChar(&command, p[0]); - } - } - else - appendStringInfoChar(&command, p[0]); - } - - fh = OpenPipeStream(command.data, "r"); + fh = OpenPipeStream(command, "r"); if (fh == NULL) { ereport(loglevel, (errcode_for_file_access(), errmsg("could not execute command \"%s\": %m", - command.data))); + command))); goto error; } @@ -91,7 +69,7 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf, ereport(loglevel, (errcode_for_file_access(), errmsg("could not read from command \"%s\": %m", - command.data))); + command))); goto error; } } @@ -111,7 +89,7 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf, ereport(loglevel, (errcode_for_file_access(), errmsg("command \"%s\" failed", - command.data), + command), errdetail_internal("%s", wait_result_to_str(pclose_rc)))); goto error; } @@ -120,7 +98,7 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf, len = pg_strip_crlf(buf); error: - pfree(command.data); + pfree(command); return len; } diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/postmaster/shell_archive.c index 71d567065a..77c7b34535 100644 --- a/src/backend/postmaster/shell_archive.c +++ b/src/backend/postmaster/shell_archive.c @@ -18,6 +18,7 @@ #include <sys/wait.h> #include "access/xlog.h" +#include "common/percentrepl.h" #include "pgstat.h" #include "postmaster/pgarch.h" @@ -44,58 +45,18 @@ shell_archive_configured(void) static bool shell_archive_file(const char *file, const char *path) { - char xlogarchcmd[MAXPGPATH]; - char *dp; - char *endp; - const char *sp; + char *xlogarchcmd; + char *nativePath = NULL; int rc; - /* - * construct the command to be executed - */ - dp = xlogarchcmd; - endp = xlogarchcmd + MAXPGPATH - 1; - *endp = '\0'; - - for (sp = XLogArchiveCommand; *sp; sp++) + if (path) { - if (*sp == '%') - { - switch (sp[1]) - { - case 'p': - /* %p: relative path of source file */ - sp++; - strlcpy(dp, path, endp - dp); - make_native_path(dp); - dp += strlen(dp); - break; - case 'f': - /* %f: filename of source file */ - sp++; - strlcpy(dp, file, endp - dp); - dp += strlen(dp); - break; - case '%': - /* convert %% to a single % */ - sp++; - if (dp < endp) - *dp++ = *sp; - break; - default: - /* otherwise treat the % as not special */ - if (dp < endp) - *dp++ = *sp; - break; - } - } - else - { - if (dp < endp) - *dp++ = *sp; - } + nativePath = pstrdup(path); + make_native_path(nativePath); } - *dp = '\0'; + + xlogarchcmd = replace_percent_placeholders(XLogArchiveCommand, "fp", + (const char *[]){file, nativePath}); ereport(DEBUG3, (errmsg_internal("executing archive command \"%s\"", @@ -155,6 +116,8 @@ shell_archive_file(const char *file, const char *path) return false; } + pfree(xlogarchcmd); + elog(DEBUG1, "archived write-ahead log file \"%s\"", file); return true; } diff --git a/src/common/Makefile b/src/common/Makefile index e9af7346c9..d88a3bf945 100644 --- a/src/common/Makefile +++ b/src/common/Makefile @@ -65,6 +65,7 @@ OBJS_COMMON = \ kwlookup.o \ link-canary.o \ md5_common.o \ + percentrepl.o \ pg_get_line.o \ pg_lzcompress.o \ pg_prng.o \ diff --git a/src/common/archive.c b/src/common/archive.c index 47fc877c90..a30146934d 100644 --- a/src/common/archive.c +++ b/src/common/archive.c @@ -20,7 +20,7 @@ #endif #include "common/archive.h" -#include "lib/stringinfo.h" +#include "common/percentrepl.h" /* * BuildRestoreCommand @@ -41,81 +41,14 @@ BuildRestoreCommand(const char *restoreCommand, const char *xlogfname, const char *lastRestartPointFname) { - StringInfoData result; - const char *sp; + char *nativePath = NULL; - /* - * Build the command to be executed. - */ - initStringInfo(&result); - - for (sp = restoreCommand; *sp; sp++) + if (xlogpath) { - if (*sp == '%') - { - switch (sp[1]) - { - case 'p': - { - char *nativePath; - - /* %p: relative path of target file */ - if (xlogpath == NULL) - { - pfree(result.data); - return NULL; - } - sp++; - - /* - * This needs to use a placeholder to not modify the - * input with the conversion done via - * make_native_path(). - */ - nativePath = pstrdup(xlogpath); - make_native_path(nativePath); - appendStringInfoString(&result, - nativePath); - pfree(nativePath); - break; - } - case 'f': - /* %f: filename of desired file */ - if (xlogfname == NULL) - { - pfree(result.data); - return NULL; - } - sp++; - appendStringInfoString(&result, xlogfname); - break; - case 'r': - /* %r: filename of last restartpoint */ - if (lastRestartPointFname == NULL) - { - pfree(result.data); - return NULL; - } - sp++; - appendStringInfoString(&result, - lastRestartPointFname); - break; - case '%': - /* convert %% to a single % */ - sp++; - appendStringInfoChar(&result, *sp); - break; - default: - /* otherwise treat the % as not special */ - appendStringInfoChar(&result, *sp); - break; - } - } - else - { - appendStringInfoChar(&result, *sp); - } + nativePath = pstrdup(xlogpath); + make_native_path(nativePath); } - return result.data; + return replace_percent_placeholders(restoreCommand, "frp", + (const char *[]){xlogfname, lastRestartPointFname, nativePath}); } diff --git a/src/common/meson.build b/src/common/meson.build index f69d75e9c6..e0f60ee48e 100644 --- a/src/common/meson.build +++ b/src/common/meson.build @@ -15,6 +15,7 @@ common_sources = files( 'kwlookup.c', 'link-canary.c', 'md5_common.c', + 'percentrepl.c', 'pg_get_line.c', 'pg_lzcompress.c', 'pg_prng.c', diff --git a/src/common/percentrepl.c b/src/common/percentrepl.c new file mode 100644 index 0000000000..d819d4a5fc --- /dev/null +++ b/src/common/percentrepl.c @@ -0,0 +1,120 @@ +/*------------------------------------------------------------------------- + * + * percentrepl.c + * Common routines to replace percent placeholders in strings + * + * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * + * IDENTIFICATION + * src/common/percentrepl.c + * + *------------------------------------------------------------------------- + */ + +#ifndef FRONTEND +#include "postgres.h" +#else +#include "postgres_fe.h" +#include "common/logging.h" +#endif + +#include "common/percentrepl.h" +#include "lib/stringinfo.h" + +/* + * replace_percent_placeholders + * + * Replace percent-letter placeholders in input string with the supplied + * values. For example, to replace %f with foo and %b with bar, call + * + * replace_percent_placeholders(instr, "bf", (const char *[]){bar, foo}); + * + * The return value is palloc'd. + * + * "%%" is replaced by a single "%". + * + * This throws an error for an unsupported placeholder or a "%" at the end of + * the input string. + * + * A value may be NULL. If the corresponding placeholder is found in the + * input string, the whole function returns NULL. + * + * This functions is meant for cases where all the values are readily + * available or cheap to compute and most invocations will use most values + * (for example for archive_command). Also, it requires that all values are + * strings. It won't be a good match for things like log prefixes or prompts + * that use a mix of data types and any invocation will only use a few of the + * possible values. + */ +char * +replace_percent_placeholders(const char *instr, const char *letters, const char *values[]) +{ + StringInfoData result; + + initStringInfo(&result); + + for (const char *sp = instr; *sp; sp++) + { + if (*sp == '%') + { + if (sp[1] == '%') + { + /* convert %% to a single % */ + sp++; + appendStringInfoChar(&result, *sp); + } + else if (sp[1] == '\0') + { + /* Incomplete escape sequence, expected a character afterward */ +#ifdef FRONTEND + pg_fatal("string ends unexpectedly after escape character \"%%\""); +#else + ereport(ERROR, + errcode(ERRCODE_SYNTAX_ERROR), + errmsg("string ends unexpectedly after escape character \"%%\"")); +#endif + } + else + { + bool found = false; + + for (const char *lp = letters; *lp; lp++) + { + if (sp[1] == *lp) + { + int num = lp - letters; + + if (!values[num]) + { + pfree(result.data); + return NULL; + } + sp++; + appendStringInfoString(&result, values[num]); + found = true; + break; + } + } + if (!found) + { + /* Unknown escape sequence */ +#ifdef FRONTEND + pg_fatal("string contains unexpected escape sequence \"%c\"", sp[1]); +#else + ereport(ERROR, + errcode(ERRCODE_SYNTAX_ERROR), + errmsg("string contains unexpected escape sequence \"%c\"", sp[1])); +#endif + } + } + } + else + { + appendStringInfoChar(&result, *sp); + } + } + + return result.data; +} diff --git a/src/include/common/percentrepl.h b/src/include/common/percentrepl.h new file mode 100644 index 0000000000..794da4e059 --- /dev/null +++ b/src/include/common/percentrepl.h @@ -0,0 +1,18 @@ +/*------------------------------------------------------------------------- + * + * percentrepl.h + * Common routines to replace percent placeholders in strings + * + * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * src/include/common/percentrepl.h + * + *------------------------------------------------------------------------- + */ +#ifndef PERCENTREPL_H +#define PERCENTREPL_H + +extern char *replace_percent_placeholders(const char *instr, const char *letters, const char *values[]); + +#endif /* PERCENTREPL_H */ -- 2.39.0
