Merge pull request #5378 from larsewi/arglist

CFE-2724: The `arglist` attribute now preserves spaces in arguments

cf-agent/verify_exec.c

@@ -335,9 +335,34 @@ 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,
-                               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)

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);
             }
         }

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;
 }

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

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);

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)
@@ -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);
+    char **argv = ArgSplitCommand(command, arglist);
 
     pid = CreatePipeAndFork(type, pd);
     if (pid == (pid_t) -1)

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)

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");
+}

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,14 +144,35 @@ 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");
     assert_false(s[9]);
     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),
     };