From 62c460c6e2ebb4da8f5f95f96973e317298a0afa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 23 Jun 2017 19:17:40 -0400 Subject: [PATCH 1/4] manager: raise level of notices about failed deserialization This is something that shouldn't happen. If it does, we want to know about it. --- src/core/manager.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index 8b818bec91..876210f353 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -2610,7 +2610,7 @@ int manager_deserialize(Manager *m, FILE *f, FDSet *fds) { uint32_t id; if (safe_atou32(val, &id) < 0) - log_debug("Failed to parse current job id value %s", val); + log_notice("Failed to parse current job id value %s", val); else m->current_job_id = MAX(m->current_job_id, id); @@ -2618,7 +2618,7 @@ int manager_deserialize(Manager *m, FILE *f, FDSet *fds) { uint32_t n; if (safe_atou32(val, &n) < 0) - log_debug("Failed to parse installed jobs counter %s", val); + log_notice("Failed to parse installed jobs counter %s", val); else m->n_installed_jobs += n; @@ -2626,7 +2626,7 @@ int manager_deserialize(Manager *m, FILE *f, FDSet *fds) { uint32_t n; if (safe_atou32(val, &n) < 0) - log_debug("Failed to parse failed jobs counter %s", val); + log_notice("Failed to parse failed jobs counter %s", val); else m->n_failed_jobs += n; @@ -2635,7 +2635,7 @@ int manager_deserialize(Manager *m, FILE *f, FDSet *fds) { b = parse_boolean(val); if (b < 0) - log_debug("Failed to parse taint /usr flag %s", val); + log_notice("Failed to parse taint /usr flag %s", val); else m->taint_usr = m->taint_usr || b; @@ -2672,7 +2672,7 @@ int manager_deserialize(Manager *m, FILE *f, FDSet *fds) { int fd; if (safe_atoi(val, &fd) < 0 || fd < 0 || !fdset_contains(fds, fd)) - log_debug("Failed to parse notify fd: %s", val); + log_notice("Failed to parse notify fd: \"%s\"", val); else { m->notify_event_source = sd_event_source_unref(m->notify_event_source); safe_close(m->notify_fd); @@ -2695,7 +2695,7 @@ int manager_deserialize(Manager *m, FILE *f, FDSet *fds) { int fd; if (safe_atoi(val, &fd) < 0 || fd < 0 || !fdset_contains(fds, fd)) - log_debug("Failed to parse cgroups agent fd: %s", val); + log_notice("Failed to parse cgroups agent fd: %s", val); else { m->cgroups_agent_event_source = sd_event_source_unref(m->cgroups_agent_event_source); safe_close(m->cgroups_agent_fd); @@ -2706,7 +2706,7 @@ int manager_deserialize(Manager *m, FILE *f, FDSet *fds) { int fd0, fd1; if (sscanf(val, "%i %i", &fd0, &fd1) != 2 || fd0 < 0 || fd1 < 0 || fd0 == fd1 || !fdset_contains(fds, fd0) || !fdset_contains(fds, fd1)) - log_debug("Failed to parse user lookup fd: %s", val); + log_notice("Failed to parse user lookup fd: %s", val); else { m->user_lookup_event_source = sd_event_source_unref(m->user_lookup_event_source); safe_close_pair(m->user_lookup_fds); @@ -2726,7 +2726,7 @@ int manager_deserialize(Manager *m, FILE *f, FDSet *fds) { log_oom(); } else if (!startswith(l, "kdbus-fd=")) /* ignore this one */ - log_debug("Unknown serialization item '%s'", l); + log_notice("Unknown serialization item '%s'", l); } for (;;) { From d233c99ac8125daa04e87f67cf13ee01fe3d9a1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 23 Jun 2017 19:20:54 -0400 Subject: [PATCH 2/4] manager: just warn about an invalid environment entry Apart from bugs (as in #6152), this can happen if we ever make our requirements for environment entries more stringent. As with the rest of deserialization, we should just warn and continue. --- src/basic/env-util.c | 2 +- src/core/manager.c | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/basic/env-util.c b/src/basic/env-util.c index e79b441ab6..c2a6f6f56e 100644 --- a/src/basic/env-util.c +++ b/src/basic/env-util.c @@ -788,7 +788,7 @@ int serialize_environment(FILE *f, char **environment) { } int deserialize_environment(char ***environment, const char *line) { - char *uce = NULL; + char *uce; int r; assert(line); diff --git a/src/core/manager.c b/src/core/manager.c index 876210f353..283720750f 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -2588,8 +2588,8 @@ int manager_deserialize(Manager *m, FILE *f, FDSet *fds) { m->n_reloading++; for (;;) { - char line[LINE_MAX], *l; - const char *val; + char line[LINE_MAX]; + const char *val, *l; if (!fgets(line, sizeof(line), f)) { if (feof(f)) @@ -2665,8 +2665,10 @@ int manager_deserialize(Manager *m, FILE *f, FDSet *fds) { dual_timestamp_deserialize(val, &m->units_load_finish_timestamp); else if (startswith(l, "env=")) { r = deserialize_environment(&m->environment, l); + if (r == -ENOMEM) + goto finish; if (r < 0) - return r; + log_notice_errno(r, "Failed to parse environment entry: \"%s\": %m", l); } else if ((val = startswith(l, "notify-fd="))) { int fd; From 417eefb17029bdabb2aafe0a6fe39c178aa497b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 23 Jun 2017 20:27:12 -0400 Subject: [PATCH 3/4] basic/env-util: when serializing, actually use escaping Fixes #6152. --- src/basic/env-util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/basic/env-util.c b/src/basic/env-util.c index c2a6f6f56e..56e7b6fd8c 100644 --- a/src/basic/env-util.c +++ b/src/basic/env-util.c @@ -779,7 +779,7 @@ int serialize_environment(FILE *f, char **environment) { if (!ce) return -ENOMEM; - fprintf(f, "env=%s\n", *e); + fprintf(f, "env=%s\n", ce); } /* caller should call ferror() */ From a096d8c88f65dcba78bcd34048f463dfd7783046 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 23 Jun 2017 20:43:48 -0400 Subject: [PATCH 4/4] tests: add tests for environment serialization --- src/test/test-env-util.c | 45 ++++++++++++++++++++++++++++++++++++++++ src/test/test-escape.c | 8 +++++++ 2 files changed, 53 insertions(+) diff --git a/src/test/test-env-util.c b/src/test/test-env-util.c index 904c50f0ed..3a2492dc6f 100644 --- a/src/test/test-env-util.c +++ b/src/test/test-env-util.c @@ -21,6 +21,8 @@ #include #include "env-util.h" +#include "fd-util.h" +#include "fileio.h" #include "string-util.h" #include "strv.h" #include "util.h" @@ -323,6 +325,48 @@ static void test_deserialize_environment(void) { assert_se(strv_equal(env, STRV_MAKE("A=1", "B=2"))); } +static void test_serialize_environment(void) { + char fn[] = "/tmp/test-env-util.XXXXXXX"; + int fd, r; + _cleanup_fclose_ FILE *f = NULL; + + _cleanup_strv_free_ char **env = strv_new("A=1", + "B=2", + "C=ąęółń", + "D=D=a\\x0Ab", + NULL); + _cleanup_strv_free_ char **env2 = NULL; + + fd = mkostemp_safe(fn); + assert_se(fd >= 0); + + assert_se(f = fdopen(fd, "r+")); + + assert_se(serialize_environment(f, env) == 0); + assert_se(fflush_and_check(f) == 0); + + rewind(f); + + for (;;) { + char line[LINE_MAX]; + const char *l; + + if (!fgets(line, sizeof line, f)) + break; + + char_array_0(line); + l = strstrip(line); + + r = deserialize_environment(&env2, l); + assert_se(r == 1); + } + assert_se(feof(f)); + + assert_se(strv_equal(env, env2)); + + unlink(fn); +} + int main(int argc, char *argv[]) { test_strv_env_delete(); test_strv_env_get(); @@ -340,6 +384,7 @@ int main(int argc, char *argv[]) { test_env_value_is_valid(); test_env_assignment_is_valid(); test_deserialize_environment(); + test_serialize_environment(); return 0; } diff --git a/src/test/test-escape.c b/src/test/test-escape.c index 181d7db5b4..d060afaffa 100644 --- a/src/test/test-escape.c +++ b/src/test/test-escape.c @@ -69,6 +69,14 @@ static void test_cunescape(void) { assert_se(cunescape("\\073", 0, &unescaped) >= 0); assert_se(streq_ptr(unescaped, ";")); + unescaped = mfree(unescaped); + + assert_se(cunescape("A=A\\\\x0aB", 0, &unescaped) >= 0); + assert_se(streq_ptr(unescaped, "A=A\\x0aB")); + unescaped = mfree(unescaped); + + assert_se(cunescape("A=A\\\\x0aB", UNESCAPE_RELAX, &unescaped) >= 0); + assert_se(streq_ptr(unescaped, "A=A\\x0aB")); } static void test_shell_escape_one(const char *s, const char *bad, const char *expected) {