Re: [Qemu-devel] [PATCH] monitor: properly handle invalid fd/vhostfd from command line

[Luiz Capitulino <lcapitulino@redhat.com>]

On Tue, 28 Sep 2010 16:57:44 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Sep 28, 2010 at 11:53:43AM -0300, Luiz Capitulino wrote:
> > On Mon, 27 Sep 2010 15:52:44 +0800
> > Jason Wang <jasowang@redhat.com> wrote:
> > 
> > > monitor_get_fd() may also be used to parse fd or vhostfd from command line, so
> > > we need to check whether the pointer of mon is NULL to avoid segmentation fault
> > > when user pass invalid name of fd or vhostfd.
> > 
> > Invalid fdname is handled just fine, I have the impression this patch fixes
> > something else.
> > 
> > Could you elaborate on the real problem here and/or show to reproduce?
> 
> Try pasing fd= (no value) as a parameter, and see what happens.

Sorry for the delay on this one.

If I'm reading this code correctly, we have two kinds of fd passing being
handled by net_handle_fd_param(): fds passed via SCM_RIGHTS (handled by
the monitor) and -net fds, passed via the command-line.

In this case, we don't want the fd passed via SCM_RIGHTS, so
net_handle_fd_param() has to be fixed to check for !mon and also check
strtol()'s return:

diff --git a/net.c b/net.c
index 3d0fde7..6aaa653 100644
--- a/net.c
+++ b/net.c
@@ -739,9 +739,9 @@ int qemu_find_nic_model(NICInfo *nd, const char * const *models,
 
 int net_handle_fd_param(Monitor *mon, const char *param)
 {
-    if (!qemu_isdigit(param[0])) {
-        int fd;
+    int fd;
 
+    if (!qemu_isdigit(param[0]) && mon) {
         fd = monitor_get_fd(mon, param);
         if (fd == -1) {
             error_report("No file descriptor named %s found", param);
@@ -750,7 +750,13 @@ int net_handle_fd_param(Monitor *mon, const char *param)
 
         return fd;
     } else {
-        return strtol(param, NULL, 0);
+        char *endptr = NULL;
+
+        fd = strtol(param, &endptr, 10);
+        if (*endptr || (fd == 0 && param == endptr)) {
+            return -1;
+        }
+        return fd;
     }
 }

There's more: qemu doesn't exit when an invalid fd is passed:

"""
~/stuff/virt/ ./qemu-qmp -hda disks/test.img -enable-kvm -m 1G -snapshot -net tap,fd=40
qemu-qmp: -net tap,fd=40: TUNGETIFF ioctl() failed: Bad file descriptor
TUNSETOFFLOAD ioctl() failed: Bad file descriptor
Warning: vlan 0 with no nics
"""

And QEMU is up and running...


>
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > >  monitor.c |    4 ++++
> > >  1 files changed, 4 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/monitor.c b/monitor.c
> > > index e602480..5bb4ff0 100644
> > > --- a/monitor.c
> > > +++ b/monitor.c
> > > @@ -2345,6 +2345,10 @@ int monitor_get_fd(Monitor *mon, const char *fdname)
> > >  {
> > >      mon_fd_t *monfd;
> > >  
> > > +    if (mon == NULL) {
> > > +        return -1;
> > > +    }
> > > +
> > >      QLIST_FOREACH(monfd, &mon->fds, next) {
> > >          int fd;
> > >  
> > > 
> > > 
>