From 8ca94009f826315002d62364efe0c880a13ba8c5 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 8 Mar 2021 21:44:39 +0100 Subject: [PATCH 1/7] basic: tighten two filename length checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes two checks where we compare string sizes when validating with FILENAME_MAX. In both cases the check apparently wants to check if the name fits in a filename, but that's not actually what FILENAME_MAX can be used for, as it — in contrast to what the name suggests — actually encodes the maximum length of a path. In both cases the stricter change doesn't actually change much, but the use of FILENAME_MAX is still misleading and typically wrong. --- src/basic/cgroup-util.c | 2 +- src/basic/user-util.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c index 0ecb8d944e..7372b06024 100644 --- a/src/basic/cgroup-util.c +++ b/src/basic/cgroup-util.c @@ -1567,7 +1567,7 @@ bool cg_controller_is_valid(const char *p) { if (!strchr(CONTROLLER_VALID, *t)) return false; - if (t - p > FILENAME_MAX) + if (t - p > NAME_MAX) return false; return true; diff --git a/src/basic/user-util.c b/src/basic/user-util.c index 1fad342cef..0ea4e409fa 100644 --- a/src/basic/user-util.c +++ b/src/basic/user-util.c @@ -836,7 +836,7 @@ bool valid_user_group_name(const char *u, ValidUserFlags flags) { if (l > (size_t) sz) return false; - if (l > FILENAME_MAX) + if (l > NAME_MAX) /* must fit in a filename */ return false; if (l > UT_NAMESIZE - 1) return false; From f470d234d3c5af97e846307b15dc6b298c75380b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 8 Mar 2021 22:27:05 +0100 Subject: [PATCH 2/7] efi-loader: make efi_loader_entry_name_valid() check a bit stricter Previously we'd just check if the ID was no-empty an no longer than FILENAME_MAX. The latter was probably a mistake, given the comment next to it. Instead of fixing that to check for NAME_MAX let's instead just switch over to filename_is_valid() which odes a similar check, plus a some minor additional checks. After all we do want that valid EFI boot menu entry ids are usable as filenames. --- src/shared/efi-loader.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/shared/efi-loader.c b/src/shared/efi-loader.c index 2aab39c32d..ee32b0b097 100644 --- a/src/shared/efi-loader.c +++ b/src/shared/efi-loader.c @@ -809,10 +809,8 @@ bool efi_has_tpm2(void) { #endif bool efi_loader_entry_name_valid(const char *s) { - if (isempty(s)) - return false; - if (strlen(s) > FILENAME_MAX) /* Make sure entry names fit in filenames */ + if (!filename_is_valid(s)) /* Make sure entry names fit in filenames */ return false; return in_charset(s, ALPHANUMERICAL "+-_."); From b775b1828df8d3ee53e8706cc8ca3d0b8726a621 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 8 Mar 2021 22:43:07 +0100 Subject: [PATCH 3/7] docs: document not to use FILENAME_MAX in our codebase It's a weird thing. Let's explain why. --- docs/CODING_STYLE.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/CODING_STYLE.md b/docs/CODING_STYLE.md index 851676bc2b..c15dc1abf7 100644 --- a/docs/CODING_STYLE.md +++ b/docs/CODING_STYLE.md @@ -587,6 +587,12 @@ layout: default time you need that please immediately undefine `basename()`, and add a comment about it, so that no code ever ends up using the POSIX version! +- Never use FILENAME_MAX. Use PATH_MAX instead (for checking maximum size of + paths) and NAME_MAX (for checking maximum size of filenames). FILENAME_MAX is + not POSIX, and is a confusingly named alias for PATH_MAX on Linux. Note the + NAME_MAX does not include space for a trailing NUL, but PATH_MAX does. UNIX + FTW! + ## Committing to git - Commit message subject lines should be prefixed with an appropriate component From 932401fd6149b87352c27a11287eefe979c30e9c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 8 Mar 2021 22:43:41 +0100 Subject: [PATCH 4/7] docs: reference NAME_MAX where we talk about filenames --- docs/USER_NAMES.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/USER_NAMES.md b/docs/USER_NAMES.md index ec07b19f30..daafdf2dce 100644 --- a/docs/USER_NAMES.md +++ b/docs/USER_NAMES.md @@ -87,8 +87,8 @@ hyphen. A size limit is enforced: the minimum of `sysconf(_SC_LOGIN_NAME_MAX)` (typically 256 on Linux; rationale: this is how POSIX suggests to detect the limit), `UT_NAMESIZE-1` (typically 31 on Linux; rationale: names longer than this cannot correctly appear in `utmp`/`wtmp` and create ambiguity with login -accounting) and `FILENAME_MAX` (4096 on Linux; rationale: user names typically -appear in directory names, i.e. the home directory), thus MIN(256, 31, 4096) = +accounting) and `NAME_MAX` (255 on Linux; rationale: user names typically +appear in directory names, i.e. the home directory), thus MIN(256, 31, 255) = 31. Note that these rules are both more strict and more relaxed than all of the From db220032334c10ff4f8b10bc4c6c86530bcc4492 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 8 Mar 2021 22:43:50 +0100 Subject: [PATCH 5/7] fs-util: replace use of FILENAME_MAX by PATH_MAX in readlinkat_malloc() While we are at it, let's also add an overflow check and do other modernizations. --- src/basic/fs-util.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index 539b0a4886..cc219d297b 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -135,34 +135,34 @@ int rename_noreplace(int olddirfd, const char *oldpath, int newdirfd, const char } int readlinkat_malloc(int fd, const char *p, char **ret) { - size_t l = FILENAME_MAX+1; - int r; + size_t l = PATH_MAX; assert(p); assert(ret); for (;;) { - char *c; + _cleanup_free_ char *c = NULL; ssize_t n; - c = new(char, l); + c = new(char, l+1); if (!c) return -ENOMEM; - n = readlinkat(fd, p, c, l-1); - if (n < 0) { - r = -errno; - free(c); - return r; - } + n = readlinkat(fd, p, c, l); + if (n < 0) + return -errno; - if ((size_t) n < l-1) { + if ((size_t) n < l) { c[n] = 0; - *ret = c; + *ret = TAKE_PTR(c); return 0; } - free(c); + if (l > (SSIZE_MAX-1)/2) /* readlinkat() returns an ssize_t, and we want an extra byte for a + * trailing NUL, hence do an overflow check relative to SSIZE_MAX-1 + * here */ + return -EFBIG; + l *= 2; } } From 445714569d986100bf67d34ba62824391522b456 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 8 Mar 2021 22:44:35 +0100 Subject: [PATCH 6/7] mountpoint-util: replace our last use of FILENAME_MAX by PATH_MAX --- src/basic/mountpoint-util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/basic/mountpoint-util.c b/src/basic/mountpoint-util.c index ed7457f8b7..adf10a4548 100644 --- a/src/basic/mountpoint-util.c +++ b/src/basic/mountpoint-util.c @@ -148,7 +148,7 @@ static bool filename_possibly_with_slash_suffix(const char *s) { if (!slash) return filename_is_valid(s); - if (slash - s > FILENAME_MAX) /* We want to allocate on the stack below, hence do a size check first */ + if (slash - s > PATH_MAX) /* We want to allocate on the stack below, hence do a size check first */ return false; if (slash[strspn(slash, "/")] != 0) /* Check that the suffix consist only of one or more slashes */ From 698660620d37395f8cf579be42133ae47ab76b1f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 8 Mar 2021 22:45:03 +0100 Subject: [PATCH 7/7] test: output FILENAME_MAX vs. PATH_MAX sizes Also, make sure our assumption that FILENAME_MAX == PATH_MAX holds. --- src/test/test-path-util.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/test/test-path-util.c b/src/test/test-path-util.c index b92a77b9f4..b49b0ae908 100644 --- a/src/test/test-path-util.c +++ b/src/test/test-path-util.c @@ -838,6 +838,15 @@ static void test_path_startswith_strv(void) { int main(int argc, char **argv) { test_setup_logging(LOG_DEBUG); + log_info("PATH_MAX=%zu\n" + "FILENAME_MAX=%zu\n" + "NAME_MAX=%zu", + (size_t) PATH_MAX, + (size_t) FILENAME_MAX, + (size_t) NAME_MAX); + + assert_cc(FILENAME_MAX == PATH_MAX); + test_print_paths(); test_path(); test_path_equal_root();