fix various issues

[libvirt PATCH 05/11] qemu_command: fix FD usage in qemuBuildInterfaceCommandLine

Posted by Pavel Hrdina 5 years, 2 months ago

If virCommandPassFD() is called with VIR_COMMAND_PASS_FD_CLOSE_PARENT
the passed FD is closed and should not be used after that call.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 src/qemu/qemu_command.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 0eec35da16..0580feb475 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8142,7 +8142,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
         g_autofree char *fdset = NULL;
         g_autofree char *addfdarg = NULL;
 
-        virCommandPassFD(cmd, vdpafd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
+        virCommandPassFD(cmd, vdpafd, 0);
         fdset = qemuVirCommandGetFDSet(cmd, vdpafd);
         if (!fdset)
             goto cleanup;
-- 
2.26.2

Re: [libvirt PATCH 05/11] qemu_command: fix FD usage in qemuBuildInterfaceCommandLine

Posted by Peter Krempa 5 years, 2 months ago

On Mon, Nov 16, 2020 at 16:38:52 +0100, Pavel Hrdina wrote:
> If virCommandPassFD() is called with VIR_COMMAND_PASS_FD_CLOSE_PARENT
> the passed FD is closed and should not be used after that call.

This description doesn't seem to make sense. The filedescriptor itself
isn't really used in the caller. The only thing that is used is the
number of the filedescriptor which is used to format the string  being
passed to qemu which is should be correct.

> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  src/qemu/qemu_command.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 0eec35da16..0580feb475 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -8142,7 +8142,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
>          g_autofree char *fdset = NULL;
>          g_autofree char *addfdarg = NULL;
>  
> -        virCommandPassFD(cmd, vdpafd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
> +        virCommandPassFD(cmd, vdpafd, 0);
>          fdset = qemuVirCommandGetFDSet(cmd, vdpafd);

A proper fix will be to move 'vdpafd = -1;' here or copy it's value
somewhere to prevent the double close ...

>          if (!fdset)
>              goto cleanup;

... on this condition.


> -- 
> 2.26.2
>

Re: [libvirt PATCH 05/11] qemu_command: fix FD usage in qemuBuildInterfaceCommandLine

Posted by Pavel Hrdina 5 years, 2 months ago

On Mon, Nov 16, 2020 at 05:18:51PM +0100, Peter Krempa wrote:
> On Mon, Nov 16, 2020 at 16:38:52 +0100, Pavel Hrdina wrote:
> > If virCommandPassFD() is called with VIR_COMMAND_PASS_FD_CLOSE_PARENT
> > the passed FD is closed and should not be used after that call.
> 
> This description doesn't seem to make sense. The filedescriptor itself
> isn't really used in the caller. The only thing that is used is the
> number of the filedescriptor which is used to format the string  being
> passed to qemu which is should be correct.

Right, the description was not accurate. By "should not be used" I meant
that we close it again in the cleanup section.

> > 
> > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > ---
> >  src/qemu/qemu_command.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 0eec35da16..0580feb475 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -8142,7 +8142,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
> >          g_autofree char *fdset = NULL;
> >          g_autofree char *addfdarg = NULL;
> >  
> > -        virCommandPassFD(cmd, vdpafd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
> > +        virCommandPassFD(cmd, vdpafd, 0);
> >          fdset = qemuVirCommandGetFDSet(cmd, vdpafd);
> 
> A proper fix will be to move 'vdpafd = -1;' here or copy it's value
> somewhere to prevent the double close ...

Moving the 'vdpafd = -1;' here would introduce a different bug because
'vdpafd' is used after that condition as well. So I guess the only
correct solution is to copy the value to a different variable.

Thanks for the review, I'll send a v2.

Pavel