Skip to content

Commit

Permalink
gnome-base/gnome-keyring: add some important fixes from unreleased git
Browse files Browse the repository at this point in the history
May fix the intermittent issues with gnome-keyring sometimes not getting
set up upon login occassionally.
Plus a potential test fix for ssh tests that I hadn't hit myself yet.

Package-Manager: Portage-2.3.69, Repoman-2.3.12
Signed-off-by: Mart Raudsepp <[email protected]>
  • Loading branch information
leio committed Sep 23, 2019
1 parent d91ec7b commit b4a8425
Show file tree
Hide file tree
Showing 4 changed files with 255 additions and 0 deletions.
37 changes: 37 additions & 0 deletions gnome-base/gnome-keyring/files/3.31.91-race-fix1.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
From 8a948b3ac17f7d1b0ff31b0cf22e655054eb5c6b Mon Sep 17 00:00:00 2001
From: Benjamin Berg <[email protected]>
Date: Tue, 14 May 2019 17:36:56 +0200
Subject: [PATCH 1/2] dbus-environment: Log Setenv call failure after
initialization

When the GNOME session is already initialized at the point that Setenv
is called, then an error is returned. Hidding this error makes it hard
to understand why the environment was not setup if things failed.
---
daemon/dbus/gkd-dbus-environment.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/daemon/dbus/gkd-dbus-environment.c b/daemon/dbus/gkd-dbus-environment.c
index 93e2b878..051de953 100644
--- a/daemon/dbus/gkd-dbus-environment.c
+++ b/daemon/dbus/gkd-dbus-environment.c
@@ -49,15 +49,11 @@ on_setenv_reply (GObject *source,
res = g_dbus_connection_call_finish (G_DBUS_CONNECTION (source), result, &error);

if (error != NULL) {
- gchar *dbus_error;
- dbus_error = g_dbus_error_get_remote_error (error);
- if (g_error_matches (error, G_DBUS_ERROR, G_DBUS_ERROR_SERVICE_UNKNOWN) ||
- g_strcmp0 (dbus_error, "org.gnome.SessionManager.NotInInitialization") == 0)
+ if (g_error_matches (error, G_DBUS_ERROR, G_DBUS_ERROR_SERVICE_UNKNOWN))
g_debug ("couldn't set environment variable in session: %s", error->message);
else
g_message ("couldn't set environment variable in session: %s", error->message);
g_error_free (error);
- g_free (dbus_error);
}

g_clear_pointer (&res, g_variant_unref);
--
2.20.1

104 changes: 104 additions & 0 deletions gnome-base/gnome-keyring/files/3.31.91-race-fix2.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
From 5d088356a9473c06564bd2cef18ca370437a17bc Mon Sep 17 00:00:00 2001
From: Benjamin Berg <[email protected]>
Date: Tue, 14 May 2019 17:42:29 +0200
Subject: [PATCH 2/2] dbus-environment: Make Setenv request synchronuous

Currently there is a potential race condition where the Setenv request
races further session startup. i.e. the clients that are started with
--start on login may quit before the Setenv DBus call is delivered. This
opens a theoretical race condition where gnome-session is already past
the initialization phase when it serves the Setenv request.
---
daemon/dbus/gkd-dbus-environment.c | 62 +++++++++++++++---------------
1 file changed, 30 insertions(+), 32 deletions(-)

diff --git a/daemon/dbus/gkd-dbus-environment.c b/daemon/dbus/gkd-dbus-environment.c
index 051de953..acf398b9 100644
--- a/daemon/dbus/gkd-dbus-environment.c
+++ b/daemon/dbus/gkd-dbus-environment.c
@@ -38,32 +38,13 @@ gkd_dbus_environment_cleanup (GDBusConnection *conn)
/* Nothing to do here */
}

-static void
-on_setenv_reply (GObject *source,
- GAsyncResult *result,
- gpointer user_data)
-{
- GError *error = NULL;
- GVariant *res;
-
- res = g_dbus_connection_call_finish (G_DBUS_CONNECTION (source), result, &error);
-
- if (error != NULL) {
- if (g_error_matches (error, G_DBUS_ERROR, G_DBUS_ERROR_SERVICE_UNKNOWN))
- g_debug ("couldn't set environment variable in session: %s", error->message);
- else
- g_message ("couldn't set environment variable in session: %s", error->message);
- g_error_free (error);
- }
-
- g_clear_pointer (&res, g_variant_unref);
-}
-
static void
setenv_request (GDBusConnection *conn, const gchar *env)
{
const gchar *value;
gchar *name;
+ GVariant *res;
+ GError *error = NULL;

/* Find the value part of the environment variable */
value = strchr (env, '=');
@@ -73,19 +54,36 @@ setenv_request (GDBusConnection *conn, const gchar *env)
name = g_strndup (env, value - env);
++value;

- g_dbus_connection_call (conn,
- SERVICE_SESSION_MANAGER,
- PATH_SESSION_MANAGER,
- IFACE_SESSION_MANAGER,
- "Setenv",
- g_variant_new ("(ss)",
- name,
- value),
- NULL, G_DBUS_CALL_FLAGS_NONE,
- -1, NULL,
- on_setenv_reply, NULL);
+ /* Note: This call does not neccessarily need to be a sync call. However
+ * under certain conditions the process will quit immediately
+ * after emitting the call. This ensures that we wait long enough
+ * for the message to be sent out (could also be done using
+ * g_dbus_connection_flush() in the exit handler when called with
+ * --start) and also ensures that gnome-session has processed the
+ * DBus message before possibly thinking that the startup of
+ * gnome-keyring has finished and continuing with forking the
+ * shell. */
+ res = g_dbus_connection_call_sync (conn,
+ SERVICE_SESSION_MANAGER,
+ PATH_SESSION_MANAGER,
+ IFACE_SESSION_MANAGER,
+ "Setenv",
+ g_variant_new ("(ss)",
+ name,
+ value),
+ NULL, G_DBUS_CALL_FLAGS_NONE,
+ -1, NULL, &error);
+
+ if (error != NULL) {
+ if (g_error_matches (error, G_DBUS_ERROR, G_DBUS_ERROR_SERVICE_UNKNOWN))
+ g_debug ("couldn't set environment variable in session: %s", error->message);
+ else
+ g_message ("couldn't set environment variable in session: %s", error->message);
+ g_error_free (error);
+ }

g_free (name);
+ g_clear_pointer (&res, g_variant_unref);
}

