Use the same variable substitution rules for cmd line as for paths

The immediate upshot is that the "load-options = sub-vars" setting now
follows more flexible substitution rules, and an environment variable
substitution can be made for part of a command-line argument rather than
only for a complete argument.

doc/manpages/dinit-service.5.m4

@@ -325,13 +325,14 @@ Specifies various options for this service. See the \fBOPTIONS\fR section. This
 directive can be specified multiple times to set additional options.
 .TP
 \fBload\-options\fR = \fIload_option\fR...
-Specifies options for interpreting other settings when loading this service
-description. Currently there is only one available option, \fBsub\-vars\fR,
-which specifies that complete command-line arguments in the form of \fB$NAME\fR should
-be replaced with the contents of the environment variable with the specified
-name. No word-splitting is performed and the variable value always becomes a single
-argument; if the variable is not defined, it is replaced with an empty (zero-length)
-argument. Environment variable variables are taken from the environment of the \fBdinit\fR
+Specifies options for interpreting other settings when loading this service description. Currently
+there is only one available option, \fBsub\-vars\fR, which specifies that command-line arguments
+(or parts thereof) in the form of \fB$NAME\fR should be replaced with the contents of the
+environment variable with the specified name. See \fBVARIABLE SUBSTITUTION\fR for details. Note
+command-line variable substitution occurs after splitting the line into separate arguments and so
+a single environment cannot be used to add multiple arguments to a command line. If a designated
+variable is not defined, it is replaced with an empty (zero-length) string, possibly producing a
+zero-length argument. Environment variable variables are taken from the environment of the \fBdinit\fR
 process, and values specified via \fBenv\-file\fR or \fBready\-notification\fR are not available.
 This functionality is likely to be re-worked or removed in the future; use of this option should
 be avoided if possible.
@@ -492,12 +493,12 @@ only one value is specified with no colon separator, it affects both the soft an
 .\"
 .SS VARIABLE SUBSTITUTION
 .\"
-Some service properties specify a path to a file or directory. For these properties, the specified
-value may contain an environment variable name, preceded by a single `\fB$\fR' character, as in `\fB$NAME\fR'.
-The value of the named environment variable will be substituted. The name must begin with a non-punctuation,
+Some service properties specify a path to a file or directory, or a command line. For these properties, the specified
+value may contain one or more environment variable names, each preceded by a single `\fB$\fR' character, as in `\fB$NAME\fR'.
+In each case the value of the named environment variable will be substituted. The name must begin with a non-punctuation,
 non-space, non-digit character, and ends before the first control character, space, or punctuation
-character other than `\fB.\fR', `\fB\-\fR' or `\fB_\fR'. To avoid substitution, a single `\fB$\fR' can be escaped with a
-second, as in `\fB$$\fR'.
+character other than `\fB.\fR', `\fB\-\fR' or `\fB_\fR'. To avoid substitution, a single `\fB$\fR'
+can be escaped with a second, as in `\fB$$\fR'.
 
 Variables for substitution come from the \fBdinit\fR environment at the time the service is loaded.
 In particular, variables set via \fBenv\-file\fR are not visible to the substitution function.

src/includes/load-service.h

