[PATCH] virt-admin: Fix segfault when libvirtd dies
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@redhat.com> --- 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) -- 2.43.2
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@redhat.com> --- 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. Claudio
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@redhat.com> --- 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. Jirka
On 3/19/24 15:58, Jiri Denemark wrote:
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@redhat.com> --- 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.
Fair enough. Reviewed-by: Claudio Fontana <cfontana@suse.de>
On a Tuesday in 2024, 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@redhat.com> --- tools/virt-admin.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (4)
-
Adam Julis -
Claudio Fontana -
Jiri Denemark -
Ján Tomko