varlink: avoid using dangling ref in varlink_close_unref()

Fixes #18025, https://bugzilla.redhat.com/show_bug.cgi?id=1931034.

We drop the reference stored in Manager.managed_oom_varlink_request in two code paths:
vl_disconnect() which is installed as a disconnect callback, and in manager_varlink_done().
But we also make a disconnect from manager_varlink_done(). So we end up with the following
call stack:

(gdb) bt
 0  vl_disconnect (s=0x112c7b0, link=0xea0070, userdata=0xe9bcc0) at ../src/core/core-varlink.c:414
 1  0x00007f1366e9d5ac in varlink_detach_server (v=0xea0070) at ../src/shared/varlink.c:1210
 2  0x00007f1366e9d664 in varlink_close (v=0xea0070) at ../src/shared/varlink.c:1228
 3  0x00007f1366e9d6b5 in varlink_close_unref (v=0xea0070) at ../src/shared/varlink.c:1240
 4  0x0000000000524629 in manager_varlink_done (m=0xe9bcc0) at ../src/core/core-varlink.c:479
 5  0x000000000048ef7b in manager_free (m=0xe9bcc0) at ../src/core/manager.c:1357
 6  0x000000000042602c in main (argc=5, argv=0x7fff439c43d8) at ../src/core/main.c:2909

When we enter vl_disconnect(), m->managed_oom_varlink_request.n_ref==1.
When we exit from vl_discconect(), m->managed_oom_varlink_request==NULL. But
varlink_close_unref() has a copy of the pointer in *v. When we continue executing
varlink_close_unref(), this pointer is dangling, and the call to varlink_unref()
is done with an invalid pointer.

diff --git a/src/shared/varlink.c b/src/shared/varlink.c
index 31128e02e0606f200d16ab91533137da0e6c6f80..6ed72075ba5beda6ac98bb2dbcaad0f96c19fcd4 100644 (file)
--- a/src/shared/varlink.c
+++ b/src/shared/varlink.c
@@ -1206,8 +1206,9 @@ 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 (dangling) ref
-         * which would destroy us before we can call varlink_clear() */
+        /* 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(). */
         varlink_ref(v);
         varlink_detach_server(v);
         varlink_clear(v);
@@ -1220,17 +1221,33 @@ Varlink* varlink_close_unref(Varlink *v) {
         if (!v)
                 return NULL;
 
-        (void) varlink_close(v);
+        /* 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 */
         return varlink_unref(v);
 }
 
 Varlink* varlink_flush_close_unref(Varlink *v) {
-        if (!v)
-                return NULL;
+        if (v)
+                varlink_flush(v);
 
-        (void) varlink_flush(v);
-        (void) varlink_close(v);
-        return varlink_unref(v);
+        return varlink_close_unref(v);
 }
 
 static int varlink_enqueue_json(Varlink *v, JsonVariant *m) {