From 493756a5310513f216d92fbd4cf0b140ce86dfec Mon Sep 17 00:00:00 2001 From: Lars Erik Wik Date: Wed, 15 Nov 2023 16:26:16 +0100 Subject: [PATCH 1/4] Added arglist argument to ArgSplitCommand function This additional function argument enables us to avoid splitting command-line arguments extracted from the `arglist` attribute in the commands promise. Ticket: CFE-2724 Changelog: None Signed-off-by: Lars Erik Wik --- libpromises/exec_tools.c | 16 ++++++++------- libpromises/exec_tools.h | 10 +++++++++- libpromises/pipes_unix.c | 6 +++--- libpromises/unix.c | 2 +- tests/unit/arg_split_test.c | 40 ++++++++++++++++++++++++++++--------- 5 files changed, 53 insertions(+), 21 deletions(-) diff --git a/libpromises/exec_tools.c b/libpromises/exec_tools.c index f33b8824a5..3684a8c621 100644 --- a/libpromises/exec_tools.c +++ b/libpromises/exec_tools.c @@ -261,7 +261,7 @@ void ArgGetExecutableAndArgs(const char *comm, char **exec, char **args) #define INITIAL_ARGS 8 -char **ArgSplitCommand(const char *comm) +char **ArgSplitCommand(const char *comm, const Seq *arglist) { const char *s = comm; @@ -320,14 +320,16 @@ char **ArgSplitCommand(const char *comm) args[argc++] = arg; } -/* Trailing NULL */ - - if (argc == argslen) + size_t extra = (arglist == NULL) ? 0 : SeqLength(arglist); + if (argc + extra + 1 /* NULL */ > argslen) { - argslen += 1; - args = xrealloc(args, argslen * sizeof(char *)); + args = xrealloc(args, (argc + extra + 1) * sizeof(char *)); + } + + for (size_t i = 0; i < extra; i++) { + args[argc++] = xstrdup(SeqAt(arglist, i)); } - args[argc++] = NULL; + args[argc] = NULL; return args; } diff --git a/libpromises/exec_tools.h b/libpromises/exec_tools.h index 459c554179..de1716a1d4 100644 --- a/libpromises/exec_tools.h +++ b/libpromises/exec_tools.h @@ -34,7 +34,15 @@ bool ShellCommandReturnsZero(const char *command, ShellType shell); bool GetExecOutput(const char *command, char **buffer, size_t *buffer_size, ShellType shell, OutputSelect output_select, int *ret_out); void ActAsDaemon(); void ArgGetExecutableAndArgs(const char *comm, char **exec, char **args); -char **ArgSplitCommand(const char *comm); + +/** + * Create a argument list as expected by execv(3) + * @param command is split on spaces, unless quoted + * @param arglist is not split (can have embedded spaces) + * @return null-terminated argument list + */ +char **ArgSplitCommand(const char *command, const Seq *arglist); + void ArgFree(char **args); #endif diff --git a/libpromises/pipes_unix.c b/libpromises/pipes_unix.c index 0adf1a5da4..45e133cc9d 100644 --- a/libpromises/pipes_unix.c +++ b/libpromises/pipes_unix.c @@ -287,7 +287,7 @@ IOData cf_popen_full_duplex(const char *command, bool capture_stderr, bool requi int parent_pipe[2]; /* From parent to child */ pid_t pid; - char **argv = ArgSplitCommand(command); + char **argv = ArgSplitCommand(command, NULL); fflush(NULL); /* Empty file buffers */ pid = CreatePipesAndFork("r+t", child_pipe, parent_pipe); @@ -377,7 +377,7 @@ FILE *cf_popen_select(const char *command, const char *type, OutputSelect output pid_t pid; FILE *pp = NULL; - char **argv = ArgSplitCommand(command); + char **argv = ArgSplitCommand(command, NULL); pid = CreatePipeAndFork(type, pd); if (pid == (pid_t) -1) @@ -475,7 +475,7 @@ FILE *cf_popensetuid(const char *command, const char *type, pid_t pid; FILE *pp = NULL; - char **argv = ArgSplitCommand(command); + char **argv = ArgSplitCommand(command, NULL); pid = CreatePipeAndFork(type, pd); if (pid == (pid_t) -1) diff --git a/libpromises/unix.c b/libpromises/unix.c index 692960b32e..c9159fbe97 100644 --- a/libpromises/unix.c +++ b/libpromises/unix.c @@ -195,7 +195,7 @@ bool ShellCommandReturnsZero(const char *command, ShellType shell) } else { - char **argv = ArgSplitCommand(command); + char **argv = ArgSplitCommand(command, NULL); int devnull; if (LogGetGlobalLevel() < LOG_LEVEL_INFO) diff --git a/tests/unit/arg_split_test.c b/tests/unit/arg_split_test.c index 0fc301c977..ef7e10f421 100644 --- a/tests/unit/arg_split_test.c +++ b/tests/unit/arg_split_test.c @@ -8,7 +8,7 @@ static void test_split_empty(void) { - char **s = ArgSplitCommand(""); + char **s = ArgSplitCommand("", NULL); assert_true(s); assert_false(*s); @@ -22,7 +22,7 @@ static void test_split_empty(void) static void test_split_easy(void) { - char **s = ArgSplitCommand("zero one two"); + char **s = ArgSplitCommand("zero one two", NULL); assert_string_equal(s[0], "zero"); assert_string_equal(s[1], "one"); @@ -42,7 +42,7 @@ static void test_split_easy(void) static void test_split_whitespace_prefix(void) { - char **s = ArgSplitCommand(" zero one two"); + char **s = ArgSplitCommand(" zero one two", NULL); assert_string_equal(s[0], "zero"); assert_string_equal(s[1], "one"); @@ -62,7 +62,7 @@ static void test_split_whitespace_prefix(void) static void test_split_quoted_beginning(void) { - char **s = ArgSplitCommand("\"quoted string\" atbeginning"); + char **s = ArgSplitCommand("\"quoted string\" atbeginning", NULL); assert_string_equal(s[0], "quoted string"); assert_string_equal(s[1], "atbeginning"); @@ -80,7 +80,7 @@ static void test_split_quoted_beginning(void) static void test_split_quoted_end(void) { - char **s = ArgSplitCommand("atend 'quoted string'"); + char **s = ArgSplitCommand("atend 'quoted string'", NULL); assert_string_equal(s[0], "atend"); assert_string_equal(s[1], "quoted string"); @@ -98,7 +98,7 @@ static void test_split_quoted_end(void) static void test_split_quoted_middle(void) { - char **s = ArgSplitCommand("at `quoted string` middle"); + char **s = ArgSplitCommand("at `quoted string` middle", NULL); assert_string_equal(s[0], "at"); assert_string_equal(s[1], "quoted string"); @@ -117,7 +117,7 @@ static void test_split_quoted_middle(void) static void test_complex_quoting(void) { - char **s = ArgSplitCommand("\"foo`'bar\""); + char **s = ArgSplitCommand("\"foo`'bar\"", NULL); assert_string_equal(s[0], "foo`'bar"); assert_false(s[1]); @@ -135,7 +135,7 @@ static void test_arguments_resize_for_null(void) { /* This test checks that extending returned argument list for NULL terminator * works correctly */ - char **s = ArgSplitCommand("0 1 2 3 4 5 6 7"); + char **s = ArgSplitCommand("0 1 2 3 4 5 6 7", NULL); assert_string_equal(s[7], "7"); assert_false(s[8]); @@ -144,7 +144,7 @@ static void test_arguments_resize_for_null(void) static void test_arguments_resize(void) { - char **s = ArgSplitCommand("0 1 2 3 4 5 6 7 8"); + char **s = ArgSplitCommand("0 1 2 3 4 5 6 7 8", NULL); assert_string_equal(s[7], "7"); assert_string_equal(s[8], "8"); @@ -152,6 +152,27 @@ static void test_arguments_resize(void) ArgFree(s); } +static void test_arguments_with_arglist(void) +{ + char *command = "lorem ipsum dolor sit amet"; + + Seq *arglist = SeqNew(10, NULL); + char extra[][32] = { "consectetur adipiscing", "elit", "sed do", "eiusmod" }; + for (size_t i = 0; i < sizeof(extra) / sizeof(*extra); i++) { + SeqAppend(arglist, extra[i]); + } + + char **argv = ArgSplitCommand(command, arglist); + SeqDestroy(arglist); + + assert_string_equal(argv[0], "lorem"); + assert_string_equal(argv[2], "dolor"); + assert_string_equal(argv[5], "consectetur adipiscing"); + assert_string_equal(argv[8], "eiusmod"); + assert_false(argv[9]); + ArgFree(argv); +} + static void test_command_promiser(void) { char *t1 = "/bin/echo"; @@ -227,6 +248,7 @@ int main() unit_test(test_complex_quoting), unit_test(test_arguments_resize_for_null), unit_test(test_arguments_resize), + unit_test(test_arguments_with_arglist), unit_test(test_command_promiser), }; From a83968e71a318a7aeea984d6d2d64545ea26048e Mon Sep 17 00:00:00 2001 From: Lars Erik Wik Date: Wed, 6 Dec 2023 11:31:26 +0100 Subject: [PATCH 2/4] Added arglist argument to cf_popensetuid() This additional function argument enables us to avoid splitting command-line arguments extracted from the `arglist` attribute in the commands promise. Ticket: CFE-2724 Changelog: None Signed-off-by: Lars Erik Wik --- cf-agent/verify_exec.c | 2 +- cf-monitord/history.c | 4 +++- libpromises/pipes.h | 2 +- libpromises/pipes_unix.c | 8 ++++---- 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/cf-agent/verify_exec.c b/cf-agent/verify_exec.c index 08fe3f9b18..53fcda39b9 100644 --- a/cf-agent/verify_exec.c +++ b/cf-agent/verify_exec.c @@ -336,7 +336,7 @@ static ActionResult RepairExec(EvalContext *ctx, const Attributes *a, else { pfp = - cf_popensetuid(cmdline, open_mode, a->contain.owner, a->contain.group, a->contain.chdir, a->contain.chroot, + cf_popensetuid(cmdline, NULL, open_mode, a->contain.owner, a->contain.group, a->contain.chdir, a->contain.chroot, a->transaction.background); } diff --git a/cf-monitord/history.c b/cf-monitord/history.c index a508a5fa3e..83cc28e51e 100644 --- a/cf-monitord/history.c +++ b/cf-monitord/history.c @@ -179,6 +179,8 @@ static void Nova_HistoryUpdate(time_t time, const Averages *newvals) static Item *NovaReSample(EvalContext *ctx, int slot, const Attributes *attr, const Promise *pp, PromiseResult *result) { assert(attr != NULL); + assert(pp != NULL); + Attributes a = *attr; // TODO: try to remove this local copy CfLock thislock; char eventname[CF_BUFSIZE]; @@ -311,7 +313,7 @@ static Item *NovaReSample(EvalContext *ctx, int slot, const Attributes *attr, co else { fin = - cf_popensetuid(pp->promiser, "r", a.contain.owner, a.contain.group, a.contain.chdir, + cf_popensetuid(pp->promiser, NULL, "r", a.contain.owner, a.contain.group, a.contain.chdir, a.contain.chroot, false); } } diff --git a/libpromises/pipes.h b/libpromises/pipes.h index d2ee246c82..58579e2096 100644 --- a/libpromises/pipes.h +++ b/libpromises/pipes.h @@ -49,7 +49,7 @@ int cf_pclose_full_duplex_side(int fd); FILE *cf_popen(const char *command, const char *type, bool capture_stderr); FILE *cf_popen_select(const char *command, const char *type, OutputSelect output_select); -FILE *cf_popensetuid(const char *command, const char *type, uid_t uid, gid_t gid, char *chdirv, char *chrootv, int background); +FILE *cf_popensetuid(const char *command, const Seq *arglist, const char *type, uid_t uid, gid_t gid, char *chdirv, char *chrootv, int background); FILE *cf_popen_sh(const char *command, const char *type); FILE *cf_popen_sh_select(const char *command, const char *type, OutputSelect output_select); FILE *cf_popen_shsetuid(const char *command, const char *type, uid_t uid, gid_t gid, char *chdirv, char *chrootv, int background); diff --git a/libpromises/pipes_unix.c b/libpromises/pipes_unix.c index 45e133cc9d..29f23e429b 100644 --- a/libpromises/pipes_unix.c +++ b/libpromises/pipes_unix.c @@ -467,15 +467,15 @@ FILE *cf_popen(const char *command, const char *type, bool capture_stderr) * WARNING: this is only allowed to be called from single-threaded code, * because of the safe_chdir() call in the forked child. */ -FILE *cf_popensetuid(const char *command, const char *type, - uid_t uid, gid_t gid, char *chdirv, char *chrootv, - ARG_UNUSED int background) +FILE *cf_popensetuid(const char *command, const Seq *arglist, + const char *type, uid_t uid, gid_t gid, char *chdirv, + char *chrootv, ARG_UNUSED int background) { int pd[2]; pid_t pid; FILE *pp = NULL; - char **argv = ArgSplitCommand(command, NULL); + char **argv = ArgSplitCommand(command, arglist); pid = CreatePipeAndFork(type, pd); if (pid == (pid_t) -1) From 984d7336f4a0924a8671137ef81bd5a27ecb394c Mon Sep 17 00:00:00 2001 From: Lars Erik Wik Date: Fri, 17 Nov 2023 13:37:36 +0100 Subject: [PATCH 3/4] The `arglist` attribute now preserves spaces in arguments The `arglist attribute in the `commands` promises now preserves whitespaces in the arguments. Whitespaces are currently not preserved on Windows, or if the `useshell` attribute is set to anything other than `"noshell"`. See ticket CFE-4294. Ticket: CFE-2724 Changelog: Commit Signed-off-by: Lars Erik Wik --- cf-agent/verify_exec.c | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/cf-agent/verify_exec.c b/cf-agent/verify_exec.c index 53fcda39b9..166f84a240 100644 --- a/cf-agent/verify_exec.c +++ b/cf-agent/verify_exec.c @@ -335,9 +335,34 @@ static ActionResult RepairExec(EvalContext *ctx, const Attributes *a, } else { - pfp = - cf_popensetuid(cmdline, NULL, open_mode, a->contain.owner, a->contain.group, a->contain.chdir, a->contain.chroot, - a->transaction.background); + char *const command = (a->args == NULL) + ? xstrdup(pp->promiser) + : StringFormat("%s %s", pp->promiser, + a->args); + Seq *arglist = NULL; + if (a->arglist != NULL) + { + arglist = SeqNew(8, NULL); + for (const Rlist *rp = a->arglist; rp != NULL; rp = rp->next) + { + if (rp->val.type == RVAL_TYPE_SCALAR) + { + SeqAppend(arglist, RlistScalarValue(rp)); + } + else + { + Log(LOG_LEVEL_ERR, + "RepairExec: invalid rval (not a scalar) in arglist of commands promise '%s'", + pp->promiser); + } + } + } + pfp = cf_popensetuid(command, arglist, open_mode, + a->contain.owner, a->contain.group, + a->contain.chdir, a->contain.chroot, + a->transaction.background); + free(command); + SeqDestroy(arglist); } if (pfp == NULL) From 933bac96b020eaaf297c8a689d4c5f4ba92adc89 Mon Sep 17 00:00:00 2001 From: Lars Erik Wik Date: Fri, 17 Nov 2023 14:46:59 +0100 Subject: [PATCH 4/4] Added test checking that spaces are preserved by `arglist` Added acceptance test checking that whitespaces are preserved by the `arglist` attribute in the `commands` promises. Whitespaces are currently not preserved on Windows, or if the `useshell` attribute is set to anything other than `"noshell"`. Ticket: CFE-2724 Changelog: None Signed-off-by: Lars Erik Wik --- tests/acceptance/08_commands/arglist.cf | 67 +++++++++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 tests/acceptance/08_commands/arglist.cf diff --git a/tests/acceptance/08_commands/arglist.cf b/tests/acceptance/08_commands/arglist.cf new file mode 100644 index 0000000000..8b344e1321 --- /dev/null +++ b/tests/acceptance/08_commands/arglist.cf @@ -0,0 +1,67 @@ +############################################################################## +# +# CFE-2724: Test that arglist attribute preserves white-spaces +# +############################################################################## + +body common control +{ + inputs => { "../default.cf.sub" }; + bundlesequence => { default("$(this.promise_filename)") }; + version => "1.0"; +} + +############################################################################## + +bundle agent init +{ + files: + "$(G.testfile).actual" + delete => tidy; + "$(G.testfile).sh" + perms => mog("700", "root", "root"), + content => '#!/bin/sh +for arg in "$(const.dollar)$(const.at)" +do + echo "$(const.dollar)arg" >> $(G.testfile).actual +done'; +} + +############################################################################## + +bundle agent test +{ + meta: + "description" -> { "CFE-2724" } + string => "Test that arglist attribute preserves white-spaces"; + "test_skip_unsupported" + string => "windows", + comment => "See ticket CFE-4294"; + + commands: + "$(G.testfile).sh" + args => "one two three", + arglist => { "four", "five six", " seven$(const.t)" }; +} + +############################################################################## + +bundle agent check +{ + vars: + "actual" + string => readfile("$(G.testfile).actual"); + "expected" + string => "one +two +three +four +five six + seven$(const.t) +"; + + methods: + "any" + usebundle => dcs_check_strcmp("$(actual)", "$(expected)", + "$(this.promise_filename)", "no"); +}