Browse Source

Substitute env-file in service files at load

The previous behavior did not substitute env-file variables at
service load, because it could only see the environment available
to dinit process. This changes that, and env-files are instead
loaded during service load, and all variables are substituted.

The vars set up during process call are not exposed here. These
would have limited usability in any case and would significantly
complicate the whole thing.

This also implements a subset of POSIX parameter expansion, and
removes the obsolete sub-vars load option.

Fixes https://github.com/davmac314/dinit/issues/109
Daniel Kolesa 1 year ago
parent
commit
c7b9e5ed61

+ 1 - 0
.gitignore

@@ -22,6 +22,7 @@
 # Integration test output:
 /src/igr-tests/basic/basic-ran
 /src/igr-tests/environ/env-record
+/src/igr-tests/environ2/env-record
 /src/igr-tests/ps-environ/env-record
 /src/igr-tests/chain-to/recorded-output
 /src/igr-tests/restart/basic-ran

+ 25 - 23
doc/manpages/dinit-service.5.m4

@@ -160,14 +160,9 @@ group of the specified user.
 \fBenv\-file\fR = \fIfile\fR
 Specifies a file containing value assignments for environment variables, in the same
 format recognised by the \fBdinit\fR command's \fB\-\-env\-file\fR option (see \fBdinit\fR(5)).
-The file is read (or re-read) whenever the service is started; the values read do not
-affect for the processing performed for the \fBsub\-vars\fR load option, which is done
-when the service description is loaded.
-The precise behaviour of this setting may change in the future.
-It is recommended to avoid depending on the specified file contents being reloaded
-whenever the service process starts.
+The file is read when the service is loaded, therefore values from it can be used in substitutions.
 .sp
-The path specified is subject to variable substitution (see \fBVARIABLE SUBSTITUTION\fR).
+That means this path is static and cannot itself have variable substitutions (see \fBVARIABLE SUBSTITUTION\fR).
 .TP
 \fBrestart\fR = {yes | true | no | false}
 Indicates whether the service should automatically restart if it stops, including due to
@@ -354,18 +349,11 @@ 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 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 variable 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.
+Currently there is only one available option, \fBexport-passwd-vars\fR, which specifies that
+the environment variables `\fBUSER\fR', `\fBLOGNAME\fR' (same as `\fBUSER\fR'),
+`\fBHOME\fR', `\fBSHELL\fR', `\fBUID\fR', and `\fBGID\fR' should be exported into the
+service's load environment (that is, overriding any global environment including the
+global environment file, but being overridable by the service's environment file).
 .TP
 \fBinittab\-id\fR = \fIid-string\fR
 When this service is started, if this setting (or the \fBinittab\-line\fR setting) has a
@@ -574,12 +562,26 @@ 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'.
+before the first control character, space, or punctuation character other than `\fB_\fR'.
 To avoid substitution, a single `\fB$\fR' can be escaped with a second, as in `\fB$$\fR'.
 .sp
-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.
+Note command-line variable substitution occurs after splitting the line into separate arguments and so
+a single environment variable 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.
+.sp
+Variable substitution also supports a limited subset of shell syntax. You can use curly
+braces to enclose the variable, as in `\fB${NAME}\fR'. Limited parameter expansion is
+also supported, specifically the forms `\fB${NAME:\-word}\fR' (substitute `\fBword\fR'
+if variable is unset or empty), `\fB${NAME\-word}\fR' (substitute `\fBword\fR' if
+variable is unset), `\fB${NAME:+word}\fR' (substitute `\fBword\fR' if variable is
+set and non\-empty), and `\fB${NAME+word}\fR' (substitute `\fBword\fR' if variable
+is set).
+.sp
+The priority of environment variables, from highest to lowest, is the service \fBenv\-file\fR,
+followed by variables set by the \fBset\-passwd\-vars\fR load option, followed by the global
+environment file of \fBdinit\fR, followed by the process environment of \fBdinit\fR. This
+applies to both the substitutions as well as the process environment of the service.
 .\"
 .SH EXAMPLES
 .LP

+ 8 - 3
src/dinitcheck.cc

