| Message ID | 20101008144002.245d55e1@doriath |
|---|---|
| State | New |
| Headers | show |
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. > 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; >>>> >>>> >>>> >>>> >> > >
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; > >>>> > >>>> > >>>> > >>>> > >> > > > > >
----- "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; > > >>>> > > >>>> > > >>>> > > >>>> > > >> > > > > > > > >
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; } }