[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] monitor: properly handle invalid fd/vhostfd fro
From: |
Luiz Capitulino |
Subject: |
Re: [Qemu-devel] [PATCH] monitor: properly handle invalid fd/vhostfd from command line |
Date: |
Wed, 13 Oct 2010 10:35:15 -0300 |
On Tue, 12 Oct 2010 14:32:25 +0800
jason wang <address@hidden> wrote:
> On 10/09/2010 01:40 AM, Luiz Capitulino wrote:
> > On Tue, 28 Sep 2010 16:57:44 +0200
> > "Michael S. Tsirkin"<address@hidden> 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<address@hidden> 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<address@hidden>
> >>>> ---
> >>>> 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;
> >>>>
> >>>>
> >>>>
> >>>>
> >>
> >
> >
>