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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Tue, 12 Oct 2010 14:32:25 +0800
jason wang <jasowang@redhat.com> wrote:

> On 10/09/2010 01:40 AM, Luiz Capitulino wrote:
> > 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:
> >    
> Yes, it's better to check strtol()'s return which was missed in the 
> original patch.
> > 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;
> >       }
> >   }
> >    
> This looks good for me.

Would you mind taking care of this one? I mean, I didn't even test this much :-)

So, you could test it more and submit it again as a proper patch.

Thanks.

> > 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...//
> >    
> Anthony have proposed a patch for this.
> //
> >
> >    
> >>      
> >>>> 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;
> >>>>
> >>>>
> >>>>
> >>>>          
> >>      
> >
> >    
>