Re: [PATCH v3 1/2] net: tap: check if the file descriptor is valid before using it

[Laurent Vivier]

On 02/07/2020 10:00, Philippe Mathieu-Daudé wrote:
> On 7/1/20 9:39 PM, Laurent Vivier wrote:
>> qemu_set_nonblock() checks that the file descriptor can be used and, if
>> not, crashes QEMU. An assert() is used for that. The use of assert() is
>> used to detect programming error and the coredump will allow to debug
>> the problem.
>>
>> But in the case of the tap device, this assert() can be triggered by
>> a misconfiguration by the user. At startup, it's not a real problem, but it
>> can also happen during the hot-plug of a new device, and here it's a
>> problem because we can crash a perfectly healthy system.
>>
>> For instance:
>>  # ip link add link virbr0 name macvtap0 type macvtap mode bridge
>>  # ip link set macvtap0 up
>>  # TAP=/dev/tap$(ip -o link show macvtap0 | cut -d: -f1)
>>  # qemu-system-x86_64 -machine q35 -device 
>> pcie-root-port,id=pcie-root-port-0 -monitor stdio 9<> $TAP
>>  (qemu) netdev_add type=tap,id=hostnet0,vhost=on,fd=9
>>  (qemu) device_add 
>> driver=virtio-net-pci,netdev=hostnet0,id=net0,bus=pcie-root-port-0
>>  (qemu) device_del net0
>>  (qemu) netdev_del hostnet0
>>  (qemu) netdev_add type=tap,id=hostnet1,vhost=on,fd=9
>>  qemu-system-x86_64: .../util/oslib-posix.c:247: qemu_set_nonblock: 
>> Assertion `f != -1' failed.
>>  Aborted (core dumped)
>>
>> To avoid that, add a function, qemu_try_set_nonblock(), that allows to 
>> report the
>> problem without crashing.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>  include/qemu/sockets.h |  1 +
>>  net/tap.c              | 16 +++++++++---
>>  util/oslib-posix.c     | 26 +++++++++++++------
>>  util/oslib-win32.c     | 57 ++++++++++++++++++++++++------------------
>>  4 files changed, 64 insertions(+), 36 deletions(-)
>>
>> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
>> index 57cd049d6edd..7d1f8135767d 100644
>> --- a/include/qemu/sockets.h
>> +++ b/include/qemu/sockets.h
>> @@ -18,6 +18,7 @@ int qemu_accept(int s, struct sockaddr *addr, socklen_t 
>> *addrlen);
>>  int socket_set_cork(int fd, int v);
>>  int socket_set_nodelay(int fd);
>>  void qemu_set_block(int fd);
>> +int qemu_try_set_nonblock(int fd);
>>  void qemu_set_nonblock(int fd);
>>  int socket_set_fast_reuse(int fd);
>>  
>> diff --git a/net/tap.c b/net/tap.c
>> index 6207f61f84ab..fb04c9044ce2 100644
>> --- a/net/tap.c
>> +++ b/net/tap.c
>> @@ -766,6 +766,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
>>      Error *err = NULL;
>>      const char *vhostfdname;
>>      char ifname[128];
>> +    int ret = 0;
> 
> No need to zero-initialize.
> 
> Otherwise:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 

I would agree with you but gcc doesn't:

net/tap.c:769:9: error: ‘ret’ may be used uninitialized in this function
[-Werror=maybe-uninitialized]
  769 |     int ret;
      |         ^~~

if nfds == 0, we don't enter in the loop and ret is never set.

Thanks,
Laurent