static void
--
2.20.1

112 changes: 112 additions & 0 deletions gnome-base/gnome-keyring/files/3.31.91-ssh-tests-fix.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
From 91bc9368ca2eedef0dec3f5aa81f641ced07a9b6 Mon Sep 17 00:00:00 2001
From: Simon McVittie <[email protected]>
Date: Sat, 9 Mar 2019 17:56:55 +0000
Subject: [PATCH] test-gkd-ssh-agent-service: Avoid race condition with server
thread

These tests create a server thread in setup() and join it in teardown(),
but there are various race conditions between them that can cause the
test to hang. These are particularly reproducible when building on a
single-CPU machine or VM, and particularly in the startup_shutdown
test (which doesn't do anything, so it runs teardown() immediately
after setup()).

It's possible to get this preemption pattern:

___ Main thread ___ ___ Server thread ___
g_thread_new() (starts)
g_cond_wait() (blocks)
...
g_cond_signal()
(gets preempted here)
exit setup()
enter teardown()
g_main_loop_quit()
g_main_loop_run()

which means g_main_loop_run() will never terminate, because it wasn't
running yet when the main thread told the GMainLoop to quit, and the
main thread won't tell it to quit again.

One way to solve this would be for the server thread to signal
test->cond from an idle callback instead of directly from
server_thread(), to guarantee that the GMainLoop is already running.
However, it seems easier to reason about if we avoid GMainLoop and
iterate the main context directly.

Signed-off-by: Simon McVittie <[email protected]>
Bug-Debian: https://bugs.debian.org/909416
---
daemon/ssh-agent/test-gkd-ssh-agent-service.c | 23 +++++++++----------
1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/daemon/ssh-agent/test-gkd-ssh-agent-service.c b/daemon/ssh-agent/test-gkd-ssh-agent-service.c
index 9a9ead99..5c7a6179 100644
--- a/daemon/ssh-agent/test-gkd-ssh-agent-service.c
+++ b/daemon/ssh-agent/test-gkd-ssh-agent-service.c
@@ -38,7 +38,8 @@ typedef struct {
EggBuffer req;
EggBuffer resp;
GkdSshAgentService *service;
- GMainLoop *loop;
+ GMainContext *server_thread_context;
+ volatile gint server_thread_stop;
GSocketConnection *connection;
GThread *thread;
GMutex lock;
@@ -49,13 +50,9 @@ static gpointer
server_thread (gpointer data)
{
Test *test = data;
- GMainContext *context;
gboolean ret;

- context = g_main_context_new ();
- test->loop = g_main_loop_new (context, FALSE);
-
- g_main_context_push_thread_default (context);
+ g_main_context_push_thread_default (test->server_thread_context);

ret = gkd_ssh_agent_service_start (test->service);
g_assert_true (ret);
@@ -64,12 +61,10 @@ server_thread (gpointer data)
g_cond_signal (&test->cond);
g_mutex_unlock (&test->lock);

- g_main_loop_run (test->loop);
+ while (g_atomic_int_get (&test->server_thread_stop) == 0)
+ g_main_context_iteration (test->server_thread_context, TRUE);

- g_main_context_pop_thread_default (context);
-
- g_main_context_unref (context);
- g_main_loop_unref (test->loop);
+ g_main_context_pop_thread_default (test->server_thread_context);

return NULL;
}
@@ -139,6 +134,7 @@ setup (Test *test, gconstpointer unused)

g_mutex_init (&test->lock);
g_cond_init (&test->cond);
+ test->server_thread_context = g_main_context_new ();

test->thread = g_thread_new ("ssh-agent", server_thread, test);

@@ -151,9 +147,12 @@ setup (Test *test, gconstpointer unused)
static void
teardown (Test *test, gconstpointer unused)
{
- g_main_loop_quit (test->loop);
+ g_atomic_int_set (&test->server_thread_stop, 1);
+ g_main_context_wakeup (test->server_thread_context);
g_thread_join (test->thread);

+ g_main_context_unref (test->server_thread_context);
+
g_clear_object (&test->connection);

gkd_ssh_agent_service_stop (test->service);
--
2.20.1

Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ DEPEND="${RDEPEND}
PDEPEND="app-crypt/pinentry[gnome-keyring]" #570512

PATCHES=(
"${FILESDIR}"/${PV}-race-fix{1,2}.patch # fix race issues on start, where sometimes keyring doesn't work after login; from origin/master
"${FILESDIR}"/${PV}-ssh-tests-fix.patch
"${FILESDIR}"/${PV}-fix-musl.patch
)

Expand Down

0 comments on commit b4a8425

Please sign in to comment.