Merge pull request #18986 from poettering/oomd-varlink-fix

varlink ref fix

2 files changed, 11 insertions, 26 deletions

diff --git a/src/core/core-varlink.c b/src/core/core-varlink.c
index b3df8cd893..b7afb87d50 100644
--- a/src/core/core-varlink.c
+++ b/src/core/core-varlink.c
@@ -475,8 +475,11 @@ int manager_varlink_init(Manager *m) {
 void manager_varlink_done(Manager *m) {
         assert(m);
 
-        /* Send the final message if we still have a subscribe request open. */
-        m->managed_oom_varlink_request = varlink_close_unref(m->managed_oom_varlink_request);
+        /* Explicitly close the varlink connection to oomd. Note we first take the varlink connection out of
+         * the manager, and only then disconnect it — in two steps – so that we don't end up accidentally
+         * unreffing it twice. After all, closing the connection might cause the disconnect handler we
+         * installed (vl_disconnect() above) to be called, where we will unref it too. */
+        varlink_close_unref(TAKE_PTR(m->managed_oom_varlink_request));
 
         m->varlink_server = varlink_server_unref(m->varlink_server);
 }
diff --git a/src/shared/varlink.c b/src/shared/varlink.c
index a271082ac3..daac6fd1b0 100644
--- a/src/shared/varlink.c
+++ b/src/shared/varlink.c
@@ -1210,9 +1210,8 @@ int varlink_close(Varlink *v) {
 
         varlink_set_state(v, VARLINK_DISCONNECTED);
 
-        /* Let's take a reference first, since varlink_detach_server() might drop the final ref from the
-         * disconnect callback, which would invalidate the pointer we are holding before we can call
-         * varlink_clear(). */
+        /* Let's take a reference first, since varlink_detach_server() might drop the final (dangling) ref
+         * which would destroy us before we can call varlink_clear() */
         varlink_ref(v);
         varlink_detach_server(v);
         varlink_clear(v);
@@ -1225,32 +1224,15 @@ Varlink* varlink_close_unref(Varlink *v) {
         if (!v)
                 return NULL;
 
-        /* A reference is given to us to be destroyed. But when calling varlink_close(), a callback might
-         * also drop a reference. We allow this, and will hold a temporary reference to the object to make
-         * sure that the object still exists when control returns to us. If there's just one reference
-         * remaining after varlink_close(), even though there were at least two right before, we'll handle
-         * that gracefully instead of crashing.
-         *
-         * In other words, this call drops the donated reference, but if the internal call to varlink_close()
-         * dropped a reference to, we don't drop the reference afain. This allows the caller to say:
-         *     global_object->varlink = varlink_close_unref(global_object->varlink);
-         * even though there is some callback which has access to global_object and may drop the reference
-         * stored in global_object->varlink. Without this step, the same code would have to be written as:
-         *     Varlink *t = TAKE_PTR(global_object->varlink);
-         *     varlink_close_unref(t);
-         */
-                                   /* n_ref >= 1 */
-        varlink_ref(v);            /* n_ref >= 2 */
-        varlink_close(v);          /* n_ref >= 1 */
-        if (v->n_ref > 1)
-                v->n_ref--;        /* n_ref >= 1 */
+        (void) varlink_close(v);
         return varlink_unref(v);
 }
 
 Varlink* varlink_flush_close_unref(Varlink *v) {
-        if (v)
-                varlink_flush(v);
+        if (!v)
+                return NULL;
 
+        (void) varlink_flush(v);
         return varlink_close_unref(v);
 }