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

[Jason Wang <jasowang@redhat.com>]

----- "Luiz Capitulino" <lcapitulino@redhat.com> wrote:

> 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.

Sure, would post this later.

> 
> > > 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;
> > >>>>
> > >>>>
> > >>>>
> > >>>>          
> > >>      
> > >
> > >    
> >