From 39ad3f1c092b5dffcbb4b1d12eb9ca407f010a3c Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Sun, 7 Mar 2021 16:42:35 +0100 Subject: 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. --- src/shared/varlink.c | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-) (limited to 'src') diff --git a/src/shared/varlink.c b/src/shared/varlink.c index 31128e02e0..6ed72075ba 100644 --- 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) { -- cgit v1.2.3