From 1644724a4d167a22c5ac18fb564c911995f7f07b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Koutn=C3=BD?= Date: Mon, 24 Feb 2025 15:44:29 +0100 Subject: [PATCH 1/5] sd-event: Fix sd_event_source leak Hinted by CID#1591563 but the issue is different -- when sd_event_source_set_destroy_callback() fails, we would use the old n_sources value and possibly missing sd_event_source_unref() of the last added source. --- src/libsystemd/sd-event/event-util.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/libsystemd/sd-event/event-util.c b/src/libsystemd/sd-event/event-util.c index 482df37225..9e0dbd30c3 100644 --- a/src/libsystemd/sd-event/event-util.c +++ b/src/libsystemd/sd-event/event-util.c @@ -274,16 +274,17 @@ int event_forward_signals( return -ENOMEM; FOREACH_ARRAY(sig, signals, n_signals) { - r = sd_event_add_signal(e, &sources[n_sources], *sig | SD_EVENT_SIGNAL_PROCMASK, event_forward_signal_callback, child); + _cleanup_(sd_event_source_unrefp) sd_event_source *s = NULL; + r = sd_event_add_signal(e, &s, *sig | SD_EVENT_SIGNAL_PROCMASK, event_forward_signal_callback, child); if (r < 0) return r; - r = sd_event_source_set_destroy_callback(sources[n_sources], event_forward_signal_destroy); + r = sd_event_source_set_destroy_callback(s, event_forward_signal_destroy); if (r < 0) return r; sd_event_source_ref(child); - n_sources++; + sources[n_sources++] = TAKE_PTR(s); } *ret_sources = TAKE_PTR(sources); From 5eceb5a7a2f46102387505261af722d1f11f3263 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Koutn=C3=BD?= Date: Mon, 24 Feb 2025 16:22:59 +0100 Subject: [PATCH 2/5] user-record: Handle invalid uid/gid case I'm not that familiar with outer code to guide Coverity with an assert(), so consider invalid uid/gid as non-matching in order to avoid -EINVAL for bit shifts calculation. Fixes: CID#1590746 --- src/shared/group-record.c | 3 +++ src/shared/user-record.c | 3 +++ 2 files changed, 6 insertions(+) diff --git a/src/shared/group-record.c b/src/shared/group-record.c index 911c6c8f0b..76d23259ce 100644 --- a/src/shared/group-record.c +++ b/src/shared/group-record.c @@ -350,6 +350,9 @@ int group_record_match(GroupRecord *h, const UserDBMatch *match) { if (!match) return true; + if (!gid_is_valid(h->gid)) + return false; + if (h->gid < match->gid_min || h->gid > match->gid_max) return false; diff --git a/src/shared/user-record.c b/src/shared/user-record.c index 4817bec073..4c8e41cd89 100644 --- a/src/shared/user-record.c +++ b/src/shared/user-record.c @@ -2775,6 +2775,9 @@ int user_record_match(UserRecord *u, const UserDBMatch *match) { if (!match) return true; + if (!uid_is_valid(u->uid)) + return false; + if (u->uid < match->uid_min || u->uid > match->uid_max) return false; From eaeb96125a16f7cf7b071391c2d3456a8397f251 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Koutn=C3=BD?= Date: Mon, 3 Mar 2025 19:15:42 +0100 Subject: [PATCH 3/5] userdb: Fix return value of groupdb_by_name() The commit 7419291670 ("userdb: move UserDBMatch handling from userdbctl into generic userdb code to allow it to be done server side") unintentionally passes return value from group_record_match() as its return value and thus diverges from other search functions that return 0 on success. Align that by returning 0 instead of 1, all existing callers are invariant to this change. --- src/shared/userdb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/userdb.c b/src/shared/userdb.c index ea352de000..18b98685db 100644 --- a/src/shared/userdb.c +++ b/src/shared/userdb.c @@ -1356,7 +1356,7 @@ int groupdb_by_name(const char *name, const UserDBMatch *match, UserDBFlags flag if (ret) *ret = TAKE_PTR(gr); - return r; + return 0; } static int groupdb_by_gid_fallbacks( From 75e2d70eef97e92499e658829f7c3e3b4d82e4de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Koutn=C3=BD?= Date: Fri, 28 Feb 2025 15:28:04 +0100 Subject: [PATCH 4/5] user-record: Make user and group matching functions total Since we can evaluate even the case with invalid ids (non-matching) we can switch the function to pure boolean with no error cases and simpler (none) return error handling. --- src/shared/group-record.c | 2 +- src/shared/group-record.h | 2 +- src/shared/user-record.c | 2 +- src/shared/user-record.h | 2 +- src/shared/userdb.c | 20 ++++---------------- 5 files changed, 8 insertions(+), 20 deletions(-) diff --git a/src/shared/group-record.c b/src/shared/group-record.c index 76d23259ce..04989eb63a 100644 --- a/src/shared/group-record.c +++ b/src/shared/group-record.c @@ -344,7 +344,7 @@ bool group_record_matches_group_name(const GroupRecord *g, const char *group_nam return false; } -int group_record_match(GroupRecord *h, const UserDBMatch *match) { +bool group_record_match(GroupRecord *h, const UserDBMatch *match) { assert(h); if (!match) diff --git a/src/shared/group-record.h b/src/shared/group-record.h index 5705fe2511..ee8d399577 100644 --- a/src/shared/group-record.h +++ b/src/shared/group-record.h @@ -43,7 +43,7 @@ int group_record_load(GroupRecord *h, sd_json_variant *v, UserRecordLoadFlags fl int group_record_build(GroupRecord **ret, ...); int group_record_clone(GroupRecord *g, UserRecordLoadFlags flags, GroupRecord **ret); -int group_record_match(GroupRecord *h, const UserDBMatch *match); +bool group_record_match(GroupRecord *h, const UserDBMatch *match); const char* group_record_group_name_and_realm(GroupRecord *h); UserDisposition group_record_disposition(GroupRecord *h); diff --git a/src/shared/user-record.c b/src/shared/user-record.c index 4c8e41cd89..89ba2fbef4 100644 --- a/src/shared/user-record.c +++ b/src/shared/user-record.c @@ -2769,7 +2769,7 @@ bool user_name_fuzzy_match(const char *names[], size_t n_names, char **matches) return false; } -int user_record_match(UserRecord *u, const UserDBMatch *match) { +bool user_record_match(UserRecord *u, const UserDBMatch *match) { assert(u); if (!match) diff --git a/src/shared/user-record.h b/src/shared/user-record.h index 8f58c5ca93..fcd2685554 100644 --- a/src/shared/user-record.h +++ b/src/shared/user-record.h @@ -532,7 +532,7 @@ static inline void userdb_match_done(UserDBMatch *match) { } bool user_name_fuzzy_match(const char *names[], size_t n_names, char **matches); -int user_record_match(UserRecord *u, const UserDBMatch *match); +bool user_record_match(UserRecord *u, const UserDBMatch *match); bool user_record_matches_user_name(const UserRecord *u, const char *username); diff --git a/src/shared/userdb.c b/src/shared/userdb.c index 18b98685db..a420504b0c 100644 --- a/src/shared/userdb.c +++ b/src/shared/userdb.c @@ -915,10 +915,7 @@ int userdb_by_name(const char *name, const UserDBMatch *match, UserDBFlags flags /* NB: we always apply our own filtering here, explicitly, regardless if the server supported it or * not. It's more robust this way, we never know how carefully the server is written, and whether it * properly implements all details of the filtering logic. */ - r = user_record_match(ur, match); - if (r < 0) - return r; - if (r == 0) + if (!user_record_match(ur, match)) return -ENOEXEC; if (ret) @@ -1001,10 +998,7 @@ int userdb_by_uid(uid_t uid, const UserDBMatch *match, UserDBFlags flags, UserRe return r; } - r = user_record_match(ur, match); - if (r < 0) - return r; - if (r == 0) + if (!user_record_match(ur, match)) return -ENOEXEC; if (ret) @@ -1347,10 +1341,7 @@ int groupdb_by_name(const char *name, const UserDBMatch *match, UserDBFlags flag } /* As above, we apply our own client-side filtering even if server-side filtering worked, for robustness and simplicity reasons. */ - r = group_record_match(gr, match); - if (r < 0) - return r; - if (r == 0) + if (!group_record_match(gr, match)) return -ENOEXEC; if (ret) @@ -1432,10 +1423,7 @@ int groupdb_by_gid(gid_t gid, const UserDBMatch *match, UserDBFlags flags, Group return r; } - r = group_record_match(gr, match); - if (r < 0) - return r; - if (r == 0) + if (!group_record_match(gr, match)) return -ENOEXEC; if (ret) From c4b75966075e01d39556a87caa778eb63d96d6f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Koutn=C3=BD?= Date: Tue, 25 Feb 2025 11:36:51 +0100 Subject: [PATCH 5/5] TEST-13-NSPAWN.nss-mymachines: Use negative matching switch The test expects _not_ to find the patterns but the run_and_grep would still print 'FAIL:' message. Use the dedicated -n option that inverts the semantics cleaner than shell's !. --- test/units/TEST-13-NSPAWN.nss-mymachines.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/units/TEST-13-NSPAWN.nss-mymachines.sh b/test/units/TEST-13-NSPAWN.nss-mymachines.sh index 817431b449..cfae33b1b3 100755 --- a/test/units/TEST-13-NSPAWN.nss-mymachines.sh +++ b/test/units/TEST-13-NSPAWN.nss-mymachines.sh @@ -114,8 +114,8 @@ for i in {100..120}; do run_and_grep "^10\.2\.0\.$i\s+STREAM" getent ahostsv4 -s mymachines nss-mymachines-manyips done run_and_grep "^fd00:dead:beef:cafe::2\s+STREAM" getent ahostsv6 -s mymachines nss-mymachines-manyips -(! run_and_grep "^fd00:" getent ahostsv4 -s mymachines nss-mymachines-manyips) -(! run_and_grep "^10\.2:" getent ahostsv6 -s mymachines nss-mymachines-manyips) +run_and_grep -n "^fd00:" getent ahostsv4 -s mymachines nss-mymachines-manyips +run_and_grep -n "^10\.2:" getent ahostsv6 -s mymachines nss-mymachines-manyips # Multiple machines at once run_and_grep "^10\.1\.0\.2\s+nss-mymachines-singleip$" getent hosts -s mymachines nss-mymachines-{singleip,manyips}