Hi, Referring to Patrick suggestion I'll prepare patch with (fd >= 0) condition.
Thanks, Kuba > -----Original Message----- > From: Patrick MacArthur [mailto:patr...@patrickmacarthur.net] > Sent: Thursday, September 21, 2017 04:28 > To: Burakov, Anatoly <anatoly.bura...@intel.com>; Kozak, KubaX > <kubax.ko...@intel.com> > Cc: dev@dpdk.org; sta...@dpdk.org > Subject: Re: [PATCH] vfio: fix close unchecked file descriptor > > On 09/20/2017 10:39 AM, Burakov, Anatoly wrote: > > On 20-Sep-17 3:34 PM, Patrick MacArthur wrote: > >> On 09/20/2017 05:59 AM, Kuba Kozak wrote: > >>> Add file descriptor value check before calling close() function. > >>> > >>> Coverity issue: 141297 > >>> Fixes: 811b6b25060f ("vfio: fix file descriptor leak in > >>> multi-process") > >>> Cc: patr...@patrickmacarthur.net > >>> Cc: sta...@dpdk.org > >>> > >>> Signed-off-by: Kuba Kozak <kubax.ko...@intel.com> > >>> --- > >>> lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c | 3 ++- > >>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c > >>> b/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c > >>> index 7e8095c..c04f548 100644 > >>> --- a/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c > >>> +++ b/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c > >>> @@ -301,7 +301,8 @@ vfio_mp_sync_thread(void __rte_unused * arg) > >>> vfio_mp_sync_send_request(conn_sock, SOCKET_ERR); > >>> else > >>> vfio_mp_sync_send_fd(conn_sock, fd); > >>> - close(fd); > >>> + if (fd != -1) > >>> + close(fd); > >> > >> IMHO this should be: > >> > >> if (fd >= 0) > >> > >> What specifically is Coverity complaining about here? Is there a > >> specific code path that leads to fd being -1 here? > >> > > Hi Patrick, > > > > There's no way the fd will be 0 - the function we get the value from > > returns a valid fd, or a -1 in case of error. In this particular case, > > the "specific code path that leads to fd being -1" is when we can't > > get a container fd for some reason. I believe this is a very remote > > possibility as by the time we're spinning up the socket listening > > thread we're pretty sure we have a working VFIO container, but this is > > a valid fix nevertheless. Maybe having it >= 0 (or > 0, to be precise) > > would be cleaner, but it really makes no difference here. > > The point of my suggestion is that it would catch *any* negative value for fd > as opposed to just -1. > > I agree 0 should never happen since it is stdin but it is technically a valid > fd that could occur if the user > program did close(STDIN_FILENO) for some reason. > > I don't feel too strongly about it but feel like if we are going to fix what > amounts to close() possibly > returning EBADF we might as well fix it for all cases. > > Thanks, > Patrick
