API: libcrmservice: Change services_action_user() not-found return code

This is mostly a refactor commit that causes a small change to the
libcrmservice API. See final paragraph.

Create a new pcmk__lookup_user() to replace crm_user_lookup().

This new function calls getpwnam() instead of getpwnam_r(). Since
Pacemaker is single-threaded and doesn't need reentrancy for this call,
I can't think of any good reason why we ever needed getpwnam_r(). It
requires defining a struct passwd and allocating a buffer to copy its
string members into, which makes everything more complicated. getpwnam()
is basically equivalent but returns a pointer and sets errno. All we
need are the uid and gid members.

Most of this commit is actually changes to mocking and unit testing, and
having to remember how the expect and will_return stuff works, due to
using getpwnam() instead of getpwnam_r().

This changes the specific error return codes from services_action_user()
in some cases. The default "no match" return code, if no entry pointer
is returned and errno is 0, is now ENOENT. Previously it was EINVAL. I
think this is acceptable, even though it technically changes the API. We
never documented specific error codes. As long as a caller considers any
nonzero code to be an error, nothing changes. (I'm not sure if there's
any legitimate use case for an external caller to create or modify a
service action directly anyway.)

Signed-off-by: Reid Wahl <[email protected]>

Author: nrwahl2

daemons/based/based_remote.c

