On Tue, 12 Oct 2010 14:32:25 +0800 jason wang <jasow...@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"<m...@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<jasow...@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<jasow...@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; > >>>> > >>>> > >>>> > >>>> > >> > > > > >