From b9d06522631a22d242374dc44a74c3b6459e3cb3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 21 Oct 2021 18:07:06 +0200 Subject: [PATCH 1/7] stat-util: specify O_DIRECTORY when reopening dir in dir_is_empty_at() That way we can fail earlier if the specified fd is not actually a directory. (Also, it's not exactly according to standards to open things without either O_RDONLY/O_RDWR...) --- src/basic/stat-util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/basic/stat-util.c b/src/basic/stat-util.c index 0004255bdb..56789b4878 100644 --- a/src/basic/stat-util.c +++ b/src/basic/stat-util.c @@ -83,7 +83,7 @@ int dir_is_empty_at(int dir_fd, const char *path) { } else { /* Note that DUPing is not enough, as the internal pointer * would still be shared and moved by FOREACH_DIRENT. */ - fd = fd_reopen(dir_fd, O_CLOEXEC); + fd = fd_reopen(dir_fd, O_RDONLY|O_DIRECTORY|O_CLOEXEC); if (fd < 0) return fd; } From ca664db25860d7fcd581380e24ad74f510c1f40f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 23 Oct 2021 00:21:20 +0200 Subject: [PATCH 2/7] dirent-util: move getdents64() related definitions to common header We want to reuse getdents64() wherever necessary, let's hence move definitions useful for that into public code. --- src/basic/dirent-util.h | 18 ++++++++++++++++++ src/basic/recurse-dir.c | 24 +++--------------------- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/src/basic/dirent-util.h b/src/basic/dirent-util.h index c7956e7c1b..73e78102a5 100644 --- a/src/basic/dirent-util.h +++ b/src/basic/dirent-util.h @@ -33,3 +33,21 @@ struct dirent *readdir_no_dot(DIR *dirp); } \ break; \ } else + +/* Maximum space one dirent structure might require at most */ +#define DIRENT_SIZE_MAX CONST_MAX(sizeof(struct dirent), offsetof(struct dirent, d_name) + NAME_MAX + 1) + +/* Only if 64bit off_t is enabled struct dirent + struct dirent64 are actually the same. We require this, and + * we want them to be interchangeable to make getdents64() work, hence verify that. */ +assert_cc(_FILE_OFFSET_BITS == 64); +assert_cc(sizeof(struct dirent) == sizeof(struct dirent64)); +assert_cc(offsetof(struct dirent, d_ino) == offsetof(struct dirent64, d_ino)); +assert_cc(sizeof_field(struct dirent, d_ino) == sizeof_field(struct dirent64, d_ino)); +assert_cc(offsetof(struct dirent, d_off) == offsetof(struct dirent64, d_off)); +assert_cc(sizeof_field(struct dirent, d_off) == sizeof_field(struct dirent64, d_off)); +assert_cc(offsetof(struct dirent, d_reclen) == offsetof(struct dirent64, d_reclen)); +assert_cc(sizeof_field(struct dirent, d_reclen) == sizeof_field(struct dirent64, d_reclen)); +assert_cc(offsetof(struct dirent, d_type) == offsetof(struct dirent64, d_type)); +assert_cc(sizeof_field(struct dirent, d_type) == sizeof_field(struct dirent64, d_type)); +assert_cc(offsetof(struct dirent, d_name) == offsetof(struct dirent64, d_name)); +assert_cc(sizeof_field(struct dirent, d_name) == sizeof_field(struct dirent64, d_name)); diff --git a/src/basic/recurse-dir.c b/src/basic/recurse-dir.c index 86018c8d29..adc855955a 100644 --- a/src/basic/recurse-dir.c +++ b/src/basic/recurse-dir.c @@ -25,9 +25,6 @@ static bool ignore_dirent(const struct dirent *de, RecurseDirFlags flags) { dot_or_dot_dot(de->d_name); } -/* Maximum space one direent structure might require at most */ -#define DIRENT_SIZE_MAX MAX(sizeof(struct dirent), offsetof(struct dirent, d_name) + NAME_MAX + 1) - int readdir_all(int dir_fd, RecurseDirFlags flags, DirectoryEntries **ret) { @@ -39,24 +36,9 @@ int readdir_all(int dir_fd, assert(dir_fd >= 0); /* Returns an array with pointers to "struct dirent" directory entries, optionally sorted. Free the - * array with readdir_all_freep(). */ - - /* Only if 64bit off_t is enabled struct dirent + struct dirent64 are actually the same. We require - * this, and we want them to be interchangeable, hence verify that. */ - assert_cc(_FILE_OFFSET_BITS == 64); - assert_cc(sizeof(struct dirent) == sizeof(struct dirent64)); - assert_cc(offsetof(struct dirent, d_ino) == offsetof(struct dirent64, d_ino)); - assert_cc(sizeof(((struct dirent*) NULL)->d_ino) == sizeof(((struct dirent64*) NULL)->d_ino)); - assert_cc(offsetof(struct dirent, d_off) == offsetof(struct dirent64, d_off)); - assert_cc(sizeof(((struct dirent*) NULL)->d_off) == sizeof(((struct dirent64*) NULL)->d_off)); - assert_cc(offsetof(struct dirent, d_reclen) == offsetof(struct dirent64, d_reclen)); - assert_cc(sizeof(((struct dirent*) NULL)->d_reclen) == sizeof(((struct dirent64*) NULL)->d_reclen)); - assert_cc(offsetof(struct dirent, d_type) == offsetof(struct dirent64, d_type)); - assert_cc(sizeof(((struct dirent*) NULL)->d_type) == sizeof(((struct dirent64*) NULL)->d_type)); - assert_cc(offsetof(struct dirent, d_name) == offsetof(struct dirent64, d_name)); - assert_cc(sizeof(((struct dirent*) NULL)->d_name) == sizeof(((struct dirent64*) NULL)->d_name)); - - /* Start with space for up to 8 directory entries. We expect at least 2 ("." + ".."), hence hopefully + * array with readdir_all_freep(). + * + * Start with space for up to 8 directory entries. We expect at least 2 ("." + ".."), hence hopefully * 8 will cover most cases comprehensively. (Note that most likely a lot more entries will actually * fit in the buffer, given we calculate maximum file name length here.) */ de = malloc(offsetof(DirectoryEntries, buffer) + DIRENT_SIZE_MAX * 8); From a4e70ef7ba3b5e2c050cce5ed2bbf58807505596 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 23 Oct 2021 00:28:24 +0200 Subject: [PATCH 3/7] dirent-util: add FOREACH macro for iterating through getdents64() buffers We already have a similar loop twice, let's make it easier to read via an iteration macro. (The new macro is a bit more careful even, as it verifies the full dirent fits into the remaining buffer when returning it) --- src/basic/dirent-util.h | 5 +++++ src/basic/recurse-dir.c | 13 ++++--------- src/basic/recurse-dir.h | 2 +- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/basic/dirent-util.h b/src/basic/dirent-util.h index 73e78102a5..768cc1de61 100644 --- a/src/basic/dirent-util.h +++ b/src/basic/dirent-util.h @@ -51,3 +51,8 @@ assert_cc(offsetof(struct dirent, d_type) == offsetof(struct dirent64, d_type)); assert_cc(sizeof_field(struct dirent, d_type) == sizeof_field(struct dirent64, d_type)); assert_cc(offsetof(struct dirent, d_name) == offsetof(struct dirent64, d_name)); assert_cc(sizeof_field(struct dirent, d_name) == sizeof_field(struct dirent64, d_name)); + +#define FOREACH_DIRENT_IN_BUFFER(de, buf, sz) \ + for (void *_end = (uint8_t*) ({ (de) = (buf); }) + (sz); \ + (uint8_t*) (de) < (uint8_t*) _end; \ + (de) = (struct dirent*) ((uint8_t*) (de) + (de)->d_reclen)) diff --git a/src/basic/recurse-dir.c b/src/basic/recurse-dir.c index adc855955a..dbada82431 100644 --- a/src/basic/recurse-dir.c +++ b/src/basic/recurse-dir.c @@ -30,6 +30,7 @@ int readdir_all(int dir_fd, DirectoryEntries **ret) { _cleanup_free_ DirectoryEntries *de = NULL; + struct dirent *entry; DirectoryEntries *nde; size_t add, sz, j; @@ -53,7 +54,7 @@ int readdir_all(int dir_fd, bs = MIN(MALLOC_SIZEOF_SAFE(de) - offsetof(DirectoryEntries, buffer), (size_t) SSIZE_MAX); assert(bs > de->buffer_size); - n = getdents64(dir_fd, de->buffer + de->buffer_size, bs - de->buffer_size); + n = getdents64(dir_fd, (uint8_t*) de->buffer + de->buffer_size, bs - de->buffer_size); if (n < 0) return -errno; if (n == 0) @@ -77,10 +78,7 @@ int readdir_all(int dir_fd, } de->n_entries = 0; - for (struct dirent *entry = (struct dirent*) de->buffer; - (uint8_t*) entry < de->buffer + de->buffer_size; - entry = (struct dirent*) ((uint8_t*) entry + entry->d_reclen)) { - + FOREACH_DIRENT_IN_BUFFER(entry, de->buffer, de->buffer_size) { if (ignore_dirent(entry, flags)) continue; @@ -100,10 +98,7 @@ int readdir_all(int dir_fd, de->entries = (struct dirent**) ((uint8_t*) de + ALIGN(offsetof(DirectoryEntries, buffer) + de->buffer_size)); j = 0; - for (struct dirent *entry = (struct dirent*) de->buffer; - (uint8_t*) entry < de->buffer + de->buffer_size; - entry = (struct dirent*) ((uint8_t*) entry + entry->d_reclen)) { - + FOREACH_DIRENT_IN_BUFFER(entry, de->buffer, de->buffer_size) { if (ignore_dirent(entry, flags)) continue; diff --git a/src/basic/recurse-dir.h b/src/basic/recurse-dir.h index 93b00f0d97..779c91e905 100644 --- a/src/basic/recurse-dir.h +++ b/src/basic/recurse-dir.h @@ -71,7 +71,7 @@ typedef struct DirectoryEntries { size_t n_entries; struct dirent** entries; size_t buffer_size; - uint8_t buffer[] _alignas_(struct dirent); + struct dirent buffer[]; } DirectoryEntries; int readdir_all(int dir_fd, RecurseDirFlags flags, DirectoryEntries **ret); From a068aceafbffcba85398cce636c25d659265087a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 23 Oct 2021 00:30:14 +0200 Subject: [PATCH 4/7] stat-util: optimize dir_is_empty_at() a bit, by using getdents64() That way we have a single syscall only for it, instead of the multiple readdir() and friends do. And we can operate entirely on the stack, no malloc() implicit. --- src/basic/dirent-util.h | 6 ++++++ src/basic/stat-util.c | 20 +++++++++++++------- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/src/basic/dirent-util.h b/src/basic/dirent-util.h index 768cc1de61..a6272f891f 100644 --- a/src/basic/dirent-util.h +++ b/src/basic/dirent-util.h @@ -56,3 +56,9 @@ assert_cc(sizeof_field(struct dirent, d_name) == sizeof_field(struct dirent64, d for (void *_end = (uint8_t*) ({ (de) = (buf); }) + (sz); \ (uint8_t*) (de) < (uint8_t*) _end; \ (de) = (struct dirent*) ((uint8_t*) (de) + (de)->d_reclen)) + +#define DEFINE_DIRENT_BUFFER(name, sz) \ + union { \ + struct dirent de; \ + uint8_t data[(sz) * DIRENT_SIZE_MAX]; \ + } name diff --git a/src/basic/stat-util.c b/src/basic/stat-util.c index 56789b4878..900f06ad1c 100644 --- a/src/basic/stat-util.c +++ b/src/basic/stat-util.c @@ -73,27 +73,33 @@ int is_device_node(const char *path) { int dir_is_empty_at(int dir_fd, const char *path) { _cleanup_close_ int fd = -1; - _cleanup_closedir_ DIR *d = NULL; + /* Allocate space for at least 3 full dirents, since every dir has at least two entries ("." + + * ".."), and only once we have seen if there's a third we know whether the dir is empty or not. */ + DEFINE_DIRENT_BUFFER(buffer, 3); struct dirent *de; + ssize_t n; if (path) { fd = openat(dir_fd, path, O_RDONLY|O_DIRECTORY|O_CLOEXEC); if (fd < 0) return -errno; } else { - /* Note that DUPing is not enough, as the internal pointer - * would still be shared and moved by FOREACH_DIRENT. */ + /* Note that DUPing is not enough, as the internal pointer would still be shared and moved + * getedents64(). */ + assert(dir_fd >= 0); + fd = fd_reopen(dir_fd, O_RDONLY|O_DIRECTORY|O_CLOEXEC); if (fd < 0) return fd; } - d = take_fdopendir(&fd); - if (!d) + n = getdents64(fd, &buffer, sizeof(buffer)); + if (n < 0) return -errno; - FOREACH_DIRENT(de, d, return -errno) - return 0; + FOREACH_DIRENT_IN_BUFFER(de, &buffer.de, n) + if (!dot_or_dot_dot(de->d_name)) + return 0; return 1; } From d96f21eec2c0d6cce25ca6782f76f053b2ed5cc9 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 23 Oct 2021 00:31:33 +0200 Subject: [PATCH 5/7] stat-util: make sure dir_is_empty_at() does something useful in all cases --- src/basic/stat-util.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/basic/stat-util.c b/src/basic/stat-util.c index 900f06ad1c..77e728bcab 100644 --- a/src/basic/stat-util.c +++ b/src/basic/stat-util.c @@ -80,9 +80,15 @@ int dir_is_empty_at(int dir_fd, const char *path) { ssize_t n; if (path) { + assert(dir_fd >= 0 || dir_fd == AT_FDCWD); + fd = openat(dir_fd, path, O_RDONLY|O_DIRECTORY|O_CLOEXEC); if (fd < 0) return -errno; + } else if (dir_fd == AT_FDCWD) { + fd = open(".", O_RDONLY|O_DIRECTORY|O_CLOEXEC); + if (fd < 0) + return -errno; } else { /* Note that DUPing is not enough, as the internal pointer would still be shared and moved * getedents64(). */ From bfc569b060bfe88b1ee03aeed0a04ad35e6e8a93 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 23 Oct 2021 00:32:59 +0200 Subject: [PATCH 6/7] test: add test for dir_is_empty_at() --- src/test/test-stat-util.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/test/test-stat-util.c b/src/test/test-stat-util.c index b27d26c81a..da90a98823 100644 --- a/src/test/test-stat-util.c +++ b/src/test/test-stat-util.c @@ -8,10 +8,12 @@ #include "alloc-util.h" #include "errno-list.h" #include "fd-util.h" +#include "fs-util.h" #include "macro.h" #include "mountpoint-util.h" #include "namespace-util.h" #include "path-util.h" +#include "rm-rf.h" #include "stat-util.h" #include "tests.h" #include "tmpfile-util.h" @@ -223,6 +225,35 @@ static void test_device_path_make_canonical(void) { } } +static void test_dir_is_empty(void) { + _cleanup_(rm_rf_physical_and_freep) char *empty_dir = NULL; + _cleanup_free_ char *j = NULL, *jj = NULL; + + log_info("/* %s */", __func__); + + assert_se(dir_is_empty_at(AT_FDCWD, "/proc") == 0); + assert_se(dir_is_empty_at(AT_FDCWD, "/icertainlydontexistdoi") == -ENOENT); + + assert_se(mkdtemp_malloc("/tmp/emptyXXXXXX", &empty_dir) >= 0); + assert_se(dir_is_empty_at(AT_FDCWD, empty_dir) > 0); + + j = path_join(empty_dir, "zzz"); + assert_se(j); + assert_se(touch(j) >= 0); + + assert_se(dir_is_empty_at(AT_FDCWD, empty_dir) == 0); + + jj = path_join(empty_dir, "ppp"); + assert_se(jj); + assert_se(touch(jj) >= 0); + + assert_se(dir_is_empty_at(AT_FDCWD, empty_dir) == 0); + assert_se(unlink(j) >= 0); + assert_se(dir_is_empty_at(AT_FDCWD, empty_dir) == 0); + assert_se(unlink(jj) >= 0); + assert_se(dir_is_empty_at(AT_FDCWD, empty_dir) > 0); +} + int main(int argc, char *argv[]) { log_show_color(true); test_setup_logging(LOG_INFO); @@ -235,6 +266,7 @@ int main(int argc, char *argv[]) { test_fd_is_ns(); test_device_major_minor_valid(); test_device_path_make_canonical(); + test_dir_is_empty(); return 0; } From 0dbce03c37d1e11837dd7f9b80b9964ca539c914 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 25 Oct 2021 10:59:56 +0200 Subject: [PATCH 7/7] tree-wide: explicitly unpoison getdents64() memory Apparently memory sanitizer doesn't grok getdents64() properly. Let's address that by explicitly marken memory initialized by getdents64() as unpoisoned. --- src/basic/recurse-dir.c | 2 ++ src/basic/stat-util.c | 2 ++ 2 files changed, 4 insertions(+) diff --git a/src/basic/recurse-dir.c b/src/basic/recurse-dir.c index dbada82431..d1fee91c32 100644 --- a/src/basic/recurse-dir.c +++ b/src/basic/recurse-dir.c @@ -60,6 +60,8 @@ int readdir_all(int dir_fd, if (n == 0) break; + msan_unpoison((uint8_t*) de->buffer + de->buffer_size, n); + de->buffer_size += n; if (de->buffer_size < bs - DIRENT_SIZE_MAX) /* Still room for one more entry, then try to diff --git a/src/basic/stat-util.c b/src/basic/stat-util.c index 77e728bcab..efac7b002e 100644 --- a/src/basic/stat-util.c +++ b/src/basic/stat-util.c @@ -103,6 +103,8 @@ int dir_is_empty_at(int dir_fd, const char *path) { if (n < 0) return -errno; + msan_unpoison(&buffer, n); + FOREACH_DIRENT_IN_BUFFER(de, &buffer.de, n) if (!dot_or_dot_dot(de->d_name)) return 0;