From 42b977810d31585656ba475c86a31976e1e1f310 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 28 Mar 2020 12:46:52 +0100 Subject: [PATCH 1/4] test-strv: add missing oom check CID#1420259. --- src/test/test-strv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/test-strv.c b/src/test/test-strv.c index dd6233175c..68c128cf80 100644 --- a/src/test/test-strv.c +++ b/src/test/test-strv.c @@ -674,7 +674,7 @@ static void test_strv_push_prepend(void) { log_info("/* %s */", __func__); - a = strv_new("foo", "bar", "three"); + assert_se(a = strv_new("foo", "bar", "three")); assert_se(strv_push_prepend(&a, strdup("first")) >= 0); assert_se(streq(a[0], "first")); From e7e9a9d0dce3fe1a133a42f69a2547749b5bcd46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 28 Mar 2020 13:03:06 +0100 Subject: [PATCH 2/4] nss-systemd: add missing jump to unlock mutex CID#1412415. --- src/nss-systemd/nss-systemd.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/nss-systemd/nss-systemd.c b/src/nss-systemd/nss-systemd.c index 9c004616f5..08fe100c05 100644 --- a/src/nss-systemd/nss-systemd.c +++ b/src/nss-systemd/nss-systemd.c @@ -432,7 +432,7 @@ enum nss_status _nss_systemd_getgrent_r( if (!getgrent_data.by_membership) { r = groupdb_iterator_get(getgrent_data.iterator, &gr); if (r == -ESRCH) { - /* So we finished iterating native groups now. let's now continue with iterating + /* So we finished iterating native groups now. Let's now continue with iterating * native memberships, and generate additional group entries for any groups * referenced there that are defined in NSS only. This means for those groups there * will be two or more entries generated during iteration, but this is apparently how @@ -511,7 +511,8 @@ enum nss_status _nss_systemd_getgrent_r( if (!members) { UNPROTECT_ERRNO; *errnop = ENOMEM; - return NSS_STATUS_TRYAGAIN; + ret = NSS_STATUS_TRYAGAIN; + goto finish; } /* Note that we currently generate one group entry per user that is part of a From 29d4392ca05a31db53176f552041bb6183351d63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 28 Mar 2020 13:24:44 +0100 Subject: [PATCH 3/4] basic: add _cleanup_ wrappers for pthread_mutex_{lock,unlock} I put the helper functions in a separate header file, because they don't fit anywhere else. pthread_mutex_{lock,unlock} is used in two places: nss-systemd and hashmap. I don't indent to convert hashmap to use the helpers, because there it'd make the code more complicated. Is it worth to create a new header file even if the only use is in nss-systemd.c? I think yes, because it feels clean and also I think it's likely that pthread_mutex_{lock,unlock} will be used in other places later. --- src/basic/meson.build | 1 + src/basic/pthread-util.h | 16 ++++++++++++++++ 2 files changed, 17 insertions(+) create mode 100644 src/basic/pthread-util.h diff --git a/src/basic/meson.build b/src/basic/meson.build index ccb22e1595..85953dab0c 100644 --- a/src/basic/meson.build +++ b/src/basic/meson.build @@ -169,6 +169,7 @@ basic_sources = files(''' process-util.h procfs-util.c procfs-util.h + pthread-util.h quota-util.c quota-util.h random-util.c diff --git a/src/basic/pthread-util.h b/src/basic/pthread-util.h new file mode 100644 index 0000000000..dc3eaba436 --- /dev/null +++ b/src/basic/pthread-util.h @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: LGPL-2.1+ */ +#pragma once + +#include + +#include "macro.h" + +static inline pthread_mutex_t* pthread_mutex_lock_assert(pthread_mutex_t *mutex) { + assert_se(pthread_mutex_lock(mutex) == 0); + return mutex; +} + +static inline void pthread_mutex_unlock_assertp(pthread_mutex_t **mutexp) { + if (*mutexp) + assert_se(pthread_mutex_unlock(*mutexp) == 0); +} From 37bc9dcc0988fe81b2d98ad0686dd33df2271c2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 28 Mar 2020 13:26:25 +0100 Subject: [PATCH 4/4] nss-systemd: use _cleanup_ for pthread_mutex_{lock,unlock} v2: separate the declaration from the assignment to appease clang. --- src/nss-systemd/nss-systemd.c | 101 +++++++++++++--------------------- 1 file changed, 39 insertions(+), 62 deletions(-) diff --git a/src/nss-systemd/nss-systemd.c b/src/nss-systemd/nss-systemd.c index 08fe100c05..581b7959bd 100644 --- a/src/nss-systemd/nss-systemd.c +++ b/src/nss-systemd/nss-systemd.c @@ -9,6 +9,7 @@ #include "group-record-nss.h" #include "macro.h" #include "nss-util.h" +#include "pthread-util.h" #include "signal-util.h" #include "strv.h" #include "user-util.h" @@ -277,10 +278,11 @@ static enum nss_status nss_systemd_endent(GetentData *p) { assert(p); - assert_se(pthread_mutex_lock(&p->mutex) == 0); + _cleanup_(pthread_mutex_unlock_assertp) pthread_mutex_t *_l = NULL; + _l = pthread_mutex_lock_assert(&p->mutex); + p->iterator = userdb_iterator_free(p->iterator); p->by_membership = false; - assert_se(pthread_mutex_unlock(&p->mutex) == 0); return NSS_STATUS_SUCCESS; } @@ -294,45 +296,41 @@ enum nss_status _nss_systemd_endgrent(void) { } enum nss_status _nss_systemd_setpwent(int stayopen) { - enum nss_status ret; - PROTECT_ERRNO; BLOCK_SIGNALS(NSS_SIGNALS_BLOCK); if (userdb_nss_compat_is_enabled() <= 0) return NSS_STATUS_NOTFOUND; - assert_se(pthread_mutex_lock(&getpwent_data.mutex) == 0); + _cleanup_(pthread_mutex_unlock_assertp) pthread_mutex_t *_l = NULL; + int r; + + _l = pthread_mutex_lock_assert(&getpwent_data.mutex); getpwent_data.iterator = userdb_iterator_free(getpwent_data.iterator); getpwent_data.by_membership = false; - ret = userdb_all(nss_glue_userdb_flags(), &getpwent_data.iterator) < 0 ? - NSS_STATUS_UNAVAIL : NSS_STATUS_SUCCESS; - - assert_se(pthread_mutex_unlock(&getpwent_data.mutex) == 0); - return ret; + r = userdb_all(nss_glue_userdb_flags(), &getpwent_data.iterator); + return r < 0 ? NSS_STATUS_UNAVAIL : NSS_STATUS_SUCCESS; } enum nss_status _nss_systemd_setgrent(int stayopen) { - enum nss_status ret; - PROTECT_ERRNO; BLOCK_SIGNALS(NSS_SIGNALS_BLOCK); if (userdb_nss_compat_is_enabled() <= 0) return NSS_STATUS_NOTFOUND; - assert_se(pthread_mutex_lock(&getgrent_data.mutex) == 0); + _cleanup_(pthread_mutex_unlock_assertp) pthread_mutex_t *_l = NULL; + int r; + + _l = pthread_mutex_lock_assert(&getgrent_data.mutex); getgrent_data.iterator = userdb_iterator_free(getgrent_data.iterator); getpwent_data.by_membership = false; - ret = groupdb_all(nss_glue_userdb_flags(), &getgrent_data.iterator) < 0 ? - NSS_STATUS_UNAVAIL : NSS_STATUS_SUCCESS; - - assert_se(pthread_mutex_unlock(&getgrent_data.mutex) == 0); - return ret; + r = groupdb_all(nss_glue_userdb_flags(), &getgrent_data.iterator); + return r < 0 ? NSS_STATUS_UNAVAIL : NSS_STATUS_SUCCESS; } enum nss_status _nss_systemd_getpwent_r( @@ -341,7 +339,6 @@ enum nss_status _nss_systemd_getpwent_r( int *errnop) { _cleanup_(user_record_unrefp) UserRecord *ur = NULL; - enum nss_status ret; int r; PROTECT_ERRNO; @@ -359,40 +356,33 @@ enum nss_status _nss_systemd_getpwent_r( if (!r) return NSS_STATUS_NOTFOUND; - assert_se(pthread_mutex_lock(&getpwent_data.mutex) == 0); + _cleanup_(pthread_mutex_unlock_assertp) pthread_mutex_t *_l = NULL; + + _l = pthread_mutex_lock_assert(&getpwent_data.mutex); if (!getpwent_data.iterator) { UNPROTECT_ERRNO; *errnop = EHOSTDOWN; - ret = NSS_STATUS_UNAVAIL; - goto finish; + return NSS_STATUS_UNAVAIL; } r = userdb_iterator_get(getpwent_data.iterator, &ur); - if (r == -ESRCH) { - ret = NSS_STATUS_NOTFOUND; - goto finish; - } + if (r == -ESRCH) + return NSS_STATUS_NOTFOUND; if (r < 0) { UNPROTECT_ERRNO; *errnop = -r; - ret = NSS_STATUS_UNAVAIL; - goto finish; + return NSS_STATUS_UNAVAIL; } r = nss_pack_user_record(ur, result, buffer, buflen); if (r < 0) { UNPROTECT_ERRNO; *errnop = -r; - ret = NSS_STATUS_TRYAGAIN; - goto finish; + return NSS_STATUS_TRYAGAIN; } - ret = NSS_STATUS_SUCCESS; - -finish: - assert_se(pthread_mutex_unlock(&getpwent_data.mutex) == 0); - return ret; + return NSS_STATUS_SUCCESS; } enum nss_status _nss_systemd_getgrent_r( @@ -402,7 +392,6 @@ enum nss_status _nss_systemd_getgrent_r( _cleanup_(group_record_unrefp) GroupRecord *gr = NULL; _cleanup_free_ char **members = NULL; - enum nss_status ret; int r; PROTECT_ERRNO; @@ -420,13 +409,14 @@ enum nss_status _nss_systemd_getgrent_r( if (!r) return NSS_STATUS_UNAVAIL; - assert_se(pthread_mutex_lock(&getgrent_data.mutex) == 0); + _cleanup_(pthread_mutex_unlock_assertp) pthread_mutex_t *_l = NULL; + + _l = pthread_mutex_lock_assert(&getgrent_data.mutex); if (!getgrent_data.iterator) { UNPROTECT_ERRNO; *errnop = EHOSTDOWN; - ret = NSS_STATUS_UNAVAIL; - goto finish; + return NSS_STATUS_UNAVAIL; } if (!getgrent_data.by_membership) { @@ -444,23 +434,20 @@ enum nss_status _nss_systemd_getgrent_r( if (r < 0) { UNPROTECT_ERRNO; *errnop = -r; - ret = NSS_STATUS_UNAVAIL; - goto finish; + return NSS_STATUS_UNAVAIL; } getgrent_data.by_membership = true; } else if (r < 0) { UNPROTECT_ERRNO; *errnop = -r; - ret = NSS_STATUS_UNAVAIL; - goto finish; + return NSS_STATUS_UNAVAIL; } else if (!STR_IN_SET(gr->group_name, root_group.gr_name, nobody_group.gr_name)) { r = membershipdb_by_group_strv(gr->group_name, nss_glue_userdb_flags(), &members); if (r < 0) { UNPROTECT_ERRNO; *errnop = -r; - ret = NSS_STATUS_UNAVAIL; - goto finish; + return NSS_STATUS_UNAVAIL; } } } @@ -472,15 +459,12 @@ enum nss_status _nss_systemd_getgrent_r( _cleanup_free_ char *user_name = NULL, *group_name = NULL; r = membershipdb_iterator_get(getgrent_data.iterator, &user_name, &group_name); - if (r == -ESRCH) { - ret = NSS_STATUS_NOTFOUND; - goto finish; - } + if (r == -ESRCH) + return NSS_STATUS_NOTFOUND; if (r < 0) { UNPROTECT_ERRNO; *errnop = -r; - ret = NSS_STATUS_UNAVAIL; - goto finish; + return NSS_STATUS_UNAVAIL; } if (STR_IN_SET(user_name, root_passwd.pw_name, nobody_passwd.pw_name)) @@ -494,8 +478,7 @@ enum nss_status _nss_systemd_getgrent_r( if (lock_fd < 0 && lock_fd != -EBUSY) { UNPROTECT_ERRNO; *errnop = -lock_fd; - ret = NSS_STATUS_UNAVAIL; - goto finish; + return NSS_STATUS_UNAVAIL; } } @@ -511,8 +494,7 @@ enum nss_status _nss_systemd_getgrent_r( if (!members) { UNPROTECT_ERRNO; *errnop = ENOMEM; - ret = NSS_STATUS_TRYAGAIN; - goto finish; + return NSS_STATUS_TRYAGAIN; } /* Note that we currently generate one group entry per user that is part of a @@ -526,15 +508,10 @@ enum nss_status _nss_systemd_getgrent_r( if (r < 0) { UNPROTECT_ERRNO; *errnop = -r; - ret = NSS_STATUS_TRYAGAIN; - goto finish; + return NSS_STATUS_TRYAGAIN; } - ret = NSS_STATUS_SUCCESS; - -finish: - assert_se(pthread_mutex_unlock(&getgrent_data.mutex) == 0); - return ret; + return NSS_STATUS_SUCCESS; } enum nss_status _nss_systemd_initgroups_dyn(