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.

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)) {
+                operation_result(o, r, &error);
+                return 1;
+        }
 
-                case HOME_UNFIXATED:
-                case HOME_ABSENT:
-                case HOME_INACTIVE:
-                case HOME_DIRTY:
-                        r = 1; /* done */
-                        break;
+        switch (home_get_state(h)) {
 
-                case HOME_LOCKED:
-                        r = sd_bus_error_setf(&error, BUS_ERROR_HOME_LOCKED, "Home %s is currently locked.", h->user_name);
-                        break;
+        case HOME_UNFIXATED:
+        case HOME_ABSENT:
+        case HOME_INACTIVE:
+        case HOME_DIRTY:
+                r = 1; /* done */
+                break;
 
-                case HOME_ACTIVE:
-                case HOME_LINGERING:
-                        r = home_deactivate_internal(h, false, &error);
-                        break;
+        case HOME_LOCKED:
+                r = sd_bus_error_setf(&error, BUS_ERROR_HOME_LOCKED, "Home %s is currently locked.", h->user_name);
+                break;
 
-                default:
-                        /* All other cases means we are currently executing an operation, which means the job remains
-                         * pending. */
-                        return 0;
-                }
+        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);

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 += [

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);

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