Re: [PATCH] virt-admin: Fix segfault when libvirtd dies

On Tue, Mar 19, 2024 at 14:34:08 +0100, Claudio Fontana wrote: > On 3/19/24 12:02, Adam Julis wrote: >> vshAdmCatchDisconnect requires non-NULL structure vshControl for >> getting connection name (stored at opaque), but >> virAdmConnectRegisterCloseCallback at vshAdmConnect called it >> with NULL. >> >> Signed-off-by: Adam Julis <ajulis(a)redhat.com&gt; --- >> tools/virt-admin.c | 2 +- 1 file changed, 1 insertion(+), 1 >> deletion(-) >> >> diff --git a/tools/virt-admin.c b/tools/virt-admin.c index >> 37bc6fe4f0..0766032e4a 100644 --- a/tools/virt-admin.c +++ >> b/tools/virt-admin.c @@ -112,7 +112,7 @@ vshAdmConnect(vshControl >> *ctl, unsigned int flags) return -1; } else { if >> (virAdmConnectRegisterCloseCallback(priv->conn, >> vshAdmCatchDisconnect, - >> NULL, NULL) < 0) + >> ctl, NULL) < 0) vshError(ctl, "%s", _("Unable to register >> disconnect callback")); >> >> if (priv->wantReconnect) > > Hi, > > if that is the case I would then expect that > virAdmConnectRegisterCloseCallback() needs to also be updated > with: > > +virCheckNonNullArgGoto(opaque, error); > > or something like that. Is there a reason why it isn't? We better > catch the error early if the API is used wrongly. Well, vshAdmCatchDisconnect requires opaque to be non-NULL, but other callbacks registered with virAdmConnectRegisterCloseCallback may not need any opaque data.