From 80b90e570e0cc950491aaeacb008b101922d8a57 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 29 Jun 2023 17:00:58 +0100 Subject: [PATCH 1/4] sysdeps-unix: Deduplicate error handling for getpwnam and getpwnam_r The only difference between these was that we only needed to allocate and free buf in the getpwnam_r case. We expect that all reasonable Unix platforms will have getpwnam_r (it's in POSIX) so adding a no-op dbus_free(NULL) to the getpwnam code path seems harmless. This will be helpful when we make the error handling better, in a subsequent commit. Helps: https://gitlab.freedesktop.org/dbus/dbus/-/issues/343 Signed-off-by: Simon McVittie --- dbus/dbus-sysdeps-unix.c | 48 ++++++++++++++-------------------------- 1 file changed, 16 insertions(+), 32 deletions(-) diff --git a/dbus/dbus-sysdeps-unix.c b/dbus/dbus-sysdeps-unix.c index 60bdc6f7..4dfdf549 100644 --- a/dbus/dbus-sysdeps-unix.c +++ b/dbus/dbus-sysdeps-unix.c @@ -2727,12 +2727,12 @@ fill_user_info (DBusUserInfo *info, * checks */ -#ifdef HAVE_GETPWNAM_R { struct passwd *p; + char *buf = NULL; +#ifdef HAVE_GETPWNAM_R int result; size_t buflen; - char *buf; struct passwd p_str; /* retrieve maximum needed size for buf */ @@ -2773,7 +2773,20 @@ fill_user_info (DBusUserInfo *info, break; } } - if (result == 0 && p == &p_str) + + if (result != 0 || p != &p_str) + p = NULL; +#else /* ! HAVE_GETPWNAM_R */ + /* I guess we're screwed on thread safety here */ +#warning getpwnam_r() not available, please report this to the dbus maintainers with details of your OS + + if (uid != DBUS_UID_UNSET) + p = getpwuid (uid); + else + p = getpwnam (username_c); +#endif /* ! HAVE_GETPWNAM_R */ + + if (p != NULL) { if (!fill_user_info_from_passwd (p, info, error)) { @@ -2792,35 +2805,6 @@ fill_user_info (DBusUserInfo *info, return FALSE; } } -#else /* ! HAVE_GETPWNAM_R */ - { - /* I guess we're screwed on thread safety here */ - struct passwd *p; - -#warning getpwnam_r() not available, please report this to the dbus maintainers with details of your OS - - if (uid != DBUS_UID_UNSET) - p = getpwuid (uid); - else - p = getpwnam (username_c); - - if (p != NULL) - { - if (!fill_user_info_from_passwd (p, info, error)) - { - return FALSE; - } - } - else - { - dbus_set_error (error, _dbus_error_from_errno (errno), - "User \"%s\" unknown or no memory to allocate password entry\n", - username_c ? username_c : "???"); - _dbus_verbose ("User %s unknown\n", username_c ? username_c : "???"); - return FALSE; - } - } -#endif /* ! HAVE_GETPWNAM_R */ /* Fill this in so we can use it to get groups */ username_c = info->username; From 12b367daaaabab838537bfbe8164f095a6294ec3 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 29 Jun 2023 17:03:51 +0100 Subject: [PATCH 2/4] sysdeps: Give a more useful error if unable to resolve a numeric uid If we want to get the struct passwd corresponding to uid 42, but we can't, it's much better to say User ID "42" unknown rather than User "???" unknown Helps: https://gitlab.freedesktop.org/dbus/dbus/-/issues/343 Signed-off-by: Simon McVittie --- dbus/dbus-sysdeps-unix.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/dbus/dbus-sysdeps-unix.c b/dbus/dbus-sysdeps-unix.c index 4dfdf549..d08516ab 100644 --- a/dbus/dbus-sysdeps-unix.c +++ b/dbus/dbus-sysdeps-unix.c @@ -2797,10 +2797,19 @@ fill_user_info (DBusUserInfo *info, } else { - dbus_set_error (error, _dbus_error_from_errno (errno), - "User \"%s\" unknown or no memory to allocate password entry\n", - username_c ? username_c : "???"); - _dbus_verbose ("User %s unknown\n", username_c ? username_c : "???"); + DBusError local_error = DBUS_ERROR_INIT; + + if (uid != DBUS_UID_UNSET) + dbus_set_error (&local_error, _dbus_error_from_errno (errno), + "User ID " DBUS_UID_FORMAT " unknown or no memory to allocate password entry", + uid); + else + dbus_set_error (&local_error, _dbus_error_from_errno (errno), + "User \"%s\" unknown or no memory to allocate password entry\n", + username_c ? username_c : "???"); + + _dbus_verbose ("%s", local_error.message); + dbus_move_error (&local_error, error); dbus_free (buf); return FALSE; } From 8de818153b6d75f6d5592949a6bf3d9d711bcfa4 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 29 Jun 2023 17:28:40 +0100 Subject: [PATCH 3/4] sysdeps: Improve error reporting for looking up a user Our implementation always assumed that both code paths set errno, but according to their API documentation, getpwnam_r and getpwuid_r actually don't: they *return* a code from the same pseudo-enum as errno. They also return 0 (but with a NULL struct passwd) if the user is not found, which these APIs don't count as an error (but we do). Similarly, in the legacy getpwnam/getpwuid code path, it is unspecified whether looking up a nonexistent user will set errno or not. Having retrieved an errno-like error code, we might as well use it in the human-readable message and not just the machine-readable code, because the human-readable message is what ends up in the system log. Helps: https://gitlab.freedesktop.org/dbus/dbus/-/issues/343 Signed-off-by: Simon McVittie --- dbus/dbus-sysdeps-unix.c | 36 +++++++++++++++++++++++++++++------- 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/dbus/dbus-sysdeps-unix.c b/dbus/dbus-sysdeps-unix.c index d08516ab..8438587f 100644 --- a/dbus/dbus-sysdeps-unix.c +++ b/dbus/dbus-sysdeps-unix.c @@ -2730,8 +2730,8 @@ fill_user_info (DBusUserInfo *info, { struct passwd *p; char *buf = NULL; -#ifdef HAVE_GETPWNAM_R int result; +#ifdef HAVE_GETPWNAM_R size_t buflen; struct passwd p_str; @@ -2774,16 +2774,32 @@ fill_user_info (DBusUserInfo *info, } } + /* There are three possibilities: + * - an error: result is a nonzero error code, p should be NULL + * - name or uid not found: result is 0, p is NULL + * - success: result is 0, p should be &p_str + * + * Ensure that in all failure cases, p is set to NULL, matching the + * getpwuid/getpwnam interface. */ if (result != 0 || p != &p_str) p = NULL; + #else /* ! HAVE_GETPWNAM_R */ /* I guess we're screwed on thread safety here */ #warning getpwnam_r() not available, please report this to the dbus maintainers with details of your OS + /* It is unspecified whether "failed to find" counts as an error, + * or whether it's reported as p == NULL without touching errno. + * Reset errno so we can distinguish. */ + errno = 0; + if (uid != DBUS_UID_UNSET) p = getpwuid (uid); else p = getpwnam (username_c); + + /* Always initialized, but only meaningful if p is NULL */ + result = errno; #endif /* ! HAVE_GETPWNAM_R */ if (p != NULL) @@ -2798,15 +2814,21 @@ fill_user_info (DBusUserInfo *info, else { DBusError local_error = DBUS_ERROR_INIT; + const char *error_str; + + if (result == 0) + error_str = "not found"; + else + error_str = _dbus_strerror (result); if (uid != DBUS_UID_UNSET) - dbus_set_error (&local_error, _dbus_error_from_errno (errno), - "User ID " DBUS_UID_FORMAT " unknown or no memory to allocate password entry", - uid); + dbus_set_error (&local_error, _dbus_error_from_errno (result), + "Looking up user ID " DBUS_UID_FORMAT ": %s", + uid, error_str); else - dbus_set_error (&local_error, _dbus_error_from_errno (errno), - "User \"%s\" unknown or no memory to allocate password entry\n", - username_c ? username_c : "???"); + dbus_set_error (&local_error, _dbus_error_from_errno (result), + "Looking up user \"%s\": %s", + username_c ? username_c : "???", error_str); _dbus_verbose ("%s", local_error.message); dbus_move_error (&local_error, error); From 6f6f861a3a593e959d23e05eacfc1307c713dc57 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 29 Jun 2023 17:51:48 +0100 Subject: [PATCH 4/4] userdb: Use "goto out" pattern for _dbus_groups_from_uid() This makes it easier to verify that _dbus_user_database_unlock_system() is called on all exit paths. The only early-return is when locking the userdb failed. Signed-off-by: Simon McVittie --- dbus/dbus-userdb-util.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/dbus/dbus-userdb-util.c b/dbus/dbus-userdb-util.c index 558acff1..73624495 100644 --- a/dbus/dbus-userdb-util.c +++ b/dbus/dbus-userdb-util.c @@ -353,6 +353,8 @@ _dbus_groups_from_uid (dbus_uid_t uid, { DBusUserDatabase *db; const DBusUserInfo *info; + dbus_bool_t ret = FALSE; + *group_ids = NULL; *n_group_ids = 0; @@ -366,15 +368,11 @@ _dbus_groups_from_uid (dbus_uid_t uid, if (db == NULL) { _DBUS_SET_OOM (error); - _dbus_user_database_unlock_system (); - return FALSE; + goto out; } if (!_dbus_user_database_get_uid (db, uid, &info, error)) - { - _dbus_user_database_unlock_system (); - return FALSE; - } + goto out; _dbus_assert (info->uid == uid); @@ -384,8 +382,7 @@ _dbus_groups_from_uid (dbus_uid_t uid, if (*group_ids == NULL) { _DBUS_SET_OOM (error); - _dbus_user_database_unlock_system (); - return FALSE; + goto out; } *n_group_ids = info->n_group_ids; @@ -393,7 +390,10 @@ _dbus_groups_from_uid (dbus_uid_t uid, memcpy (*group_ids, info->group_ids, info->n_group_ids * sizeof (dbus_gid_t)); } + ret = TRUE; +out: + _DBUS_ASSERT_ERROR_XOR_BOOL (error, ret); _dbus_user_database_unlock_system (); - return TRUE; + return ret; } /** @} */