Attachment 67350 Details for Bug 55067 – util: Clean up spawn_with_login_uid() error handling

[patch] util: Clean up spawn_with_login_uid() error handling

util-Clean-up-spawnwithloginuid-error-handling.patch (text/plain), 4.21 KB, created by Colin Walters on 2012-09-18 19:19:26 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Colin Walters

Created: 2012-09-18 19:19:26 UTC

Size: 4.21 KB

patch

obsolete

>From f31ce5c504054f349690ed45726e426085c3ffd5 Mon Sep 17 00:00:00 2001
>From: Colin Walters <walters@verbum.org>
>Date: Tue, 18 Sep 2012 14:58:57 -0400
>Subject: [PATCH] util: Clean up spawn_with_login_uid() error handling
>
>So I looked at this function because we were emitting a warning due to
>ignoring the return value of write().  This turned into some yak
>shaving around error handling:
>
>1) We were only checking WEXITSTATUS(), but this is broken in the
>   case where !WIFEXITED(), for example WIFSIGNALED().  If available,
>   use g_spawn_check_exit_status() from newer GLib which makes this
>   easy.
>2) If we failed to read the loginuid, we would happily pass
>   uninitialized data from the stack and write that as our login uid.
>   Handle this by only doing the loginuid dance if we read something.
>
>Also, use G_N_ELEMENTS() instead of hardcoding the length again.
>
>https://bugs.freedesktop.org/show_bug.cgi?id=55067
>---
> src/util.c | 72 +++++++++++++++++++++++++++++++++++++++++---------------------
> 1 file changed, 48 insertions(+), 24 deletions(-)
>
>diff --git a/src/util.c b/src/util.c
>index 2e363a3..c6ff5f1 100644
>--- a/src/util.c
>+++ b/src/util.c
>@@ -201,6 +201,32 @@ get_caller_loginuid (GDBusMethodInvocation *context, gchar *loginuid, gint size)
>         g_free (path);
> }
> 
>+static gboolean
>+compat_check_exit_status (int      estatus,
>+                          GError **error)
>+{
>+#if GLIB_CHECK_VERSION(2, 33, 12)
>+        return g_spawn_check_exit_status (estatus, error);
>+#else
>+        if (!WIFEXITED (estatus)) {
>+                g_set_error (error,
>+                             G_SPAWN_ERROR,
>+                             G_SPAWN_ERROR_FAILED,
>+                             "Exited abnormally");
>+                return FALSE;
>+        }
>+        if (WEXITSTATUS (estatus) != 0) {
>+                g_set_error (error,
>+                             G_SPAWN_ERROR,
>+                             G_SPAWN_ERROR_FAILED,
>+                             "Exited with code %d",
>+                             WEXITSTATUS(estatus));
>+                return FALSE;
>+        }
>+        return TRUE;
>+#endif
>+}
>+
> static void
> setup_loginuid (gpointer data)
> {
>@@ -208,8 +234,11 @@ setup_loginuid (gpointer data)
>         int fd;
> 
>         fd = open ("/proc/self/loginuid", O_WRONLY);
>-        write (fd, id, strlen (id));
>-        close (fd);
>+        if (fd == -1)
>+                _exit (1);
>+        if (write (fd, id, strlen (id)) == -1)
>+                _exit (1);
>+        (void) close (fd);
> }
> 
> gboolean
>@@ -217,35 +246,30 @@ spawn_with_login_uid (GDBusMethodInvocation  *context,
>                       const gchar            *argv[],
>                       GError                **error)
> {
>-        GError *local_error;
>+        gboolean ret = FALSE;
>         gchar loginuid[20];
>-        gchar *std_err;
>         gint status;
>+        GSpawnChildSetupFunc setup_func;
>+        gpointer setup_data;
> 
>-        get_caller_loginuid (context, loginuid, 20);
>+        loginuid[0] = '\0';
>+        get_caller_loginuid (context, loginuid, G_N_ELEMENTS (loginuid));
> 
>-        local_error = NULL;
>-        std_err = NULL;
>-
>-        if (!g_spawn_sync (NULL, (gchar**)argv, NULL, 0, setup_loginuid, loginuid, NULL, &std_err, &status, &local_error)) {
>-                g_propagate_error (error, local_error);
>-                g_free (std_err);
>-                return FALSE;
>-        }
>-
>-        if (WEXITSTATUS (status) != 0) {
>-                g_set_error (error,
>-                             G_SPAWN_ERROR,
>-                             G_SPAWN_ERROR_FAILED,
>-                             "%s returned an error (%d): %s",
>-                             argv[0], WEXITSTATUS(status), std_err);
>-                g_free (std_err);
>-                return FALSE;
>+        if (loginuid[0]) {
>+                setup_func = setup_loginuid;
>+                setup_data = loginuid;
>+        } else {
>+                setup_func = setup_data = NULL;
>         }
> 
>-        g_free (std_err);
>+        if (!g_spawn_sync (NULL, (gchar**)argv, NULL, 0, setup_func, setup_data, NULL, NULL, &status, error))
>+                goto out;
>+        if (!compat_check_exit_status (status, error))
>+                goto out;
> 
>-        return TRUE;
>+        ret = TRUE;
>+ out:
>+        return ret;
> }
> 
> gint
>-- 
>1.7.11.4

Actions: View | Splinter Review

Attachments on bug 55067: 67350 | 67406

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.