From e225e5c3c63822c51756ea4eb3545522523dbc1f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 16 May 2018 22:14:59 -0400 Subject: [PATCH 1/5] process-util: mention that wait_for_terminate_with_timeout() should be called with SIGCHLD blocked --- src/basic/process-util.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/basic/process-util.c b/src/basic/process-util.c index 65abaaf888..a503aa50dc 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -740,14 +740,17 @@ int wait_for_terminate_and_check(const char *name, pid_t pid, WaitFlags flags) { /* * Return values: - * < 0 : wait_for_terminate_with_timeout() failed to get the state of the - * process, the process timed out, the process was terminated by a - * signal, or failed for an unknown reason. + * + * < 0 : wait_for_terminate_with_timeout() failed to get the state of the process, the process timed out, the process + * was terminated by a signal, or failed for an unknown reason. + * * >=0 : The process terminated normally with no failures. * - * Success is indicated by a return value of zero, a timeout is indicated - * by ETIMEDOUT, and all other child failure states are indicated by error - * is indicated by a non-zero value. + * Success is indicated by a return value of zero, a timeout is indicated by ETIMEDOUT, and all other child failure + * states are indicated by error is indicated by a non-zero value. + * + * This call assumes SIGCHLD has been blocked already, in particular before the child to wait for has been forked off + * to remain entirely race-free. */ int wait_for_terminate_with_timeout(pid_t pid, usec_t timeout) { sigset_t mask; From 92f14395cd43bb7969d3c7edaa53100d2f3c31d2 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 1 Jun 2018 21:43:43 +0200 Subject: [PATCH 2/5] missing: define kernel internal limit TASK_COMM_LEN in userspace too We already use it at two places, and we are about to add one too. Arbitrary literally hardcoded limits suck. --- src/basic/missing.h | 7 +++++++ src/basic/process-util.c | 2 +- src/test/test-process-util.c | 2 +- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/basic/missing.h b/src/basic/missing.h index e714078f31..dbdbd6f260 100644 --- a/src/basic/missing.h +++ b/src/basic/missing.h @@ -1407,4 +1407,11 @@ struct statx { #define AT_STATX_DONT_SYNC 0x4000 #endif +/* The maximum thread/process name length including trailing NUL byte. This mimics the kernel definition of the same + * name, which we need in userspace at various places but is not defined in userspace currently, neither under this + * name nor any other. */ +#ifndef TASK_COMM_LEN +#define TASK_COMM_LEN 16 +#endif + #include "missing_syscall.h" diff --git a/src/basic/process-util.c b/src/basic/process-util.c index a503aa50dc..058f077de0 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -296,7 +296,7 @@ int rename_process(const char name[]) { * can use PR_SET_NAME, which sets the thread name for the calling thread. */ if (prctl(PR_SET_NAME, name) < 0) log_debug_errno(errno, "PR_SET_NAME failed: %m"); - if (l > 15) /* Linux process names can be 15 chars at max */ + if (l >= TASK_COMM_LEN) /* Linux process names can be 15 chars at max */ truncated = true; /* Second step, change glibc's ID of the process name. */ diff --git a/src/test/test-process-util.c b/src/test/test-process-util.c index fd5b44dc13..8b975ff988 100644 --- a/src/test/test-process-util.c +++ b/src/test/test-process-util.c @@ -366,7 +366,7 @@ static void test_rename_process_now(const char *p, int ret) { assert_se(get_process_comm(0, &comm) >= 0); log_info("comm = <%s>", comm); - assert_se(strneq(comm, p, 15)); + assert_se(strneq(comm, p, TASK_COMM_LEN-1)); assert_se(get_process_cmdline(0, 0, false, &cmdline) >= 0); /* we cannot expect cmdline to be renamed properly without privileges */ From 76a359736f0f57d8a77b143fe682eceed51d9cda Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 1 Jun 2018 21:45:06 +0200 Subject: [PATCH 3/5] escape: add an explanatory comment about buffer sizes --- src/basic/escape.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/basic/escape.c b/src/basic/escape.c index 2e605b2ebe..6734fba1f3 100644 --- a/src/basic/escape.c +++ b/src/basic/escape.c @@ -18,6 +18,8 @@ int cescape_char(char c, char *buf) { char *buf_old = buf; + /* Needs space for 4 characters in the buffer */ + switch (c) { case '\a': From 61f6e276714c0bc5c43f7f0c9daf1863f3958987 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 1 Jun 2018 21:45:23 +0200 Subject: [PATCH 4/5] string-util: tweak cellescape() a bit For short buffer sizes cellescape() was a bit wasteful, as it might suffice to to drop a single character to find enough place for the full four byte ellipsis, if that one character was a four character escape. With this rework we'll guarantee to drop the minimum number of characters from the end to fit in the ellipsis. If the buffers we write to are large this doesn't matter much. However, if they are short (as they are when talking about the process comm field) then it starts to matter that we put as much information as we can in the space we get. --- src/basic/string-util.c | 57 ++++++++++++++++++++++------ src/test/test-string-util.c | 76 ++++++++++++++++++++++++++++++++----- 2 files changed, 112 insertions(+), 21 deletions(-) diff --git a/src/basic/string-util.c b/src/basic/string-util.c index b06b50397e..4c7ab3c4d3 100644 --- a/src/basic/string-util.c +++ b/src/basic/string-util.c @@ -627,25 +627,58 @@ char *cellescape(char *buf, size_t len, const char *s) { * very end. */ - size_t i; - const char *t = s; + size_t i = 0, last_char_width[4] = {}, k = 0, j; - assert(len > 4 + 4 + 1); /* two chars and the terminator */ + assert(len > 0); /* at least a terminating NUL */ - for (i = 0; i < len - 9; t++) { - if (!*t) + for (;;) { + char four[4]; + int w; + + if (*s == 0) /* terminating NUL detected? then we are done! */ goto done; - i += cescape_char(*t, buf + i); + + w = cescape_char(*s, four); + if (i + w + 1 > len) /* This character doesn't fit into the buffer anymore? In that case let's + * ellipsize at the previous location */ + break; + + /* OK, there was space, let's add this escaped character to the buffer */ + memcpy(buf + i, four, w); + i += w; + + /* And remember its width in the ring buffer */ + last_char_width[k] = w; + k = (k + 1) % 4; + + s++; } - /* We have space for one more char and terminating nul at this point */ - if (*t) { - if (*(t+1)) - i += write_ellipsis(buf + i, false); - else - i += cescape_char(*t, buf + i); + /* Ellipsation is necessary. This means we might need to truncate the string again to make space for 4 + * characters ideally, but the buffer is shorter than that in the first place take what we can get */ + for (j = 0; j < ELEMENTSOF(last_char_width); j++) { + + if (i + 4 <= len) /* nice, we reached our space goal */ + break; + + k = k == 0 ? 3 : k - 1; + if (last_char_width[k] == 0) /* bummer, we reached the beginning of the strings */ + break; + + assert(i >= last_char_width[k]); + i -= last_char_width[k]; } + if (i + 4 <= len) /* yay, enough space */ + i += write_ellipsis(buf + i, false); + else if (i + 3 <= len) { /* only space for ".." */ + buf[i++] = '.'; + buf[i++] = '.'; + } else if (i + 2 <= len) /* only space for a single "." */ + buf[i++] = '.'; + else + assert(i + 1 <= len); + done: buf[i] = '\0'; return buf; diff --git a/src/test/test-string-util.c b/src/test/test-string-util.c index 413adfda7d..8b176781de 100644 --- a/src/test/test-string-util.c +++ b/src/test/test-string-util.c @@ -81,21 +81,79 @@ static void test_ascii_strcasecmp_nn(void) { static void test_cellescape(void) { char buf[40]; - assert_se(streq(cellescape(buf, 10, "1"), "1")); - assert_se(streq(cellescape(buf, 10, "12"), "12")); - assert_se(streq(cellescape(buf, 10, "123"), is_locale_utf8() ? "1…" : "1...")); + assert_se(streq(cellescape(buf, 1, ""), "")); + assert_se(streq(cellescape(buf, 1, "1"), "")); + assert_se(streq(cellescape(buf, 1, "12"), "")); - assert_se(streq(cellescape(buf, 10, "1\011"), "1\\t")); - assert_se(streq(cellescape(buf, 10, "1\020"), "1\\020")); - assert_se(streq(cellescape(buf, 10, "1\020x"), is_locale_utf8() ? "1…" : "1...")); + assert_se(streq(cellescape(buf, 2, ""), "")); + assert_se(streq(cellescape(buf, 2, "1"), "1")); + assert_se(streq(cellescape(buf, 2, "12"), ".")); + assert_se(streq(cellescape(buf, 2, "123"), ".")); + + assert_se(streq(cellescape(buf, 3, ""), "")); + assert_se(streq(cellescape(buf, 3, "1"), "1")); + assert_se(streq(cellescape(buf, 3, "12"), "12")); + assert_se(streq(cellescape(buf, 3, "123"), "..")); + assert_se(streq(cellescape(buf, 3, "1234"), "..")); + + assert_se(streq(cellescape(buf, 4, ""), "")); + assert_se(streq(cellescape(buf, 4, "1"), "1")); + assert_se(streq(cellescape(buf, 4, "12"), "12")); + assert_se(streq(cellescape(buf, 4, "123"), "123")); + assert_se(streq(cellescape(buf, 4, "1234"), is_locale_utf8() ? "…" : "...")); + assert_se(streq(cellescape(buf, 4, "12345"), is_locale_utf8() ? "…" : "...")); + + assert_se(streq(cellescape(buf, 5, ""), "")); + assert_se(streq(cellescape(buf, 5, "1"), "1")); + assert_se(streq(cellescape(buf, 5, "12"), "12")); + assert_se(streq(cellescape(buf, 5, "123"), "123")); + assert_se(streq(cellescape(buf, 5, "1234"), "1234")); + assert_se(streq(cellescape(buf, 5, "12345"), is_locale_utf8() ? "1…" : "1...")); + assert_se(streq(cellescape(buf, 5, "123456"), is_locale_utf8() ? "1…" : "1...")); + + assert_se(streq(cellescape(buf, 1, "\020"), "")); + assert_se(streq(cellescape(buf, 2, "\020"), ".")); + assert_se(streq(cellescape(buf, 3, "\020"), "..")); + assert_se(streq(cellescape(buf, 4, "\020"), "…")); + assert_se(streq(cellescape(buf, 5, "\020"), "\\020")); + + assert_se(streq(cellescape(buf, 5, "1234\020"), "1…")); + assert_se(streq(cellescape(buf, 6, "1234\020"), "12…")); + assert_se(streq(cellescape(buf, 7, "1234\020"), "123…")); + assert_se(streq(cellescape(buf, 8, "1234\020"), "1234…")); + assert_se(streq(cellescape(buf, 9, "1234\020"), "1234\\020")); + + assert_se(streq(cellescape(buf, 1, "\t\n"), "")); + assert_se(streq(cellescape(buf, 2, "\t\n"), ".")); + assert_se(streq(cellescape(buf, 3, "\t\n"), "..")); + assert_se(streq(cellescape(buf, 4, "\t\n"), "…")); + assert_se(streq(cellescape(buf, 5, "\t\n"), "\\t\\n")); + + assert_se(streq(cellescape(buf, 5, "1234\t\n"), "1…")); + assert_se(streq(cellescape(buf, 6, "1234\t\n"), "12…")); + assert_se(streq(cellescape(buf, 7, "1234\t\n"), "123…")); + assert_se(streq(cellescape(buf, 8, "1234\t\n"), "1234…")); + assert_se(streq(cellescape(buf, 9, "1234\t\n"), "1234\\t\\n")); + + assert_se(streq(cellescape(buf, 4, "x\t\020\n"), "…")); + assert_se(streq(cellescape(buf, 5, "x\t\020\n"), "x…")); + assert_se(streq(cellescape(buf, 6, "x\t\020\n"), "x…")); + assert_se(streq(cellescape(buf, 7, "x\t\020\n"), "x\\t…")); + assert_se(streq(cellescape(buf, 8, "x\t\020\n"), "x\\t…")); + assert_se(streq(cellescape(buf, 9, "x\t\020\n"), "x\\t…")); + assert_se(streq(cellescape(buf, 10, "x\t\020\n"), "x\\t\\020\\n")); + + assert_se(streq(cellescape(buf, 6, "1\011"), "1\\t")); + assert_se(streq(cellescape(buf, 6, "1\020"), "1\\020")); + assert_se(streq(cellescape(buf, 6, "1\020x"), is_locale_utf8() ? "1…" : "1...")); assert_se(streq(cellescape(buf, 40, "1\020"), "1\\020")); assert_se(streq(cellescape(buf, 40, "1\020x"), "1\\020x")); assert_se(streq(cellescape(buf, 40, "\a\b\f\n\r\t\v\\\"'"), "\\a\\b\\f\\n\\r\\t\\v\\\\\\\"\\'")); - assert_se(streq(cellescape(buf, 10, "\a\b\f\n\r\t\v\\\"'"), is_locale_utf8() ? "\\a…" : "\\a...")); - assert_se(streq(cellescape(buf, 11, "\a\b\f\n\r\t\v\\\"'"), is_locale_utf8() ? "\\a…" : "\\a...")); - assert_se(streq(cellescape(buf, 12, "\a\b\f\n\r\t\v\\\"'"), is_locale_utf8() ? "\\a\\b…" : "\\a\\b...")); + assert_se(streq(cellescape(buf, 6, "\a\b\f\n\r\t\v\\\"'"), is_locale_utf8() ? "\\a…" : "\\a...")); + assert_se(streq(cellescape(buf, 7, "\a\b\f\n\r\t\v\\\"'"), is_locale_utf8() ? "\\a…" : "\\a...")); + assert_se(streq(cellescape(buf, 8, "\a\b\f\n\r\t\v\\\"'"), is_locale_utf8() ? "\\a\\b…" : "\\a\\b...")); assert_se(streq(cellescape(buf, sizeof buf, "1\020"), "1\\020")); assert_se(streq(cellescape(buf, sizeof buf, "1\020x"), "1\\020x")); From ce268825539ebb94ed9755b9e78480abd863ece1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 16 May 2018 21:50:35 -0400 Subject: [PATCH 5/5] process-util: also filter non-printable characters in get_process_com() We already do that in get_process_cmdline(), which is very similar in behaviour otherwise. Hence, let's be safe and also filter them in get_process_comm(). Let's try to retain as much information as we can though and escape rather than suppress unprintable characters. Let's not increase comm names beyond the kernel limit on such names however. Also see discussion about this here: https://marc.info/?l=linux-api&m=152649570404881&w=2 --- src/basic/process-util.c | 19 +++++++++++++++---- src/test/test-process-util.c | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/src/basic/process-util.c b/src/basic/process-util.c index 058f077de0..e3be7b9793 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -76,20 +76,31 @@ int get_process_state(pid_t pid) { return (unsigned char) state; } -int get_process_comm(pid_t pid, char **name) { +int get_process_comm(pid_t pid, char **ret) { + _cleanup_free_ char *escaped = NULL, *comm = NULL; const char *p; int r; - assert(name); + assert(ret); assert(pid >= 0); + escaped = new(char, TASK_COMM_LEN); + if (!escaped) + return -ENOMEM; + p = procfs_file_alloca(pid, "comm"); - r = read_one_line_file(p, name); + r = read_one_line_file(p, &comm); if (r == -ENOENT) return -ESRCH; + if (r < 0) + return r; - return r; + /* Escape unprintable characters, just in case, but don't grow the string beyond the underlying size */ + cellescape(escaped, TASK_COMM_LEN, comm); + + *ret = TAKE_PTR(escaped); + return 0; } int get_process_cmdline(pid_t pid, size_t max_length, bool comm_fallback, char **line) { diff --git a/src/test/test-process-util.c b/src/test/test-process-util.c index 8b975ff988..ef18cff70d 100644 --- a/src/test/test-process-util.c +++ b/src/test/test-process-util.c @@ -91,6 +91,38 @@ static void test_get_process_comm(pid_t pid) { log_info("PID"PID_FMT" $PATH: '%s'", pid, strna(i)); } +static void test_get_process_comm_escape_one(const char *input, const char *output) { + _cleanup_free_ char *n = NULL; + + log_info("input: <%s> — output: <%s>", input, output); + + assert_se(prctl(PR_SET_NAME, input) >= 0); + assert_se(get_process_comm(0, &n) >= 0); + + log_info("got: <%s>", n); + + assert_se(streq_ptr(n, output)); +} + +static void test_get_process_comm_escape(void) { + _cleanup_free_ char *saved = NULL; + + assert_se(get_process_comm(0, &saved) >= 0); + + test_get_process_comm_escape_one("", ""); + test_get_process_comm_escape_one("foo", "foo"); + test_get_process_comm_escape_one("012345678901234", "012345678901234"); + test_get_process_comm_escape_one("0123456789012345", "012345678901234"); + test_get_process_comm_escape_one("äöüß", "\\303\\244\\303…"); + test_get_process_comm_escape_one("xäöüß", "x\\303\\244…"); + test_get_process_comm_escape_one("xxäöüß", "xx\\303\\244…"); + test_get_process_comm_escape_one("xxxäöüß", "xxx\\303\\244…"); + test_get_process_comm_escape_one("xxxxäöüß", "xxxx\\303\\244…"); + test_get_process_comm_escape_one("xxxxxäöüß", "xxxxx\\303…"); + + assert_se(prctl(PR_SET_NAME, saved) >= 0); +} + static void test_pid_is_unwaited(void) { pid_t pid; @@ -572,6 +604,7 @@ int main(int argc, char *argv[]) { test_get_process_comm(getpid()); } + test_get_process_comm_escape(); test_pid_is_unwaited(); test_pid_is_alive(); test_personality();