From 1b0c39458bab864f0279195c19513f15543cf1ee Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 22 Jun 2025 11:09:11 +0900 Subject: [PATCH 1/4] test-glob-util: drop redundant test cases The removed glob() with GLOB_ALTDIRFUNC is exactly the same as what safe_glob() does, which is tested in TEST(safe_glob) below. Let's drop the test cases. --- src/test/test-glob-util.c | 31 ------------------------------- 1 file changed, 31 deletions(-) diff --git a/src/test/test-glob-util.c b/src/test/test-glob-util.c index 7e1a264cb9..754dc77447 100644 --- a/src/test/test-glob-util.c +++ b/src/test/test-glob-util.c @@ -51,37 +51,6 @@ TEST(glob_exists) { assert_se(r == 0); } -static void closedir_wrapper(void* v) { - (void) closedir(v); -} - -TEST(glob_no_dot) { - char template[] = "/tmp/test-glob-util.XXXXXXX"; - const char *fn; - - _cleanup_globfree_ glob_t g = { - .gl_closedir = closedir_wrapper, - .gl_readdir = (struct dirent *(*)(void *)) readdir_no_dot, - .gl_opendir = (void *(*)(const char *)) opendir, - .gl_lstat = lstat, - .gl_stat = stat, - }; - - int r; - - assert_se(mkdtemp(template)); - - fn = strjoina(template, "/*"); - r = glob(fn, GLOB_NOSORT|GLOB_BRACE|GLOB_ALTDIRFUNC, NULL, &g); - assert_se(r == GLOB_NOMATCH); - - fn = strjoina(template, "/.*"); - r = glob(fn, GLOB_NOSORT|GLOB_BRACE|GLOB_ALTDIRFUNC, NULL, &g); - assert_se(r == GLOB_NOMATCH); - - (void) rm_rf(template, REMOVE_ROOT|REMOVE_PHYSICAL); -} - TEST(safe_glob) { char template[] = "/tmp/test-glob-util.XXXXXXX"; const char *fn, *fn2, *fname; From 2a5f950e5643a74bef70b1c3c46ec33ad0e3fd41 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 23 Jun 2025 08:55:54 +0900 Subject: [PATCH 2/4] glob-util: rework safe_glob() Currently, callers of safe_glob() set an empty glob_t or glob_t with opendir func, and all other components are always zero. So, let's introduce safe_glob_full() which optionally takes opendir function, rather than glob_t, and returns result strv, rather than storing results in glob_t. Also, introduce safe_glob() which is a trivial wrapper of safe_glob_full() without opendir func. No functional change, just refactoring. --- src/basic/glob-util.c | 92 +++++++++++++++++++------------------- src/basic/glob-util.h | 10 +++-- src/core/exec-credential.c | 6 +-- src/core/execute.c | 8 ++-- src/test/test-glob-util.c | 27 +++++------ src/tmpfiles/tmpfiles.c | 16 +++---- 6 files changed, 78 insertions(+), 81 deletions(-) diff --git a/src/basic/glob-util.c b/src/basic/glob-util.c index 9eec8b3f09..5843ef088f 100644 --- a/src/basic/glob-util.c +++ b/src/basic/glob-util.c @@ -6,81 +6,83 @@ #include "dirent-util.h" #include "errno-util.h" #include "glob-util.h" -#include "log.h" #include "string-util.h" #include "strv.h" -static void closedir_wrapper(void* v) { - (void) closedir(v); -} +DEFINE_TRIVIAL_DESTRUCTOR(closedir_wrapper, void, closedir); -int safe_glob(const char *path, int flags, glob_t *pglob) { - int k; +int safe_glob_full(const char *path, int flags, opendir_t opendir_func, char ***ret) { + _cleanup_(globfree) glob_t g = { + .gl_closedir = closedir_wrapper, + .gl_readdir = (struct dirent* (*)(void *)) readdir_no_dot, + .gl_opendir = (void* (*)(const char *)) (opendir_func ?: opendir), + .gl_lstat = lstat, + .gl_stat = stat, + }; + int r; - /* We want to set GLOB_ALTDIRFUNC ourselves, don't allow it to be set. */ - assert(!(flags & GLOB_ALTDIRFUNC)); - - if (!pglob->gl_closedir) - pglob->gl_closedir = closedir_wrapper; - if (!pglob->gl_readdir) - pglob->gl_readdir = (struct dirent *(*)(void *)) readdir_no_dot; - if (!pglob->gl_opendir) - pglob->gl_opendir = (void *(*)(const char *)) opendir; - if (!pglob->gl_lstat) - pglob->gl_lstat = lstat; - if (!pglob->gl_stat) - pglob->gl_stat = stat; + assert(path); errno = 0; - k = glob(path, flags | GLOB_ALTDIRFUNC, NULL, pglob); - if (k == GLOB_NOMATCH) + r = glob(path, flags | GLOB_ALTDIRFUNC, NULL, &g); + if (r == GLOB_NOMATCH) return -ENOENT; - if (k == GLOB_NOSPACE) + if (r == GLOB_NOSPACE) return -ENOMEM; - if (k != 0) + if (r != 0) return errno_or_else(EIO); - if (strv_isempty(pglob->gl_pathv)) + + if (strv_isempty(g.gl_pathv)) return -ENOENT; + if (ret) { + *ret = g.gl_pathv; + TAKE_STRUCT(g); /* To avoid the result being freed. */ + } + return 0; } -int glob_first(const char *path, char **ret_first) { - _cleanup_globfree_ glob_t g = {}; - int k; +int glob_first(const char *path, char **ret) { + _cleanup_strv_free_ char **v = NULL; + int r; assert(path); - k = safe_glob(path, GLOB_NOSORT|GLOB_BRACE, &g); - if (k == -ENOENT) { - if (ret_first) - *ret_first = NULL; + r = safe_glob(path, GLOB_NOSORT|GLOB_BRACE, &v); + if (r == -ENOENT) { + if (ret) + *ret = NULL; return false; } - if (k < 0) - return k; + if (r < 0) + return r; - if (ret_first) { - assert(g.gl_pathv && g.gl_pathv[0]); + assert(!strv_isempty(v)); - char *first = strdup(g.gl_pathv[0]); - if (!first) - return log_oom_debug(); - *ret_first = first; + if (ret) { + /* Free all results except for the first one. */ + STRV_FOREACH(p, strv_skip(v, 1)) + *p = mfree(*p); + + /* Then, take the first result. */ + *ret = TAKE_PTR(*v); } return true; } int glob_extend(char ***strv, const char *path, int flags) { - _cleanup_globfree_ glob_t g = {}; - int k; + char **v; + int r; - k = safe_glob(path, GLOB_NOSORT|GLOB_BRACE|flags, &g); - if (k < 0) - return k; + assert(path); - return strv_extend_strv(strv, g.gl_pathv, false); + r = safe_glob(path, GLOB_NOSORT|GLOB_BRACE|flags, &v); + if (r < 0) + return r; + + return strv_extend_strv_consume(strv, v, /* filter_duplicates = */ false); } int glob_non_glob_prefix(const char *path, char **ret) { diff --git a/src/basic/glob-util.h b/src/basic/glob-util.h index 4fa23f5048..0e9088895f 100644 --- a/src/basic/glob-util.h +++ b/src/basic/glob-util.h @@ -5,11 +5,15 @@ #include "forward.h" -/* Note: this function modifies pglob to set various functions. */ -int safe_glob(const char *path, int flags, glob_t *pglob); +typedef DIR* (*opendir_t)(const char *); + +int safe_glob_full(const char *path, int flags, opendir_t opendir_func, char ***ret); +static inline int safe_glob(const char *path, int flags, char ***ret) { + return safe_glob_full(path, flags, NULL, ret); +} /* Note: which match is returned depends on the implementation/system and not guaranteed to be stable */ -int glob_first(const char *path, char **ret_first); +int glob_first(const char *path, char **ret); #define glob_exists(path) glob_first(path, NULL) int glob_extend(char ***strv, const char *path, int flags); diff --git a/src/core/exec-credential.c b/src/core/exec-credential.c index 9bc7540e5e..3077ddd92e 100644 --- a/src/core/exec-credential.c +++ b/src/core/exec-credential.c @@ -539,20 +539,20 @@ static int load_credential_glob( assert(search_path); STRV_FOREACH(d, search_path) { - _cleanup_globfree_ glob_t pglob = {}; + _cleanup_strv_free_ char **paths = NULL; _cleanup_free_ char *j = NULL; j = path_join(*d, ic->glob); if (!j) return -ENOMEM; - r = safe_glob(j, 0, &pglob); + r = safe_glob(j, /* flags = */ 0, &paths); if (r == -ENOENT) continue; if (r < 0) return r; - FOREACH_ARRAY(p, pglob.gl_pathv, pglob.gl_pathc) { + STRV_FOREACH(p, paths) { _cleanup_free_ char *fn = NULL; _cleanup_(erase_and_freep) char *data = NULL; size_t size; diff --git a/src/core/execute.c b/src/core/execute.c index 1fdb319042..e3c3f49887 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -911,7 +911,7 @@ static int exec_context_load_environment(const Unit *unit, const ExecContext *c, assert(ret); STRV_FOREACH(i, c->environment_files) { - _cleanup_globfree_ glob_t pglob = {}; + _cleanup_strv_free_ char **paths = NULL; bool ignore = false; char *fn = *i; @@ -927,7 +927,7 @@ static int exec_context_load_environment(const Unit *unit, const ExecContext *c, } /* Filename supports globbing, take all matching files */ - r = safe_glob(fn, 0, &pglob); + r = safe_glob(fn, /* flags = */ 0, &paths); if (r < 0) { if (ignore) continue; @@ -935,9 +935,9 @@ static int exec_context_load_environment(const Unit *unit, const ExecContext *c, } /* When we don't match anything, -ENOENT should be returned */ - assert(pglob.gl_pathc > 0); + assert(!strv_isempty(paths)); - FOREACH_ARRAY(path, pglob.gl_pathv, pglob.gl_pathc) { + STRV_FOREACH(path, paths) { _cleanup_strv_free_ char **p = NULL; r = load_env_file(NULL, *path, &p); diff --git a/src/test/test-glob-util.c b/src/test/test-glob-util.c index 754dc77447..a9880f15c8 100644 --- a/src/test/test-glob-util.c +++ b/src/test/test-glob-util.c @@ -9,6 +9,7 @@ #include "fs-util.h" #include "glob-util.h" #include "rm-rf.h" +#include "strv.h" #include "tests.h" #include "tmpfile-util.h" @@ -54,31 +55,25 @@ TEST(glob_exists) { TEST(safe_glob) { char template[] = "/tmp/test-glob-util.XXXXXXX"; const char *fn, *fn2, *fname; + _cleanup_strv_free_ char **v = NULL; - _cleanup_globfree_ glob_t g = {}; - int r; - - assert_se(mkdtemp(template)); + ASSERT_NOT_NULL(mkdtemp(template)); fn = strjoina(template, "/*"); - r = safe_glob(fn, 0, &g); - assert_se(r == -ENOENT); + ASSERT_ERROR(safe_glob(fn, /* flags = */ 0, &v), ENOENT); fn2 = strjoina(template, "/.*"); - r = safe_glob(fn2, GLOB_NOSORT|GLOB_BRACE, &g); - assert_se(r == -ENOENT); + ASSERT_ERROR(safe_glob(fn2, GLOB_NOSORT|GLOB_BRACE, &v), ENOENT); fname = strjoina(template, "/.foobar"); - assert_se(touch(fname) == 0); + ASSERT_OK(touch(fname)); - r = safe_glob(fn, 0, &g); - assert_se(r == -ENOENT); + ASSERT_ERROR(safe_glob(fn, /* flags = */ 0, &v), ENOENT); - r = safe_glob(fn2, GLOB_NOSORT|GLOB_BRACE, &g); - assert_se(r == 0); - assert_se(g.gl_pathc == 1); - ASSERT_STREQ(g.gl_pathv[0], fname); - ASSERT_NULL(g.gl_pathv[1]); + ASSERT_OK(safe_glob(fn2, GLOB_NOSORT|GLOB_BRACE, &v)); + ASSERT_EQ(strv_length(v), 1u); + ASSERT_STREQ(v[0], fname); + ASSERT_NULL(v[1]); (void) rm_rf(template, REMOVE_ROOT|REMOVE_PHYSICAL); } diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index 16e2742fc1..be9462b97b 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -2570,23 +2570,21 @@ finish: } static int glob_item(Context *c, Item *i, action_t action) { - _cleanup_globfree_ glob_t g = { - .gl_opendir = (void *(*)(const char *)) opendir_nomod, - }; + _cleanup_strv_free_ char **paths = NULL; int r; assert(c); assert(i); assert(action); - r = safe_glob(i->path, GLOB_NOSORT|GLOB_BRACE, &g); + r = safe_glob_full(i->path, GLOB_NOSORT|GLOB_BRACE, opendir_nomod, &paths); if (r == -ENOENT) return 0; if (r < 0) return log_error_errno(r, "Failed to glob '%s': %m", i->path); r = 0; - STRV_FOREACH(fn, g.gl_pathv) + STRV_FOREACH(fn, paths) /* We pass CREATION_EXISTING here, since if we are globbing for it, it always has to exist */ RET_GATHER(r, action(c, i, *fn, CREATION_EXISTING)); @@ -2598,23 +2596,21 @@ static int glob_item_recursively( Item *i, fdaction_t action) { - _cleanup_globfree_ glob_t g = { - .gl_opendir = (void *(*)(const char *)) opendir_nomod, - }; + _cleanup_strv_free_ char **paths = NULL; int r; assert(c); assert(i); assert(action); - r = safe_glob(i->path, GLOB_NOSORT|GLOB_BRACE, &g); + r = safe_glob_full(i->path, GLOB_NOSORT|GLOB_BRACE, opendir_nomod, &paths); if (r == -ENOENT) return 0; if (r < 0) return log_error_errno(r, "Failed to glob '%s': %m", i->path); r = 0; - STRV_FOREACH(fn, g.gl_pathv) { + STRV_FOREACH(fn, paths) { _cleanup_close_ int fd = -EBADF; /* Make sure we won't trigger/follow file object (such as device nodes, automounts, ...) From 9a9139e087e7c4fd15d90c10803b8ada983a2c68 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 23 Jun 2025 09:00:20 +0900 Subject: [PATCH 3/4] test: use safe_glob() at one more place This also makes the test use ASSERT_OK() and STRV_FOREACH() macros. No functional change, just refactoring. --- src/resolve/test-dns-packet.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/src/resolve/test-dns-packet.c b/src/resolve/test-dns-packet.c index 88c6cb127b..5e818106e8 100644 --- a/src/resolve/test-dns-packet.c +++ b/src/resolve/test-dns-packet.c @@ -9,6 +9,7 @@ #include "resolved-dns-packet.h" #include "resolved-dns-rr.h" #include "siphash24.h" +#include "strv.h" #include "tests.h" #include "unaligned.h" @@ -119,29 +120,26 @@ static void test_dns_resource_record_get_cname_target(void) { } int main(int argc, char **argv) { - int N; - _cleanup_globfree_ glob_t g = {}; + _cleanup_strv_free_ char **v = NULL; char **fnames; test_setup_logging(LOG_DEBUG); - if (argc >= 2) { - N = argc - 1; + if (argc >= 2) fnames = argv + 1; - } else { + else { _cleanup_free_ char *pkts_glob = NULL; - assert_se(get_testdata_dir("test-resolve/*.pkts", &pkts_glob) >= 0); - assert_se(glob(pkts_glob, GLOB_NOSORT, NULL, &g) == 0); - N = g.gl_pathc; - fnames = g.gl_pathv; + ASSERT_OK(get_testdata_dir("test-resolve/*.pkts", &pkts_glob)); + ASSERT_OK(safe_glob(pkts_glob, GLOB_NOSORT, &v)); + fnames = v; } - for (int i = 0; i < N; i++) { - test_packet_from_file(fnames[i], false); - puts(""); - test_packet_from_file(fnames[i], true); - if (i + 1 < N) + STRV_FOREACH(p, fnames) { + if (p != fnames) puts(""); + test_packet_from_file(*p, false); + puts(""); + test_packet_from_file(*p, true); } test_dns_resource_record_get_cname_target(); From f117272f6645fc2fe9751898770603ed07d15cea Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 23 Jun 2025 09:20:55 +0900 Subject: [PATCH 4/4] glob-util: drop unused _cleanup_globfree_ --- src/basic/glob-util.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/basic/glob-util.h b/src/basic/glob-util.h index 0e9088895f..ea3e869319 100644 --- a/src/basic/glob-util.h +++ b/src/basic/glob-util.h @@ -19,6 +19,4 @@ int glob_extend(char ***strv, const char *path, int flags); int glob_non_glob_prefix(const char *path, char **ret); -#define _cleanup_globfree_ _cleanup_(globfree) - bool string_is_glob(const char *p) _pure_;