Re: [Qemu-devel] [PATCH] check return value of fcntl() to detect invalid fd

[Jason Wang <jasowang@redhat.com>]

On 12/22/2014 01:28 PM, Amos Kong wrote:
> On Mon, Dec 22, 2014 at 11:48:29AM +0800, Jason Wang wrote:
>> On 12/19/2014 09:25 PM, Amos Kong wrote:
>>> Passing some invalid fds in QEMU commandline, the fds don't exist.
>>> QEMU will get error "TUNGETIFF ioctl() failed: Bad file descriptor",
>>> and coredump in setting queues.
>>>
>>> This patch checked return value of first operate to fd, QEMU will
>>> report error and exit without coredump. It's effected for both netdev
>>> fds and vhost_net fds.
>>>
>>> Signed-off-by: Amos Kong <akong@redhat.com>
>>> ---
>>>  net/tap.c | 16 +++++++++++++---
>>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/tap.c b/net/tap.c
>>> index bde6b58..039280a 100644
>>> --- a/net/tap.c
>>> +++ b/net/tap.c
>>> @@ -688,7 +688,7 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>>>                   NetClientState *peer)
>>>  {
>>>      const NetdevTapOptions *tap;
>>> -    int fd, vnet_hdr = 0, i = 0, queues;
>>> +    int fd, vnet_hdr = 0, i = 0, queues, ret;
>>>      /* for the no-fd, no-helper case */
>>>      const char *script = NULL; /* suppress wrong "uninit'd use" gcc warning */
>>>      const char *downscript = NULL;
>>> @@ -722,7 +722,12 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>>>              return -1;
>>>          }
>>>  
>>> -        fcntl(fd, F_SETFL, O_NONBLOCK);
>>> +        ret = fcntl(fd, F_SETFL, O_NONBLOCK);
>>> +        if (ret < 0) {
>>> +            error_report("Fail to set file status to nonblock, "
>>> +                         "%s", strerror(-ret));
>>> +            return -1;
>>> +        }
>> This may not work. There may be still some kinds of fd can pass this but
>> still fail at TUNGETIFF or other tun ioctls.
> Early catching the error is better. This only help to check if the fd
> exists.

If you just want to check the existence. Why don't you do it in
monitor_handle_fd_param() to let other case to benefit also? And
probably F_GETFL is better in this case.

But doing this does not solve the issue you mention in the commit log.
Even if fd exists, if it was not a tap fd, qemu may still abort.