@@ -345,6 +345,9 @@ service_record *load_service(service_set_t &services, const std::string &name,
 
     service_settings_wrapper<prelim_dep> settings;
 
+    environment menv{};
+    environment renv{};
+
     string line;
     service_file.exceptions(ios::badbit);
 
@@ -386,16 +389,18 @@ service_record *load_service(service_set_t &services, const std::string &name,
 
     bool issued_var_subst_warning = false;
 
-    auto resolve_var = [&](const string &name) {
+    environment::env_map renvmap = renv.build(menv);
+
+    auto resolve_var = [&](const string &name, environment::env_map const &envmap) {
         if (!issued_var_subst_warning) {
             report_service_description_err(name, "warning: variable substitution performed by dinitcheck "
                     "for file paths may not match dinitd (environment may differ)");
             issued_var_subst_warning = true;
         }
-        return resolve_env_var(name);
+        return resolve_env_var(name, envmap);
     };
 
-    settings.finalise(report_err, report_err, resolve_var);
+    settings.finalise(report_err, renvmap, report_err, resolve_var);
 
     auto check_command = [&](const char *setting_name, const char *command) {
         struct stat command_stat;

+ 1 - 1
src/igr-tests/Makefile

@@ -7,7 +7,7 @@ igr-runner: igr-runner.cc
 	$(CXX) $(CXXFLAGS) $(CXXFLAGS_EXTRA) $(LDFLAGS) $(LDFLAGS_EXTRA) igr-runner.cc -o igr-runner
 
 clean:
-	rm -f igr-runner basic/basic-ran environ/env-record ps-environ/env-record chain-to/recorded-output var-subst/args-record
+	rm -f igr-runner basic/basic-ran environ/env-record environ2/env-record ps-environ/env-record chain-to/recorded-output var-subst/args-record
 	rm -f restart/basic-ran
 	rm -f check-basic/output.txt check-lint/output.txt check-cycle/output.txt no-command-error/dinit-run.log
 	rm -rf reload1/sd

+ 6 - 0
src/igr-tests/environ/checkenv.sh

@@ -4,4 +4,10 @@ if [ "$TEST_VAR_ONE" = "hello" ]; then
     echo "$DINIT_SOCKET_PATH" >> ./env-record
 fi
 
+if [ "$1" = "hello" ]; then
+    echo gotenv1 >> ./env-record
+elif [ "$1" = "goodbye" ]; then
+    echo gotenv2 >> ./env-record
+fi
+
 echo "$TEST_VAR_ONE" >> ./env-record

+ 1 - 1
src/igr-tests/environ/run-test.sh

@@ -19,7 +19,7 @@ rm -f ./env-record
 
 STATUS=FAIL
 if [ -e env-record ]; then
-   if [ "$(cat env-record)" = "$(echo $DINIT_SOCKET_PATH; echo hello; echo goodbye; echo 3; echo 2; echo 1)" ]; then
+   if [ "$(cat env-record)" = "$(echo $DINIT_SOCKET_PATH; echo gotenv1; echo hello; echo gotenv2; echo goodbye; echo 3; echo 2; echo 1)" ]; then
        STATUS=PASS
    fi
 fi

+ 1 - 1
src/igr-tests/environ/sd/checkenv

@@ -1,3 +1,3 @@
 type = process
-command = ./checkenv.sh
+command = ./checkenv.sh $TEST_VAR_ONE
 restart = false

+ 10 - 0
src/igr-tests/environ2/checkenv.sh

@@ -0,0 +1,10 @@
+#!/bin/sh
+
+echo "$TEST_VAR" >> ./env-record
+echo "$TEST_VAR_BASE" >> ./env-record
+echo "$TEST_VAR_ONE" >> ./env-record
+echo "$USER" >> ./env-record
+echo "$LOGNAME" >> ./env-record
+echo "$SHELL" >> ./env-record
+echo "$UID" >> ./env-record
+echo "$GID" >> ./env-record

+ 4 - 0
src/igr-tests/environ2/env-dinit

@@ -0,0 +1,4 @@
+TEST_VAR_BASE=hello
+TEST_VAR_ONE=world
+# this is set per-service, so override will do nothing
+LOGNAME=dinit-igr-test

+ 3 - 0
src/igr-tests/environ2/env-service

@@ -0,0 +1,3 @@
+TEST_VAR_ONE=override
+# so we know we can override it from here
+SHELL=/bogus/value

+ 41 - 0
src/igr-tests/environ2/run-test.sh

@@ -0,0 +1,41 @@
+#!/bin/sh
+
+cd "$(dirname "$0")"
+
+export DINIT_SOCKET_PATH="$(pwd)/socket"
+
+rm -f ./env-record
+
+RUSER=$(id -nu)
+RUID=$(id -u)
+RGID=$(id -g)
+
+# unset to make sure dinit can initialize this itself
+unset USER
+unset LOGNAME
+unset SHELL
+unset UID
+unset GID
+
+# test whether vars from global environment propagate
+export TEST_VAR="helloworld"
+
+"$DINIT_EXEC" -d sd -u -p socket -q \
+        -e env-dinit \
+	checkenv
+
+USER="$RUSER"
+# we try to override this one in env-dinit, but it should be set per-service
+LOGNAME="$USER"
+# these are overriden in env files
+SHELL="/bogus/value"
+
+STATUS=FAIL
+if [ -e env-record ]; then
+   if [ "$(cat env-record)" = "$(echo helloworld; echo hello; echo override; echo $USER; echo $LOGNAME; echo $SHELL; echo $RUID; echo $RGID)" ]; then
+       STATUS=PASS
+   fi
+fi
+
+if [ $STATUS = PASS ]; then exit 0; fi
+exit 1

+ 5 - 0
src/igr-tests/environ2/sd/checkenv

@@ -0,0 +1,5 @@
+type = process
+command = ./checkenv.sh $TEST_VAR_ONE $TEST_VAR_BASE
+restart = false
+env-file = env-service
+load-options = export-passwd-vars

+ 1 - 1
src/igr-tests/igr-runner.cc

@@ -12,7 +12,7 @@ extern char **environ;
 
 int main(int argc, char **argv)
 {
-    const char * const test_dirs[] = { "basic", "environ", "ps-environ", "chain-to", "force-stop",
+    const char * const test_dirs[] = { "basic", "environ", "environ2", "ps-environ", "chain-to", "force-stop",
             "restart", "check-basic", "check-cycle", "check-lint", "reload1", "reload2", "no-command-error",
             "add-rm-dep", "var-subst", "svc-start-fail", "dep-not-found" };
     constexpr int num_tests = sizeof(test_dirs) / sizeof(test_dirs[0]);

+ 1 - 1
src/igr-tests/var-subst/checkargs.sh

@@ -1,3 +1,3 @@
 #!/bin/sh
 
-echo 1:"$1" 2:"$2" 3:"$3" >> ./args-record
+echo 1:"$1" 2:"$2" 3:"$3" 4:"$4" >> ./args-record

+ 1 - 1
src/igr-tests/var-subst/run-test.sh

@@ -10,7 +10,7 @@ export TEST_VAR_ONE="var one" TEST_VAR_TWO=vartwo TEST_VAR_THREE=varthree
 
 STATUS=FAIL
 if [ -e args-record ]; then
-   if [ "$(cat args-record)" = "1:xxxvar one/yyy 2:vartwovarthree 3:" ]; then
+   if [ "$(cat args-record)" = "1:xxxvar one/yyy 2:vartwovarthree 3:varfour 4:" ]; then
        STATUS=PASS
    fi
 fi

+ 1 - 2
src/igr-tests/var-subst/sd/checkargs

@@ -1,4 +1,3 @@
 type = process
-command = ./checkargs.sh xxx$TEST_VAR_ONE/yyy $TEST_VAR_TWO$TEST_VAR_THREE
-load-options = sub-vars
+command = ./checkargs.sh xxx$TEST_VAR_ONE/yyy ${TEST_VAR_TWO}$TEST_VAR_THREE ${TEST_VAR_FOUR:-varfour}
 restart = false

+ 8 - 0
src/includes/dinit-env.h

@@ -95,6 +95,14 @@ public:
 
         // map of variable name (via string_view) to its index in env_list
         std::unordered_map<string_view, unsigned, hash_sv> var_map;
+
+        const char *lookup(string_view sv) const {
+            auto it = var_map.find(sv);
+            if (it != var_map.end()) {
+                return env_list[it->second] + sv.size() + 1;
+            }
+            return nullptr;
+        }
     };
 
     // return environment variable in form NAME=VALUE. Assumes that the real environment is the parent.

+ 125 - 72
src/includes/load-service.h

@@ -15,6 +15,7 @@
 #include <grp.h>
 #include <pwd.h>
 
+#include "dinit-env.h"
 #include "dinit-utmp.h"
 #include "dinit-util.h"
 #include "service-constants.h"
@@ -164,7 +165,9 @@ inline int signal_name_to_number(std::string &signame) noexcept
 }
 
 // Read a setting/variable name; return empty string if no valid name
-inline string read_config_name(string_iterator & i, string_iterator end) noexcept
+//
+// If env is set, dashes/dots are not allowed as they break correct envvar parsing
+inline string read_config_name(string_iterator & i, string_iterator end, bool env = false) noexcept
 {
     using std::locale;
     using std::ctype;
@@ -184,9 +187,9 @@ inline string read_config_name(string_iterator & i, string_iterator end) noexcep
         return {};
     }
 
-    // Within the setting name, allow dash and dot; also allow any non-control, non-punctuation,
-    // non-space character.
-    while (i != end && (*i == '-' || *i == '.' || *i == '_'
+    // Within the setting name, allow dash and dot unless parsing envvar name
+    // also allow any non-control, non-punctuation non-space character.
+    while (i != end && (((*i == '-' || *i == '.') && !env) || *i == '_'
             || (!facet.is(ctype<char>::cntrl, *i) && !facet.is(ctype<char>::punct, *i)
                     && !facet.is(ctype<char>::space, *i)))) {
         rval += *i;
@@ -311,6 +314,56 @@ inline string read_setting_value(unsigned line_num, string_iterator & i, string_
     return rval;
 }
 
+inline void fill_environment_userinfo(uid_t uid, const std::string &service_name, environment &env)
+{
+    if (uid == (uid_t)-1) {
+        uid = geteuid();
+    }
+
+    char buf[std::numeric_limits<unsigned long long>::digits10 + 1];
+    snprintf(buf, sizeof(buf), "%llu", (unsigned long long)uid);
+
+    errno = 0;
+    struct passwd *pwent = getpwuid(uid);
+
+    if (!pwent) {
+        if (!errno) {
+            throw service_load_exc(service_name, std::string("user id '") + buf + "' does not exist in system database");
+        } else {
+            throw service_load_exc(service_name, std::string("error accessing user database: ") + strerror(errno));
+        }
+    }
+
+    std::string enval;
+
+    // USER
+    enval = "USER=";
+    enval += pwent->pw_name;
+    env.set_var(std::move(enval));
+    // LOGNAME
+    enval = "LOGNAME=";
+    enval += pwent->pw_name;
+    env.set_var(std::move(enval));
+    // HOME
+    enval = "HOME=";
+    enval += pwent->pw_dir;
+    env.set_var(std::move(enval));
+    // SHELL
+    enval = "SHELL=";
+    enval += pwent->pw_shell;
+    env.set_var(std::move(enval));
+    // UID (non-standard, but useful)
+    enval = "UID=";
+    snprintf(buf, sizeof(buf), "%llu", (unsigned long long)pwent->pw_uid);
+    enval += buf;
+    env.set_var(std::move(enval));
+    // GID (non-standard, but useful)
+    enval = "GID=";
+    snprintf(buf, sizeof(buf), "%llu", (unsigned long long)pwent->pw_gid);
+    enval += buf;
+    env.set_var(std::move(enval));
+}
+
 // Parse a userid parameter which may be a numeric user ID or a username. If a name, the
 // userid is looked up via the system user database (getpwnam() function). In this case,
 // the associated group is stored in the location specified by the group_p parameter if
@@ -609,65 +662,21 @@ void process_service_file(string name, std::istream &service_file, T func)
 static auto dummy_lint = [](const char *){};
 
 // Resolve leading variables in paths using the environment
-static auto resolve_env_var = [](const string &name){
-    const char *r = getenv(name.c_str());
-    if (r == nullptr) {
-        return "";
-    }
-    return r;
+static auto resolve_env_var = [](const string &name, environment::env_map const &envmap){
+    return envmap.lookup(name);
 };
 
-// Resolve a path with variable substitutions ($varname). '$$' resolves to a single '$'.
-// Throws setting_exception on failure.
-//    p           - path to resolve
-//    var_resolve - function to translate names to values; returning string or const char *;
-//                  may throw setting_exception
-template <typename T>
-inline std::string resolve_path(const char *setting_name, std::string &&p, T &var_resolve)
-{
-    auto dpos = p.find('$');
-    if (dpos == string::npos) {
-        // shortcut the case where there are no substitutions:
-        return std::move(p);
-    }
-
-    string r;
-    string::size_type last_pos = 0;
-
-    do {
-        r.append(p, last_pos, dpos - last_pos); // non-substituted portion
-        ++dpos;
-        if (dpos < p.size() && p[dpos] == '$') {
-            // double '$' resolves to a single '$' in output
-            r += '$';
-            last_pos = dpos + 1;
-            dpos = p.find('$', last_pos);
-            continue;
-        }
-        auto i = std::next(p.begin(), dpos);
-        string name = read_config_name(i, p.end());
-        if (name.empty()) {
-            throw service_description_exc(setting_name, "invalid/missing variable name after '$'");
-        }
-        string value = var_resolve(name);
-        r.append(value);
-        last_pos = i - p.begin();
-        dpos = p.find('$', last_pos);
-    } while (dpos != string::npos);
-
-    r.append(p, last_pos, string::npos);
-
-    return r;
-}
-
-// Substitute variable references in a command line with their values. Specified offsets must give
+// Substitute variable references in a value with their values. Specified offsets must give
 // the location of separate arguments after word splitting and are adjusted appropriately.
+// If you simply wish to substitute all variables in the given string, pass an offsets list
+// containing one pair with the string's bounds (0, size). '$$' resolves to a single '$'.
 //
 // 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(const char *setting_name, std::string &line,
-        std::list<std::pair<unsigned,unsigned>> &offsets, T &var_resolve)
+static void value_var_subst(const char *setting_name, std::string &line,
+        std::list<std::pair<unsigned,unsigned>> &offsets, T &var_resolve,
+        environment::env_map const &envmap)
 {
     auto dindx = line.find('$');
     if (dindx == string::npos) {
@@ -676,7 +685,7 @@ static void cmdline_var_subst(const char *setting_name, std::string &line,
 
     if (line.length() > (size_t)std::numeric_limits<int>::max()) {
         // (avoid potential for overflow later)
-        throw service_description_exc(setting_name, "command line too long");
+        throw service_description_exc(setting_name, "value too long");
     }
 
     auto i = offsets.begin();
@@ -690,7 +699,6 @@ static void cmdline_var_subst(const char *setting_name, std::string &line,
 
         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 += '$';
@@ -699,18 +707,60 @@ static void cmdline_var_subst(const char *setting_name, std::string &line,
             }
             else {
                 // variable
-                auto i = std::next(line.begin(), dindx + 1);
-                string name = read_config_name(i, line.end());
+                bool brace = line[dindx + 1] == '{';
+                auto i = std::next(line.begin(), dindx + 1 + int(brace));
+                // read environment variable name as specified by POSIX; do
+                // not use read_config_name, it allows characters not allowed
+                // in env vars and breaks further parsing
+                string name = read_config_name(i, line.end(), true);
                 if (name.empty()) {
                     throw service_description_exc(setting_name, "invalid/missing variable name after '$'");
                 }
+                char altmode = '\0';
+                bool colon = false;
+                auto altbeg = i, altend = i;
+                if (brace) {
+                    /* ${foo+val}, ${foo-val}, ${foo:+val}, ${foo:-val} */
+                    if (*i == ':') {
+                        colon = true;
+                        ++i;
+                        if (*i != '+' && *i != '-') {
+                            throw service_description_exc(setting_name, "invalid syntax in variable substitution");
+                        }
+                    }
+                    if (*i == '+' || *i == '-') {
+                        altmode = *i++;
+                        altbeg = altend = i;
+                        while (*altend != '}') {
+                            ++altend;
+                        }
+                        i = altend;
+                    }
+                    if (*i++ != '}') {
+                        throw service_description_exc(setting_name, "unmatched '{' in variable substitution");
+                    }
+                }
                 size_t line_len_before = r_line.size();
-                r_line.append(var_resolve(name));
+                auto *resolved = var_resolve(name, envmap);
+                /* apply shell-like substitutions */
+                if (altmode == '-') {
+                    if (!resolved || (colon && !*resolved)) {
+                        r_line.append(altbeg, altend);
+                    } else if (resolved) {
+                        r_line.append(resolved);
+                    }
+                } else if (altmode == '+') {
+                    if (resolved && (!colon || *resolved)) {
+                        r_line.append(altbeg, altend);
+                    }
+                } else if (resolved) {
+                    r_line.append(resolved);
+                }
                 size_t line_len_after = r_line.size();
 
                 if (line_len_after > (size_t)std::numeric_limits<int>::max()) {
                     // (avoid potential overflow)
-                    throw service_description_exc(setting_name, "command line too long (after substitution)");
+                    throw service_description_exc(setting_name, "value too long (after substitution)");
                 }
 
                 xpos = i - line.begin();
@@ -734,7 +784,6 @@ static void cmdline_var_subst(const char *setting_name, std::string &line,
 
     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.
@@ -754,7 +803,7 @@ class service_settings_wrapper
     string pid_file;
     string env_file;
 
-    bool do_sub_vars = false;
+    bool export_passwd_vars = false;
 
     service_type_t service_type = service_type_t::INTERNAL;
     list<dep_type> depends;
@@ -806,7 +855,7 @@ class service_settings_wrapper
     // checks even when the dummy_lint function is used. (Ideally the compiler would optimise them away).
     template <typename T, typename U = decltype(dummy_lint), typename V = decltype(resolve_env_var),
             bool do_report_lint = !std::is_same<U, decltype(dummy_lint)>::value>
-    void finalise(T &report_error, U &report_lint = dummy_lint, V &var_subst = resolve_env_var)
+    void finalise(T &report_error, environment::env_map const &envmap, U &report_lint = dummy_lint, V &var_subst = resolve_env_var)
     {
         if (service_type == service_type_t::PROCESS || service_type == service_type_t::BGPROCESS
                 || service_type == service_type_t::SCRIPTED) {
@@ -883,9 +932,14 @@ class service_settings_wrapper
 
         // Resolve paths via variable substitution
         {
+            std::list<std::pair<unsigned, unsigned>> offsets;
+            /* reserve item */
+            offsets.emplace_back(0, 0);
             auto do_resolve = [&](const char *setting_name, string &setting_value) {
                 try {
-                    setting_value = resolve_path(setting_name, std::move(setting_value), var_subst);
+                    offsets.front().first = 0;
+                    offsets.front().second = setting_value.size();
+                    value_var_subst(setting_name, setting_value, offsets, var_subst, envmap);
                 }
                 catch (service_description_exc &exc) {
                     report_error((string() + setting_name + ": " + exc.exc_description).c_str());
@@ -894,7 +948,6 @@ class service_settings_wrapper
 
             do_resolve("socket-listen", socket_path);
             do_resolve("logfile", logfile);
-            do_resolve("env-file", env_file);
             do_resolve("working-dir", working_dir);
             do_resolve("pid-file", pid_file);
         }
@@ -1120,12 +1173,12 @@ void process_service_line(settings_wrapper &settings, const char *name, string &
         for (auto indexpair : indices) {
             string option_txt = load_opts.substr(indexpair.first,
                     indexpair.second - indexpair.first);
-            if (option_txt == "sub-vars") {
-                // substitute environment variables in command line
-                settings.do_sub_vars = true;
+            if (option_txt == "export-passwd-vars") {
+                settings.export_passwd_vars = true;
             }
-            else if (option_txt == "no-sub-vars") {
-                settings.do_sub_vars = false;
+            else if (option_txt == "sub-vars") {
+                // noop: for backwards compatibility only
+                // we don't support no-sub-vars anymore, however
             }
             else {
                 throw service_description_exc(name, "unknown load option: " + option_txt, line_num);

+ 7 - 0
src/includes/service.h

@@ -249,6 +249,8 @@ class service_record
     protected:
     service_flags_t onstart_flags;
     
+    environment service_env; // holds the environment populated during load
+
     bool auto_restart : 1;    // whether to restart this (process) if it dies unexpectedly
     bool smooth_recovery : 1; // whether the service process can restart without bringing down service
 
@@ -519,6 +521,11 @@ class service_record
         return start_explicit;
     }
 
+    void set_environment(environment &&env) noexcept
+    {
+        this->service_env = std::move(env);
+    }
+
     // Set whether this service should automatically restart when it dies
     void set_auto_restart(bool auto_restart) noexcept
     {

+ 37 - 13
src/load-service.cc

@@ -29,14 +29,13 @@ using string_iterator = std::string::iterator;
 //   offsets - the [start,end) pair of offsets of the command and each argument within the string
 //
 static void do_env_subst(const char *setting_name, ha_string &line,
-        std::list<std::pair<unsigned,unsigned>> &offsets, bool do_sub_vars)
+        std::list<std::pair<unsigned,unsigned>> &offsets,
+        environment::env_map const &envmap)
 {
     using namespace dinit_load;
-    if (do_sub_vars) {
-        std::string line_s = std::string(line.c_str(), line.length());
-        cmdline_var_subst(setting_name, line_s, offsets, resolve_env_var);
-        line = line_s;
-    }
+    std::string line_s = std::string(line.c_str(), line.length());
+    value_var_subst(setting_name, line_s, offsets, resolve_env_var, envmap);
+    line = line_s;
 }
 
 // Process a dependency directory - filenames contained within correspond to service names which
@@ -358,7 +357,31 @@ service_record * dirload_service_set::load_reload_service(const char *name, serv
             throw service_load_exc(name, msg);
         };
 
-        settings.finalise(report_err);
+        environment srv_env;
+
+        /* fill user vars before reading env file */
+        if (settings.export_passwd_vars) {
+            fill_environment_userinfo(settings.run_as_uid, name, srv_env);
+        }
+
+        // this mapping is temporary, for load substitutions
+        // the reason for this is that the environment actually *may* change
+        // after load, e.g. through dinitctl setenv (either from the outside
+        // or from within services) and we want this to refresh for each
+        // process invocation
+        environment::env_map srv_envmap;
+
+        if (!settings.env_file.empty()) {
+            try {
+                read_env_file(settings.env_file.data(), false, srv_env);
+            } catch (const std::system_error &se) {
+                throw service_load_exc(name, std::string("could not load environment file: ") + se.what());
+            }
+        }
+
+        srv_envmap = srv_env.build(main_env);
+
+        settings.finalise(report_err, srv_envmap);
         auto service_type = settings.service_type;
 
         if (reload_svc != nullptr) {
@@ -464,8 +487,8 @@ service_record * dirload_service_set::load_reload_service(const char *name, serv
         }
 
         if (service_type == service_type_t::PROCESS) {
-            do_env_subst("command", settings.command, settings.command_offsets, settings.do_sub_vars);
-            do_env_subst("stop-command", settings.stop_command, settings.stop_command_offsets, settings.do_sub_vars);
+            do_env_subst("command", settings.command, settings.command_offsets, srv_envmap);
+            do_env_subst("stop-command", settings.stop_command, settings.stop_command_offsets, srv_envmap);
             std::vector<const char *> stop_arg_parts = separate_args(settings.stop_command, settings.stop_command_offsets);
             process_service *rvalps;
             if (create_new_record) {
@@ -505,8 +528,8 @@ service_record * dirload_service_set::load_reload_service(const char *name, serv
             #endif
         }
         else if (service_type == service_type_t::BGPROCESS) {
-            do_env_subst("command", settings.command, settings.command_offsets, settings.do_sub_vars);
-            do_env_subst("stop-command", settings.stop_command, settings.stop_command_offsets, settings.do_sub_vars);
+            do_env_subst("command", settings.command, settings.command_offsets, srv_envmap);
+            do_env_subst("stop-command", settings.stop_command, settings.stop_command_offsets, srv_envmap);
             std::vector<const char *> stop_arg_parts = separate_args(settings.stop_command, settings.stop_command_offsets);
             bgproc_service *rvalps;
             if (create_new_record) {
@@ -542,8 +565,8 @@ service_record * dirload_service_set::load_reload_service(const char *name, serv
             settings.onstart_flags.runs_on_console = false;
         }
         else if (service_type == service_type_t::SCRIPTED) {
-            do_env_subst("command", settings.command, settings.command_offsets, settings.do_sub_vars);
-            do_env_subst("stop-command", settings.stop_command, settings.stop_command_offsets, settings.do_sub_vars);
+            do_env_subst("command", settings.command, settings.command_offsets, srv_envmap);
+            do_env_subst("stop-command", settings.stop_command, settings.stop_command_offsets, srv_envmap);
             std::vector<const char *> stop_arg_parts = separate_args(settings.stop_command, settings.stop_command_offsets);
             scripted_service *rvalps;
             if (create_new_record) {
@@ -599,6 +622,7 @@ service_record * dirload_service_set::load_reload_service(const char *name, serv
         rval->set_socket_details(std::move(settings.socket_path), settings.socket_perms,
                 settings.socket_uid, settings.socket_gid);
         rval->set_chain_to(std::move(settings.chain_to_name));
+        rval->set_environment(std::move(srv_env));
 
         if (create_new_record && reload_svc != nullptr) {
             // switch dependencies on old record so that they refer to the new record

+ 5 - 12
src/run-child-proc.cc

@@ -89,7 +89,6 @@ void base_process_service::run_child_proc(run_proc_params params) noexcept
     constexpr int csenvbufsz = 12 + ((CHAR_BIT * sizeof(int) - 1 + 2) / 3) + 1;
     char csenvbuf[csenvbufsz];
 
-    environment proc_env;
     environment::env_map proc_env_map;
 
     run_proc_err err;
@@ -143,12 +142,6 @@ void base_process_service::run_child_proc(run_proc_params params) noexcept
     }
 
     try {
-        // Read environment from file
-        if (params.env_file != nullptr && *params.env_file != 0) {
-            err.stage = exec_stage::READ_ENV_FILE;
-            read_env_file(params.env_file, false, proc_env);
-        }
-
         // Set up notify-fd variable:
         if (notify_var != nullptr && *notify_var != 0) {
             err.stage = exec_stage::SET_NOTIFYFD_VAR;
@@ -159,7 +152,7 @@ void base_process_service::run_child_proc(run_proc_params params) noexcept
             char * var_str = (char *) malloc(req_sz);
             if (var_str == nullptr) goto failure_out;
             snprintf(var_str, req_sz, "%s=%d", notify_var, notify_fd);
-            proc_env.set_var(var_str);
+            service_env.set_var(var_str);
         }
 
         // Set up Systemd-style socket activation:
@@ -170,20 +163,20 @@ void base_process_service::run_child_proc(run_proc_params params) noexcept
             if (dup2(socket_fd, 3) == -1) goto failure_out;
             if (socket_fd != 3) close(socket_fd);
 
-            proc_env.set_var("LISTEN_FDS=1");
+            service_env.set_var("LISTEN_FDS=1");
             snprintf(nbuf, bufsz, "LISTEN_PID=%jd", static_cast<intmax_t>(getpid()));
-            proc_env.set_var(nbuf);
+            service_env.set_var(nbuf);
         }
 
         if (csfd != -1) {
             err.stage = exec_stage::SETUP_CONTROL_SOCKET;
             snprintf(csenvbuf, csenvbufsz, "DINIT_CS_FD=%d", csfd);
-            proc_env.set_var(csenvbuf);
+            service_env.set_var(csenvbuf);
         }
 
         // We'll re-use READ_ENV_FILE stage here; it's accurate enough.
         err.stage = exec_stage::READ_ENV_FILE;
-        proc_env_map = proc_env.build(main_env);
+        proc_env_map = service_env.build(main_env);
     }
     catch (std::system_error &sys_err) {
         errno = sys_err.code().value();

+ 37 - 14
src/tests/loadtests.cc

@@ -11,9 +11,13 @@
 
 std::string test_service_dir;
 
+environment tenv;
+environment::env_map tenvmap;
+
 void init_test_service_dir()
 {
     test_service_dir = "./test-services";
+    tenvmap = tenv.build(main_env);
 }
 
 void test_basic()
@@ -26,39 +30,57 @@ void test_basic()
 void test_env_subst()
 {
     dirload_service_set sset(test_service_dir.c_str());
-    setenv("ONEVAR", "a", true);
-    setenv("TWOVAR", "hellohello", true);
+    bp_sys::setenv("ONEVAR", "a", true);
+    bp_sys::setenv("TWOVAR", "hellohello", true);
+    bp_sys::setenv("THREEVAR", "", true);
     // leave THREEVAR undefined
     auto t2 = static_cast<base_process_service *>(sset.load_service("t2"));
     auto exec_parts = t2->get_exec_arg_parts();
     assert(strcmp("echo", exec_parts[0]) == 0);
-    assert(strcmp("a", exec_parts[1]) == 0);
-    assert(strcmp("hellohello", exec_parts[2]) == 0);
-    assert(strcmp("", exec_parts[3]) == 0);
+    assert(strcmp("a", exec_parts[1]) == 0); // $ONEVAR
+    assert(strcmp("a", exec_parts[2]) == 0); // ${ONEVAR}
+    assert(strcmp("b", exec_parts[3]) == 0); // ${ONEVAR+b}
+    assert(strcmp("b", exec_parts[4]) == 0); // ${ONEVAR:+b}
+    assert(strcmp("hellohello", exec_parts[5]) == 0); // $TWOVAR
+    assert(strcmp("hellohello", exec_parts[6]) == 0); // ${TWOVAR}
+    assert(strcmp("hellohello", exec_parts[7]) == 0); // ${TWOVAR-world}
+    assert(strcmp("hellohello", exec_parts[8]) == 0); // ${TWOVAR:-world}
+    assert(strcmp("",      exec_parts[9]) == 0); // $THREEVAR
+    assert(strcmp("",      exec_parts[10]) == 0); // ${THREEVAR}
+    assert(strcmp("empty", exec_parts[11]) == 0); // ${THREEVAR+empty}
+    assert(strcmp("",      exec_parts[12]) == 0); // ${THREEVAR:+empty}
+    assert(strcmp("",      exec_parts[13]) == 0); // ${THREEVAR-empty}
+    assert(strcmp("empty", exec_parts[14]) == 0); // ${THREEVAR:-empty}
+    assert(strcmp("",       exec_parts[15]) == 0); // $FOURVAR
+    assert(strcmp("",       exec_parts[16]) == 0); // ${FOURVAR}
+    assert(strcmp("",       exec_parts[17]) == 0); // ${FOURVAR+empty2}
+    assert(strcmp("",       exec_parts[18]) == 0); // ${FOURVAR:+empty2}
+    assert(strcmp("empty2", exec_parts[19]) == 0); // ${FOURVAR-empty2}
+    assert(strcmp("empty2", exec_parts[20]) == 0); // ${FOURVAR:-empty2}
 }
 
 void test_env_subst2()
 {
-    auto resolve_env_var = [](const std::string &name){
+    auto resolve_env_var = [](const std::string &name, environment::env_map const &) {
         if (name == "ONE_VAR") return "a";
         if (name == "TWOVAR") return "hellohello";
         return "";
     };
 
-    std::string line = "test x$ONE_VAR~ y$TWOVAR$$ONE_VAR";
+    std::string line = "test x$ONE_VAR-${ONE_VAR}~ y$${TWOVAR}$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(1 /* line_num */, li, le, &offsets);
 
-    dinit_load::cmdline_var_subst("command", line, offsets, resolve_env_var);
+    dinit_load::value_var_subst("command", line, offsets, resolve_env_var, tenvmap);
 
-    assert(line == "test xa~ yhellohello$ONE_VAR");
+    assert(line == "test xa-a~ y${TWOVAR}hellohello$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}));
+    assert((*std::next(offsets.begin(), 1) == std::pair<unsigned,unsigned>{5, 10}));
+    assert((*std::next(offsets.begin(), 2) == std::pair<unsigned,unsigned>{11, 39}));
 }
 
 void test_nonexistent()
@@ -198,12 +220,12 @@ void test_path_env_subst()
 
     auto report_error = [](const char *msg) {};
 
-    auto resolve_var = [](const std::string &name) {
+    auto resolve_var = [](const std::string &name, environment::env_map const &) -> const char * {
         if (name == "username") return "testsuccess";
-        return "";
+        return nullptr;
     };
 
-    settings.finalise(report_error, report_error /* lint */, resolve_var);
+    settings.finalise(report_error, tenvmap, report_error /* lint */, resolve_var);
 
     assert(settings.service_type == service_type_t::PROCESS);
     assert(settings.command == "/something/test");
@@ -224,5 +246,6 @@ int main(int argc, char **argv)
     RUN_TEST(test_nonexistent, "          ");
     RUN_TEST(test_settings, "             ");
     RUN_TEST(test_path_env_subst, "       ");
+    bp_sys::clearenv();
     return 0;
 }

+ 1 - 2
src/tests/test-services/t2

@@ -1,3 +1,2 @@
 type = process
-load-options = sub-vars
-command = echo $ONEVAR $TWOVAR $THREEVAR
+command = echo $ONEVAR ${ONEVAR} ${ONEVAR+b} ${ONEVAR:+b} $TWOVAR ${TWOVAR} ${TWOVAR-world} ${TWOVAR:-world} $THREEVAR ${THREEVAR} ${THREEVAR+empty} ${THREEVAR:+empty} ${THREEVAR-empty} ${THREEVAR:-empty} $FOURVAR ${FOURVAR} ${FOURVAR+empty2} ${FOURVAR:+empty2} ${FOURVAR-empty2} ${FOURVAR:-empty2}