Merge branch 'issue343-3' into 'master'

sysdeps: Improve error reporting for looking up a user

See merge request dbus/dbus!442

2 files changed, 53 insertions, 38 deletions

diff --git a/dbus/dbus-sysdeps-unix.c b/dbus/dbus-sysdeps-unix.c
index 60bdc6f7..8438587f 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;
     int result;
+#ifdef HAVE_GETPWNAM_R
     size_t buflen;
-    char *buf;
     struct passwd p_str;
 
     /* retrieve maximum needed size for buf */
@@ -2773,54 +2773,69 @@ fill_user_info (DBusUserInfo       *info,
             break;
           }
       }
-    if (result == 0 && p == &p_str)
-      {
-        if (!fill_user_info_from_passwd (p, info, error))
-          {
-            dbus_free (buf);
-            return FALSE;
-          }
-        dbus_free (buf);
-      }
-    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 : "???");
-        dbus_free (buf);
-        return FALSE;
-      }
-  }
+
+    /* 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 */
-    struct passwd *p;
-
 #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)
       {
         if (!fill_user_info_from_passwd (p, info, error))
           {
+            dbus_free (buf);
             return FALSE;
           }
+        dbus_free (buf);
       }
     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;
+        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 (result),
+                          "Looking up user ID " DBUS_UID_FORMAT ": %s",
+                          uid, error_str);
+        else
+          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);
+        dbus_free (buf);
         return FALSE;
       }
   }
-#endif  /* ! HAVE_GETPWNAM_R */
 
   /* Fill this in so we can use it to get groups */
   username_c = info->username;
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;
 }
 /** @} */