homed: Release(): fix assertion failure

This fixes a race condition crash in homed that would happen in the
following sequence of events:

1. Client 1 takes a ref on the home area
2. Client 1 calls some method via dbus
3. Client 2 calls Release()

In homed, the Release() would check if a ref is still held (in this
case: yes it is) and returns an error. Except that is done through a
code-path that asserts that no operations are ongoing. In this case,
it's valid to have an ongoing operation, and so the assertion fails
causing homed to crash.

diff --git a/src/home/homed-home.c b/src/home/homed-home.c
index 447e8c597cadd12ca9ce33b75157d1355432fce3..757881c2e6021719199c089c83f678d5a2648f19 100644 (file)
--- a/src/home/homed-home.c
+++ b/src/home/homed-home.c
@@ -2810,7 +2810,8 @@ static int home_dispatch_acquire(Home *h, Operation *o) {
         case HOME_ABSENT:
                 r = sd_bus_error_setf(&error, BUS_ERROR_HOME_ABSENT,
                                       "Home %s is currently missing or not plugged in.", h->user_name);
-                goto check;
+                operation_result(o, r, &error);
+                return 1;
 
         case HOME_INACTIVE:
         case HOME_DIRTY:
@@ -2841,7 +2842,6 @@ static int home_dispatch_acquire(Home *h, Operation *o) {
         if (r >= 0)
                 r = call(h, o->secret, for_state, &error);
 
- check:
         if (r != 0) /* failure or completed */
                 operation_result(o, r, &error);
         else /* ongoing */
@@ -2871,33 +2871,35 @@ static int home_dispatch_release(Home *h, Operation *o) {
         assert(o);
         assert(o->type == OPERATION_RELEASE);
 
-        if (home_is_referenced(h))
+        if (home_is_referenced(h)) {
                 /* If there's now a reference again, then let's abort the release attempt */
                 r = sd_bus_error_setf(&error, BUS_ERROR_HOME_BUSY, "Home %s is currently referenced.", h->user_name);
-        else {
-                switch (home_get_state(h)) {
-
-                case HOME_UNFIXATED:
-                case HOME_ABSENT:
-                case HOME_INACTIVE:
-                case HOME_DIRTY:
-                        r = 1; /* done */
-                        break;
-
-                case HOME_LOCKED:
-                        r = sd_bus_error_setf(&error, BUS_ERROR_HOME_LOCKED, "Home %s is currently locked.", h->user_name);
-                        break;
-
-                case HOME_ACTIVE:
-                case HOME_LINGERING:
-                        r = home_deactivate_internal(h, false, &error);
-                        break;
-
-                default:
-                        /* All other cases means we are currently executing an operation, which means the job remains
-                         * pending. */
-                        return 0;
-                }
+                operation_result(o, r, &error);
+                return 1;
+        }
+
+        switch (home_get_state(h)) {
+
+        case HOME_UNFIXATED:
+        case HOME_ABSENT:
+        case HOME_INACTIVE:
+        case HOME_DIRTY:
+                r = 1; /* done */
+                break;
+
+        case HOME_LOCKED:
+                r = sd_bus_error_setf(&error, BUS_ERROR_HOME_LOCKED, "Home %s is currently locked.", h->user_name);
+                break;
+
+        case HOME_ACTIVE:
+        case HOME_LINGERING:
+                r = home_deactivate_internal(h, false, &error);
+                break;
+
+        default:
+                /* All other cases means we are currently executing an operation, which means the job remains
+                 * pending. */
+                return 0;
         }
 
         assert(!h->current_operation);

diff --git a/src/home/meson.build b/src/home/meson.build
index a1e912f6a0c629778ed20bd348bd221a073aa244..f573c5fb15af69ac57f25f45373293a75d768750 100644 (file)
--- a/src/home/meson.build
+++ b/src/home/meson.build
@@ -106,6 +106,11 @@ executables += [
                         threads,
                 ],
         },
+        test_template + {
+                'sources' : files('test-homed-regression-31896.c'),
+                'conditions' : ['ENABLE_HOMED'],
+                'type' : 'manual',
+        },
 ]
 
 modules += [

diff --git a/src/home/test-homed-regression-31896.c b/src/home/test-homed-regression-31896.c
new file mode 100644 (file)
index 0000000..1530a2f
--- /dev/null
+++ b/src/home/test-homed-regression-31896.c
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+
+#include "bus-locator.h"
+#include "main-func.h"
+#include "tests.h"
+
+static int run(int argc, char **argv) {
+        _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL;
+        _cleanup_(sd_bus_message_unrefp) sd_bus_message *ref = NULL;
+        _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL;
+        const char *username = NULL;
+
+        /* This is a regression test for the following bug:
+         * https://github.com/systemd/systemd/pull/31896
+         * It is run as part of TEST-46-HOMED
+         */
+
+        test_setup_logging(LOG_DEBUG);
+        assert_se(sd_bus_open_system(&bus) >= 0);
+
+        assert_se(argc == 2);
+        username = argv[1];
+
+        assert_se(bus_call_method(bus, bus_home_mgr, "RefHomeUnrestricted", NULL, &ref, "sb", username, true) >= 0);
+
+        assert_se(bus_call_method_async(bus, NULL, bus_home_mgr, "AuthenticateHome", NULL, NULL, "ss", username, "{}") >= 0);
+        assert_se(sd_bus_flush(bus) >= 0);
+
+        (void) bus_call_method(bus, bus_home_mgr, "ReleaseHome", &error, NULL, "s", username);
+        assert_se(!sd_bus_error_has_name(&error, SD_BUS_ERROR_NO_REPLY)); /* Make sure we didn't crash */
+
+        return 0;
+}
+
+DEFINE_MAIN_FUNCTION(run);

diff --git a/test/units/testsuite-46.sh b/test/units/testsuite-46.sh
index b20b39d762b97fce1ed9c7999982838e3430a073..7a2bee817956f6b5eb9228ba0eb74798f56ab3db 100755 (executable)
--- a/test/units/testsuite-46.sh
+++ b/test/units/testsuite-46.sh
@@ -207,6 +207,10 @@ PASSWORD=xEhErW0ndafV4s homectl with test-user -- rm /home/test-user/xyz
 PASSWORD=xEhErW0ndafV4s homectl with test-user -- test ! -f /home/test-user/xyz
 (! PASSWORD=xEhErW0ndafV4s homectl with test-user -- test -f /home/test-user/xyz)
 
+# Regression tests
+wait_for_state test-user inactive
+/usr/lib/systemd/tests/unit-tests/manual/test-homed-regression-31896 test-user
+
 wait_for_state test-user inactive
 homectl remove test-user