[dpdk-dev] [PATCH] vfio: fix close unchecked file descriptor

Kozak, KubaX kubax.kozak at intel.com
Thu Sep 21 11:49:26 CEST 2017


Hi,

Referring to Patrick suggestion I'll prepare patch with  (fd >= 0) condition.

Thanks,
Kuba

> -----Original Message-----
> From: Patrick MacArthur [mailto:patrick at patrickmacarthur.net]
> Sent: Thursday, September 21, 2017 04:28
> To: Burakov, Anatoly <anatoly.burakov at intel.com>; Kozak, KubaX <kubax.kozak at intel.com>
> Cc: dev at dpdk.org; stable at 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: patrick at patrickmacarthur.net
> >>> Cc: stable at dpdk.org
> >>>
> >>> Signed-off-by: Kuba Kozak <kubax.kozak at 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


More information about the dev mailing list