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

[jason wang <jasowang@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3243 bytes --]

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


[-- Attachment #2: Type: text/html, Size: 4389 bytes --]