virt-admin: Fix segfault when libvirtd dies

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

Posted by Adam Julis 1 month, 1 week ago

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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

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

Posted by Ján Tomko 1 month, 1 week ago

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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

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

Posted by Claudio Fontana 1 month, 1 week ago

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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

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

Posted by Jiri Denemark 1 month, 1 week ago

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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

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

Posted by Claudio Fontana 1 month, 1 week ago

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>
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org