Skip to content

Commit

Permalink
Add BinaryDirectory parameter to specify a directory where binaries e…
Browse files Browse the repository at this point in the history
…xecuted by Exec*= should be found

Closes systemd#6308
  • Loading branch information
alexlzhu authored and alexlzhu committed Jul 17, 2021
1 parent a607df2 commit 32ad393
Show file tree
Hide file tree
Showing 15 changed files with 107 additions and 11 deletions.
10 changes: 10 additions & 0 deletions man/systemd.exec.xml
Expand Up @@ -89,6 +89,16 @@

<variablelist class='unit-directives'>

<varlistentry>
<term><varname>BinaryDirectory=</varname></term>

<listitem><para>Takes a directory path relative to which the executable passed to
<function>execve()</function> can be specified. The name of the executable has to be non standard.
At the moment BinaryDirectory will not take precedence over the default paths set at compilation time.
Thus it is not recommended to use this when calling standard linux binaries.
</para></listitem>
</varlistentry>

<varlistentry>
<term><varname>WorkingDirectory=</varname></term>

Expand Down
2 changes: 1 addition & 1 deletion src/analyze/analyze-verify.c
Expand Up @@ -124,7 +124,7 @@ int verify_executable(Unit *u, const ExecCommand *exec) {
if (exec->flags & EXEC_COMMAND_IGNORE_FAILURE)
return 0;

r = find_executable_full(exec->path, false, NULL, NULL);
r = find_executable_full(exec->path, NULL, false, NULL, NULL);
if (r < 0)
return log_unit_error_errno(u, r, "Command %s is not executable: %m", exec->path);

Expand Down
6 changes: 5 additions & 1 deletion src/basic/path-util.c
Expand Up @@ -637,7 +637,7 @@ static int check_x_access(const char *path, int *ret_fd) {
return 0;
}

int find_executable_full(const char *name, bool use_path_envvar, char **ret_filename, int *ret_fd) {
int find_executable_full(const char *name, char *binary_directory, bool use_path_envvar, char **ret_filename, int *ret_fd) {
int last_error, r;
const char *p = NULL;

Expand Down Expand Up @@ -669,6 +669,10 @@ int find_executable_full(const char *name, bool use_path_envvar, char **ret_file
if (!p)
p = DEFAULT_PATH;

if (binary_directory) {
p = binary_directory;
}

last_error = -ENOENT;

/* Resolve a single-component name to a full path */
Expand Down
4 changes: 2 additions & 2 deletions src/basic/path-util.h
Expand Up @@ -99,9 +99,9 @@ int path_strv_make_absolute_cwd(char **l);
char** path_strv_resolve(char **l, const char *root);
char** path_strv_resolve_uniq(char **l, const char *root);

int find_executable_full(const char *name, bool use_path_envvar, char **ret_filename, int *ret_fd);
int find_executable_full(const char *name, char *binary_directory, bool use_path_envvar, char **ret_filename, int *ret_fd);
static inline int find_executable(const char *name, char **ret_filename) {
return find_executable_full(name, true, ret_filename, NULL);
return find_executable_full(name, NULL, true, ret_filename, NULL);
}

bool paths_check_timestamp(const char* const* paths, usec_t *paths_ts_usec, bool update);
Expand Down
17 changes: 17 additions & 0 deletions src/core/dbus-execute.c
Expand Up @@ -1090,6 +1090,7 @@ const sd_bus_vtable bus_exec_vtable[] = {
SD_BUS_PROPERTY("LimitRTPRIOSoft", "t", bus_property_get_rlimit, offsetof(ExecContext, rlimit[RLIMIT_RTPRIO]), SD_BUS_VTABLE_PROPERTY_CONST),
SD_BUS_PROPERTY("LimitRTTIME", "t", bus_property_get_rlimit, offsetof(ExecContext, rlimit[RLIMIT_RTTIME]), SD_BUS_VTABLE_PROPERTY_CONST),
SD_BUS_PROPERTY("LimitRTTIMESoft", "t", bus_property_get_rlimit, offsetof(ExecContext, rlimit[RLIMIT_RTTIME]), SD_BUS_VTABLE_PROPERTY_CONST),
SD_BUS_PROPERTY("BinaryDirectory", "s", NULL, 0, SD_BUS_VTABLE_PROPERTY_CONST),

This comment has been minimized.

Copy link
@anitazha

anitazha Jul 19, 2021

Did changing this to SD_BUS_PROPERTY("BinaryDirectory", "s", NULL, offsetof(ExecContext, binary_directory), SD_BUS_VTABLE_PROPERTY_CONST), not work?

This comment has been minimized.

Copy link
@alexlzhu

alexlzhu Jul 19, 2021

Owner

That works as well. How was it working when the offset was 0?

This comment has been minimized.

Copy link
@anitazha

anitazha Jul 19, 2021

Hmmm I'm not sure an offset of 0 would have worked. This is the "getter" path for BinaryDirectory= so I don't think you would have run into issues unless you tried to read the dbus property (i.e. with busctl to examine the bus. systemctl show -p BinaryDirectory <service> might also go through this path, not sure). I'm expecting it would have returned gibberish with no offset

SD_BUS_PROPERTY("WorkingDirectory", "s", property_get_working_directory, 0, SD_BUS_VTABLE_PROPERTY_CONST),
SD_BUS_PROPERTY("RootDirectory", "s", NULL, offsetof(ExecContext, root_directory), SD_BUS_VTABLE_PROPERTY_CONST),
SD_BUS_PROPERTY("RootImage", "s", NULL, offsetof(ExecContext, root_image), SD_BUS_VTABLE_PROPERTY_CONST),
Expand Down Expand Up @@ -2635,6 +2636,22 @@ int bus_exec_context_set_transient_property(

return 1;

} else if (streq(name, "BinaryDirectory")) {
const char *s;
bool missing_ok;

This comment has been minimized.

Copy link
@anitazha

anitazha Jul 19, 2021

Same here missing_ok is not initialized and isn't relevant in BinaryDirectory= so please remove it.


r = sd_bus_message_read(message, "s", &s);
if (r < 0)
return r;

if (!UNIT_WRITE_FLAGS_NOOP(flags)) {
r = free_and_strdup(&c->binary_directory, empty_to_null(s));
if (r < 0)
return r;
unit_write_settingf(u, flags|UNIT_ESCAPE_SPECIFIERS, name, "BinaryDirectory=%s%s", missing_ok ? "-" : "", s);
}

return 1;
} else if (streq(name, "WorkingDirectory")) {
const char *s;
bool missing_ok;
Expand Down
4 changes: 3 additions & 1 deletion src/core/execute.c
Expand Up @@ -4316,7 +4316,7 @@ static int exec_child(

_cleanup_free_ char *executable = NULL;
_cleanup_close_ int executable_fd = -1;
r = find_executable_full(command->path, false, &executable, &executable_fd);
r = find_executable_full(command->path, context->binary_directory, false, &executable, &executable_fd);
if (r < 0) {
if (r != -ENOMEM && (command->flags & EXEC_COMMAND_IGNORE_FAILURE)) {
log_unit_struct_errno(unit, LOG_INFO, r,
Expand Down Expand Up @@ -5260,6 +5260,7 @@ void exec_context_dump(const ExecContext *c, FILE* f, const char *prefix) {

fprintf(f,
"%sUMask: %04o\n"
"%sBinaryDirectory: %s\n"
"%sWorkingDirectory: %s\n"
"%sRootDirectory: %s\n"
"%sNonBlocking: %s\n"
Expand All @@ -5284,6 +5285,7 @@ void exec_context_dump(const ExecContext *c, FILE* f, const char *prefix) {
"%sProtectProc: %s\n"
"%sProcSubset: %s\n",
prefix, c->umask,
prefix, empty_to_root(c->binary_directory),
prefix, empty_to_root(c->working_directory),
prefix, empty_to_root(c->root_directory),
prefix, yes_no(c->non_blocking),
Expand Down
2 changes: 1 addition & 1 deletion src/core/execute.h
Expand Up @@ -167,7 +167,7 @@ struct ExecContext {
char **unset_environment;

struct rlimit *rlimit[_RLIMIT_MAX];
char *working_directory, *root_directory, *root_image, *root_verity, *root_hash_path, *root_hash_sig_path;
char *binary_directory, *working_directory, *root_directory, *root_image, *root_verity, *root_hash_path, *root_hash_sig_path;
void *root_hash, *root_hash_sig;
size_t root_hash_size, root_hash_sig_size;
LIST_HEAD(MountOptions, root_image_options);
Expand Down
1 change: 1 addition & 0 deletions src/core/load-fragment-gperf.gperf.in
Expand Up @@ -2,6 +2,7 @@

{%- macro EXEC_CONTEXT_CONFIG_ITEMS(type) -%}
{# Define the context options only once #}
{{type}}.BinaryDirectory, config_parse_binary_directory, 0, offsetof({{type}}, exec_context)

This comment has been minimized.

Copy link
@anitazha

anitazha Jul 19, 2021

Can you elaborate why config_parse_unit_path_printf does not work here? Or even config_parse_string? I'm wondering what issues show up since code-wise it looks like they should be interchangeable.

This comment has been minimized.

Copy link
@alexlzhu

alexlzhu Jul 19, 2021

Owner
Jul 19 15:36:45 image systemd[1]: Starting Hostname Service...                                                             │     0,                                  offsetof({{type}}, exec_context)
Jul 19 15:36:45 image systemd[57]: Failed to configure loopback device: Operation not permitted                            │  24 {{type}}.CPUSchedulingPriority,            config_parse_exec_cpu_sched_prio,
Jul 19 15:36:46 image systemd[1]: Started Hostname Service.                                                                │     0,                                  offsetof({{type}}, exec_context)
Jul 19 15:37:16 image systemd[1]: systemd-hostnamed.service: Deactivated successfully.                                     │  25 {{type}}.CPUSchedulingResetOnFork,         config_parse_bool,
Jul 19 15:41:55 image systemd[45]: Created slice User Background Tasks Slice.                                              │     0,                                  offsetof({{type}}, exec_context.cpu_sched_reset_on_fork)
Jul 19 15:41:55 image systemd[45]: Starting Cleanup of User's Temporary Files and Directories...                           │  26 {{type}}.CPUAffinity,                      config_parse_exec_cpu_affinity,
Jul 19 15:41:55 image systemd[45]: Finished Cleanup of User's Temporary Files and Directories.                             │     0,                                  offsetof({{type}}, exec_context)
Jul 19 15:43:04 image systemd[1]: Starting test.service...                                                                 │  27 {{type}}.NUMAPolicy,                       config_parse_numa_policy,
Jul 19 15:43:04 image systemd[1]: test.service: Main process exited, code=dumped, status=11/SEGV                           │     0,                                  offsetof({{type}}, exec_context.numa_policy.type)
Jul 19 15:43:04 image systemd[1]: test.service: Control process exited, code=dumped, status=11/SEGV                        │ @
Jul 19 15:43:04 image systemd[1]: test.service: Failed with result 'core-dump'.                                            │ NORMAL > test > src/core/load-fragment-gperf.gperf.in                                    << utf-8[unix] <   1% :   7:  1  <
Jul 19 15:43:04 image systemd[1]: Failed to start test.service.```

This comment has been minimized.

Copy link
@alexlzhu

alexlzhu Jul 19, 2021

Owner
[root@image ~]# systemd status test                                                                                        │     0,                                  offsetof({{type}}, exec_context)
-bash: systemd: command not found                                                                                          │  22 {{type}}.IOSchedulingPriority,             config_parse_exec_io_priority,
[root@image ~]# systemctl status test                                                                                      │     0,                                  offsetof({{type}}, exec_context)
                                                                                                                           │  23 {{type}}.CPUSchedulingPolicy,              config_parse_exec_cpu_sched_policy,
Broadcast message from systemd-journald@image (Mon 2021-07-19 15:44:08 PDT):                                               │     0,                                  offsetof({{type}}, exec_context)
                                                                                                                           │  24 {{type}}.CPUSchedulingPriority,            config_parse_exec_cpu_sched_prio,
systemd[1]: Caught <SEGV>, dumped core as pid 90.                                                                          │     0,                                  offsetof({{type}}, exec_context)
                                                                                                                           │  25 {{type}}.CPUSchedulingResetOnFork,         config_parse_bool,
                                                                                                                           │     0,                                  offsetof({{type}}, exec_context.cpu_sched_reset_on_fork)
Broadcast message from systemd-journald@image (Mon 2021-07-19 15:44:08 PDT):                                               │  26 {{type}}.CPUAffinity,                      config_parse_exec_cpu_affinity,
                                                                                                                           │     0,                                  offsetof({{type}}, exec_context)
systemd[1]: Exiting PID 1...                                                                                               │  27 {{type}}.NUMAPolicy,                       config_parse_numa_policy,
                                                                                                                           │     0,                                  offsetof({{type}}, exec_context.numa_policy.type)
Failed to get properties: Connection reset by peer                                                                         │ @
[root@image ~]#                                                                                                            │ NORMAL > test > src/core/load-fragment-gperf.gperf.in                                    << utf-8[unix] <   1% :   7:  1  <
Container image failed with error code 255.
```

This comment has been minimized.

Copy link
@anitazha

anitazha Jul 19, 2021

Ooooh I see. It's because the offset needs to use exec_context.binary_directory, not exec_context. config_parse_unit_path_printf has to point the exact struct element.

{{type}}.WorkingDirectory, config_parse_working_directory, 0, offsetof({{type}}, exec_context)
{{type}}.RootDirectory, config_parse_unit_path_printf, true, offsetof({{type}}, exec_context.root_directory)
{{type}}.RootImage, config_parse_unit_path_printf, true, offsetof({{type}}, exec_context.root_image)
Expand Down
43 changes: 43 additions & 0 deletions src/core/load-fragment.c
Expand Up @@ -2509,6 +2509,49 @@ int config_parse_user_group_strv_compat(
}
}

int config_parse_binary_directory(
const char *unit,
const char *filename,
unsigned line,
const char *section,
unsigned section_line,
const char *lvalue,
int ltype,
const char *rvalue,
void *data,
void *userdata) {

ExecContext *c = data;
const Unit *u = userdata;
bool missing_ok;

This comment has been minimized.

Copy link
@anitazha

anitazha Jul 19, 2021

missing_ok is never initialized and has no meaning in BinaryDirectory=. Please remove all uses of it in the BinaryDirectory= logic.

int r;

assert(filename);
assert(lvalue);
assert(rvalue);
assert(c);
assert(u);


_cleanup_free_ char *k = NULL;

r = unit_path_printf(u, rvalue, &k);
if (r < 0) {
log_syntax(unit, missing_ok ? LOG_WARNING : LOG_ERR, filename, line, r,
"Failed to resolve unit specifiers in binary directory path '%s'%s: %m",
rvalue, missing_ok ? ", ignoring" : "");
return missing_ok ? 0 : -ENOEXEC;
}

r = path_simplify_and_warn(k, PATH_CHECK_ABSOLUTE | (missing_ok ? 0 : PATH_CHECK_FATAL), unit, filename, line, lvalue);
if (r < 0)
return missing_ok ? 0 : -ENOEXEC;

free_and_replace(c->binary_directory, k);

return 0;
}

int config_parse_working_directory(
const char *unit,
const char *filename,
Expand Down
1 change: 1 addition & 0 deletions src/core/load-fragment.h
Expand Up @@ -107,6 +107,7 @@ CONFIG_PARSER_PROTOTYPE(config_parse_protect_home);
CONFIG_PARSER_PROTOTYPE(config_parse_protect_system);
CONFIG_PARSER_PROTOTYPE(config_parse_bus_name);
CONFIG_PARSER_PROTOTYPE(config_parse_exec_utmp_mode);
CONFIG_PARSER_PROTOTYPE(config_parse_binary_directory);
CONFIG_PARSER_PROTOTYPE(config_parse_working_directory);
CONFIG_PARSER_PROTOTYPE(config_parse_fdname);
CONFIG_PARSER_PROTOTYPE(config_parse_sec_fix_0);
Expand Down
1 change: 1 addition & 0 deletions src/nspawn/nspawn-gperf.gperf
Expand Up @@ -31,6 +31,7 @@ Exec.DropCapability, config_parse_capability, 0, of
Exec.KillSignal, config_parse_signal, 0, offsetof(Settings, kill_signal)
Exec.Personality, config_parse_personality, 0, offsetof(Settings, personality)
Exec.MachineID, config_parse_id128, 0, offsetof(Settings, machine_id)
Exec.BinaryDirectory, config_parse_path, 0, offsetof(Settings, binary_directory)
Exec.WorkingDirectory, config_parse_path, 0, offsetof(Settings, working_directory)
Exec.PivotRoot, config_parse_pivot_root, 0, 0
Exec.PrivateUsers, config_parse_private_users, 0, 0
Expand Down
2 changes: 2 additions & 0 deletions src/nspawn/nspawn-settings.h
Expand Up @@ -128,6 +128,7 @@ typedef enum SettingsMask {
SETTING_CREDENTIALS = UINT64_C(1) << 30,
SETTING_BIND_USER = UINT64_C(1) << 31,
SETTING_RLIMIT_FIRST = UINT64_C(1) << 32, /* we define one bit per resource limit here */
SETTING_BINARY_DIRECTORY = UINT64_C(1) << 33,
SETTING_RLIMIT_LAST = UINT64_C(1) << (32 + _RLIMIT_MAX - 1),
_SETTINGS_MASK_ALL = (UINT64_C(1) << (32 + _RLIMIT_MAX)) -1,
_SETTING_FORCE_ENUM_WIDTH = UINT64_MAX
Expand Down Expand Up @@ -171,6 +172,7 @@ typedef struct Settings {
int kill_signal;
unsigned long personality;
sd_id128_t machine_id;
char *binary_directory;
char *working_directory;
char *pivot_root_new;
char *pivot_root_old;
Expand Down
1 change: 1 addition & 0 deletions src/run/run.c
Expand Up @@ -1718,6 +1718,7 @@ static int run(int argc, char* argv[]) {
if (!strv_isempty(arg_cmdline) &&
arg_transport == BUS_TRANSPORT_LOCAL &&
!strv_find_startswith(arg_property, "RootDirectory=") &&
!strv_find_startswith(arg_property, "BinaryDirectory=") &&
!strv_find_startswith(arg_property, "RootImage=")) {
/* Patch in an absolute path to fail early for user convenience, but only when we can do it
* (i.e. we will be running from the same file system). This also uses the user's $PATH,
Expand Down
1 change: 1 addition & 0 deletions src/shared/bus-unit-util.c
Expand Up @@ -913,6 +913,7 @@ static int bus_append_execute_property(sd_bus_message *m, const char *field, con
"UtmpMode",
"PAMName",
"TTYPath",
"BinaryDirectory",
"WorkingDirectory",
"RootDirectory",
"SyslogIdentifier",
Expand Down
23 changes: 18 additions & 5 deletions src/test/test-path-util.c
Expand Up @@ -204,12 +204,12 @@ static void test_find_executable_full(void) {

log_info("/* %s */", __func__);

assert_se(find_executable_full("sh", true, &p, NULL) == 0);
assert_se(find_executable_full("sh", NULL, true, &p, NULL) == 0);
puts(p);
assert_se(streq(basename(p), "sh"));
free(p);

assert_se(find_executable_full("sh", false, &p, NULL) == 0);
assert_se(find_executable_full("sh", NULL, false, &p, NULL) == 0);
puts(p);
assert_se(streq(basename(p), "sh"));
free(p);
Expand All @@ -221,18 +221,31 @@ static void test_find_executable_full(void) {

assert_se(unsetenv("PATH") == 0);

assert_se(find_executable_full("sh", true, &p, NULL) == 0);
assert_se(find_executable_full("sh", NULL, true, &p, NULL) == 0);
puts(p);
assert_se(streq(basename(p), "sh"));
free(p);

assert_se(find_executable_full("sh", false, &p, NULL) == 0);
assert_se(find_executable_full("sh", NULL, false, &p, NULL) == 0);
puts(p);
assert_se(streq(basename(p), "sh"));
free(p);

if (oldpath)
assert_se(setenv("PATH", oldpath, true) >= 0);

system("touch /tmp/temp_file");

This comment has been minimized.

Copy link
@anitazha

anitazha Jul 19, 2021

Using mkostemp_safe() to create a random directory would be better so the files don't conflict in the even that this test is run with multiple instances in parallel. For example: https://github.com/systemd/systemd/blob/main/src/test/test-fileio.c#L405-L412. Also there is a risk of the file leaking (i.e. not getting cleaned up) if execution was shut down part way through the test. The _cleanup_(unlink_tempfilep) will take care of that.

system("chmod +x /tmp/temp_file");

This comment has been minimized.

Copy link
@anitazha

anitazha Jul 19, 2021

With C we have access to the underlying library functions there is no need to use system() calls. You can call chmod() directly and check for errors.


assert_se(find_executable_full("temp_file", "/tmp", false, &p, NULL) == 0);
puts(p);
assert_se(streq(p, "/tmp/temp_file"));
free(p);

system("rm /tmp/temp_file");

This comment has been minimized.

Copy link
@anitazha

anitazha Jul 19, 2021

Use unlink()

assert_se(find_executable_full("temp_file", "/tmp", false, &p, NULL) != 0);
puts(p);

}

static void test_find_executable(const char *self) {
Expand Down Expand Up @@ -277,7 +290,7 @@ static void test_find_executable_exec_one(const char *path) {
pid_t pid;
int r;

r = find_executable_full(path, false, &t, &fd);
r = find_executable_full(path, NULL, false, &t, &fd);

log_info_errno(r, "%s: %s → %s: %d/%m", __func__, path, t ?: "-", fd);

Expand Down

0 comments on commit 32ad393

Please sign in to comment.