@@ -611,7 +611,7 @@ static auto resolve_env_var = [](const string &name){
 //    p           - path to resolve
 //    var_resolve - function to translate names to values; returning string or const char *;
 //                  may throw setting_exception
-template <class T>
+template <typename T>
 inline std::string resolve_path(std::string &&p, T &var_resolve)
 {
     auto dpos = p.find('$');
@@ -649,6 +649,83 @@ inline std::string resolve_path(std::string &&p, T &var_resolve)
     return r;
 }
 
+// Substitute variable references in a command line with their values. Specified offsets must give
+// the location of separate arguments after word splitting and are adjusted appropriately.
+//
+// throws: setting_exception if a $-substitution is ill-formed, or if the command line is too long;
+//         bad_alloc on allocation failure
+template <typename T>
+static void cmdline_var_subst(std::string &line, std::list<std::pair<unsigned,unsigned>> &offsets,
+        T &var_resolve)
+{
+    auto dindx = line.find('$');
+    if (dindx == string::npos) {
+        return;
+    }
+
+    if (line.length() > (size_t)std::numeric_limits<int>::max()) {
+        // (avoid potential for overflow later)
+        throw setting_exception("command line too long");
+    }
+
+    auto i = offsets.begin();
+    unsigned xpos = 0; // position to copy from
+    std::string r_line;
+    int offadj = 0;
+
+    while (i != offsets.end()) {
+
+        i->first += offadj; // don't adjust end yet
+
+        while (i->second > dindx) {
+            r_line.append(line, xpos, dindx - xpos); // copy unmatched part
+
+            if (line[dindx + 1] == '$') {
+                // double dollar, collapse to single
+                r_line += '$';
+                xpos = dindx + 2;
+                --offadj;
+            }
+            else {
+                // variable
+                auto i = std::next(line.begin(), dindx + 1);
+                string name = read_config_name(i, line.end());
+                if (name.empty()) {
+                    throw setting_exception("invalid/missing variable name after '$'");
+                }
+                size_t line_len_before = r_line.size();
+                r_line.append(var_resolve(name));
+                size_t line_len_after = r_line.size();
+
+                if (line_len_after > (size_t)std::numeric_limits<int>::max()) {
+                    // (avoid potential overflow)
+                    throw setting_exception("command line too long (after substitution)");
+                }
+
+                xpos = i - line.begin();
+                int name_len = xpos - dindx;
+
+                offadj += (int)line_len_after - (int)line_len_before - name_len;
+            }
+
+            dindx = line.find('$', xpos);
+        }
+
+        i->second += offadj;
+        ++i;
+
+        while (i != offsets.end() && i->second < dindx) {
+            i->first += offadj;
+            i->second += offadj;
+            ++i;
+        }
+    }
+
+    r_line.append(line, xpos); // copy final unmatched part
+    line = std::move(r_line);
+    return;
+}
+
 // A wrapper type for service parameters. It is parameterised by dependency type.
 template <class dep_type>
 class service_settings_wrapper

src/load-service.cc

@@ -29,40 +29,9 @@ using string_iterator = std::string::iterator;
 static void do_env_subst(std::string &line, std::list<std::pair<unsigned,unsigned>> &offsets,
         bool do_sub_vars)
 {
+    using namespace dinit_load;
     if (do_sub_vars) {
-        auto i = offsets.begin();
-        std::string r_line = line.substr(i->first, i->second - i->first); // copy command part
-        for (++i; i != offsets.end(); ++i) {
-            auto &offset_pair = *i;
-            if (line[offset_pair.first] == '$') {
-                // Do subsitution for this part:
-                auto env_name = line.substr(offset_pair.first + 1,
-                        offset_pair.second - offset_pair.first - 1);
-                char *env_val = getenv(env_name.c_str());
-                if (env_val != nullptr) {
-                    auto val_len = strlen(env_val);
-                    r_line += " ";
-                    offset_pair.first = r_line.length();
-                    offset_pair.second = offset_pair.first + val_len;
-                    r_line += env_val;
-                }
-                else {
-                    // specified enironment variable not set: treat as an empty string
-                    offset_pair.first = r_line.length();
-                    offset_pair.second = offset_pair.first;
-                }
-            }
-            else {
-                // No subsitution for this part:
-                r_line += " ";
-                auto new_offs = r_line.length();
-                auto len = offset_pair.second - offset_pair.first;
-                r_line += line.substr(offset_pair.first, len);
-                offset_pair.first = new_offs;
-                offset_pair.second = new_offs + len;
-            }
-        }
-        line = std::move(r_line);
+        cmdline_var_subst(line, offsets, resolve_env_var);
     }
 }
 

src/tests/loadtests.cc

@@ -37,6 +37,31 @@ void test_env_subst()
     assert(strcmp("", exec_parts[3]) == 0);
 }
 
+void test_env_subst2()
+{
+    auto resolve_env_var = [](const std::string &name){
+        if (name == "ONE_VAR") return "a";
+        if (name == "TWOVAR") return "hellohello";
+        return "";
+    };
+
+    std::string line = "test x$ONE_VAR~ y$TWOVAR$$ONE_VAR";
+    std::list<std::pair<unsigned,unsigned>> offsets;
+    std::string::iterator li = line.begin();
+    std::string::iterator le = line.end();
+    dinit_load::read_setting_value(li, le, &offsets);
+
+    dinit_load::cmdline_var_subst(line, offsets, resolve_env_var);
+
+    //std::cout << "line = " << line << std::endl; // XXX
+    assert(line == "test xa~ yhellohello$ONE_VAR");
+
+    assert(offsets.size() == 3);
+    assert((*std::next(offsets.begin(), 0) == std::pair<unsigned,unsigned>{0, 4}));
+    assert((*std::next(offsets.begin(), 1) == std::pair<unsigned,unsigned>{5, 8}));
+    assert((*std::next(offsets.begin(), 2) == std::pair<unsigned,unsigned>{9, 28}));
+}
+
 void test_nonexistent()
 {
     bool got_service_not_found = false;
@@ -196,6 +221,7 @@ int main(int argc, char **argv)
     init_test_service_dir();
     RUN_TEST(test_basic, "                ");
     RUN_TEST(test_env_subst, "            ");
+    RUN_TEST(test_env_subst2, "           ");
     RUN_TEST(test_nonexistent, "          ");
     RUN_TEST(test_settings, "             ");
     RUN_TEST(test_path_env_subst, "       ");