@@ -151,16 +151,18 @@ static int
 check_group_membership(const char *usr, const char *grp)
 {
     int index = 0;
-    struct passwd *pwd = NULL;
+    gid_t gid = 0;
     struct group *group = NULL;
+    int rc = pcmk_rc_ok;
 
-    pwd = getpwnam(usr);
-    if (pwd == NULL) {
-        crm_notice("Rejecting remote client: '%s' is not a valid user", usr);
+    rc = pcmk__lookup_user(usr, NULL, &gid);
+    if (rc != pcmk_rc_ok) {
+        crm_notice("Rejecting remote client: could not find user '%s': %s",
+                   usr, pcmk_rc_str(rc));
         return FALSE;
     }
 
-    group = getgrgid(pwd->pw_gid);
+    group = getgrgid(gid);
     if (group != NULL && pcmk__str_eq(grp, group->gr_name, pcmk__str_none)) {
         return TRUE;
     }

include/crm/common/internal.h

@@ -10,9 +10,11 @@
 #ifndef PCMK__CRM_COMMON_INTERNAL__H
 #define PCMK__CRM_COMMON_INTERNAL__H
 
-#include <unistd.h>             // pid_t, getpid()
+#include <pwd.h>                // struct passwd
+#include <unistd.h>             // getpid()
 #include <stdbool.h>            // bool
 #include <stdint.h>             // uint8_t, uint64_t
+#include <sys/types.h>          // pid_t, uid_t, gid_t
 #include <inttypes.h>           // PRIu64
 
 #include <glib.h>               // guint, GList, GHashTable
@@ -249,6 +251,7 @@ pcmk__flag_text(uint64_t flag_group, uint64_t flags)
 int pcmk__daemon_user(uid_t *uid, gid_t *gid);
 void pcmk__daemonize(const char *name, const char *pidfile);
 char *pcmk__generate_uuid(void);
+int pcmk__lookup_user(const char *name, uid_t *uid, gid_t *gid);
 void pcmk__panic(const char *reason);
 pid_t pcmk__locate_sbd(void);
 void pcmk__sleep_ms(unsigned int ms);

lib/common/mock.c

@@ -356,41 +356,30 @@ __wrap_fopen64(const char *pathname, const char *mode)
 }
 #endif
 
-/* getpwnam_r()
+/* getpwnam()
  *
- * If pcmk__mock_getpwnam_r is set to true, later calls to getpwnam_r() must be
+ * If pcmk__mock_getpwnam is set to true, later calls to getpwnam() must be
  * preceded by:
  *
- *     expect_*(__wrap_getpwnam_r, name[, ...]);
- *     expect_*(__wrap_getpwnam_r, pwd[, ...]);
- *     expect_*(__wrap_getpwnam_r, buf[, ...]);
- *     expect_*(__wrap_getpwnam_r, buflen[, ...]);
- *     expect_*(__wrap_getpwnam_r, result[, ...]);
- *     will_return(__wrap_getpwnam_r, return_value);
- *     will_return(__wrap_getpwnam_r, ptr_to_result_struct);
+ *     expect_*(__wrap_getpwnam, name[, ...]);
+ *     will_return(__wrap_getpwnam, errno_to_set);
+ *     will_return(__wrap_getpwnam, ptr_to_result_struct);
  *
  * expect_* functions: https://api.cmocka.org/group__cmocka__param.html
  */
 
-bool pcmk__mock_getpwnam_r = false;
+bool pcmk__mock_getpwnam = false;
 
-int
-__wrap_getpwnam_r(const char *name, struct passwd *pwd, char *buf,
-                  size_t buflen, struct passwd **result)
+struct passwd *
+__wrap_getpwnam(const char *name)
 {
-    if (pcmk__mock_getpwnam_r) {
-        int retval = mock_type(int);
-
+    if (pcmk__mock_getpwnam) {
         check_expected_ptr(name);
-        check_expected_ptr(pwd);
-        check_expected_ptr(buf);
-        check_expected(buflen);
-        check_expected_ptr(result);
-        *result = mock_ptr_type(struct passwd *);
-        return retval;
+        errno = mock_type(int);
+        return mock_ptr_type(struct passwd *);
 
     } else {
-        return __real_getpwnam_r(name, pwd, buf, buflen, result);
+        return __real_getpwnam(name);
     }
 }
 

lib/common/mock_private.h

@@ -1,5 +1,5 @@
 /*
- * Copyright 2021-2024 the Pacemaker project contributors
+ * Copyright 2021-2025 the Pacemaker project contributors
  *
  * The version control history for this file may have further details.
  *
@@ -74,11 +74,9 @@ struct group * __real_getgrent(void);
 void __wrap_endgrent(void);
 void __real_endgrent(void);
 
-extern bool pcmk__mock_getpwnam_r;
-int __real_getpwnam_r(const char *name, struct passwd *pwd,
-                      char *buf, size_t buflen, struct passwd **result);
-int __wrap_getpwnam_r(const char *name, struct passwd *pwd,
-                      char *buf, size_t buflen, struct passwd **result);
+extern bool pcmk__mock_getpwnam;
+struct passwd *__real_getpwnam(const char *name);
+struct passwd *__wrap_getpwnam(const char *name);
 
 extern bool pcmk__mock_readlink;
 ssize_t __real_readlink(const char *restrict path, char *restrict buf,

lib/common/tests/utils/Makefile.am

@@ -13,12 +13,12 @@ include $(top_srcdir)/mk/unittest.mk
 
 # Add "_test" to the end of all test program names to simplify .gitignore.
 check_PROGRAMS = compare_version_test			\
-		 crm_user_lookup_test			\
 		 pcmk__daemon_user_test			\
 		 pcmk__fail_attr_name_test 		\
 		 pcmk__failcount_name_test 		\
 		 pcmk__getpid_s_test 			\
 		 pcmk__lastfailure_name_test 		\
+		 pcmk__lookup_user_test			\
 		 pcmk__realloc_test 			\
 		 pcmk__timeout_ms2s_test
 

lib/common/tests/utils/crm_user_lookup_test.c

@@ -23,20 +23,15 @@ no_matching_pwent(void **state)
     uid_t uid = 0;
     gid_t gid = 0;
 
-    // Set getpwnam_r() return value and result parameter
-    pcmk__mock_getpwnam_r = true;
+    pcmk__mock_getpwnam = true;
 
-    expect_string(__wrap_getpwnam_r, name, "hacluster");
-    expect_any(__wrap_getpwnam_r, pwd);
-    expect_any(__wrap_getpwnam_r, buf);
-    expect_value(__wrap_getpwnam_r, buflen, PCMK__PW_BUFFER_LEN);
-    expect_any(__wrap_getpwnam_r, result);
-    will_return(__wrap_getpwnam_r, ENOENT);
-    will_return(__wrap_getpwnam_r, NULL);
+    expect_string(__wrap_getpwnam, name, "hacluster");
+    will_return(__wrap_getpwnam, 0);
+    will_return(__wrap_getpwnam, NULL);
 
     assert_int_equal(pcmk__daemon_user(&uid, &gid), ENOENT);
 
-    pcmk__mock_getpwnam_r = false;
+    pcmk__mock_getpwnam = false;
 }
 
 static void
@@ -48,25 +43,21 @@ entry_found(void **state)
     // We don't care about the other fields of the passwd entry
     struct passwd returned_ent = { .pw_uid = 1000, .pw_gid = 1000 };
 
-    // Test getpwnam_r() returning a valid passwd entry with null output args
+    // Test getpwnam() returning a valid passwd entry with null output args
 
-    pcmk__mock_getpwnam_r = true;
+    pcmk__mock_getpwnam = true;
 
-    expect_string(__wrap_getpwnam_r, name, "hacluster");
-    expect_any(__wrap_getpwnam_r, pwd);
-    expect_any(__wrap_getpwnam_r, buf);
-    expect_value(__wrap_getpwnam_r, buflen, PCMK__PW_BUFFER_LEN);
-    expect_any(__wrap_getpwnam_r, result);
-    will_return(__wrap_getpwnam_r, 0);
-    will_return(__wrap_getpwnam_r, &returned_ent);
+    expect_string(__wrap_getpwnam, name, "hacluster");
+    will_return(__wrap_getpwnam, 0);
+    will_return(__wrap_getpwnam, &returned_ent);
 
     assert_int_equal(pcmk__daemon_user(NULL, NULL), pcmk_rc_ok);
 
-    // Test getpwnam_r() returning a valid passwd entry with non-NULL outputs
+    // Test getpwnam() returning a valid passwd entry with non-NULL outputs
 
     /* We don't need to call expect_*() or will_return() again because
      * pcmk__daemon_user() will have cached the uid/gid from the previous call
-     * and won't make another call to getpwnam_r().
+     * and won't make another call to getpwnam().
      */
     assert_int_equal(pcmk__daemon_user(&uid, NULL), pcmk_rc_ok);
     assert_int_equal(uid, 1000);
@@ -82,7 +73,7 @@ entry_found(void **state)
     assert_int_equal(uid, 1000);
     assert_int_equal(gid, 1000);
 
-    pcmk__mock_getpwnam_r = false;
+    pcmk__mock_getpwnam = false;
 }
 
 PCMK__UNIT_TEST(NULL, NULL,