From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from plateau.patrickmacarthur.net (plateau.patrickmacarthur.net [174.138.60.243]) by dpdk.org (Postfix) with ESMTP id 8482D7D4E; Thu, 21 Sep 2017 04:28:12 +0200 (CEST) Received: from [10.2.0.133] (WPIS-64-140-248-237.worldpath.net [64.140.248.237]) by plateau.patrickmacarthur.net (Postfix) with ESMTPSA id A0236BE613; Thu, 21 Sep 2017 02:28:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=patrickmacarthur.net; s=mail; t=1505960885; bh=IZvxsdCjrnte1nKgSOQ9wXzTLQbCvl9KJBJhDy7Dn9A=; h=From:Subject:To:Cc:References:Date:In-Reply-To; b=5Rtb5XPynfrq5c1/K9CrpUA1946iOElQ/SR19s3L++/ywP5YHTpuCd/ku8XfrOE4N Yj7/qQn/N5Fz/p4DlyP8LDOm9H2D4ZssdSQCXfW+qvEam05VK3SQ86iauoK66LH5u5 XmU1YJS7DiZdDqolsP9zbgc0ilYT7Aqzoz9azGp0= From: Patrick MacArthur To: "Burakov, Anatoly" , Kuba Kozak Cc: dev@dpdk.org, stable@dpdk.org References: <1505901573-463-1-git-send-email-kubax.kozak@intel.com> <3abbb473-6d7f-9b48-d33f-59bd098e0ebf@intel.com> Message-ID: <56ae6eed-85a5-0dcb-22d1-8567f0d5d342@patrickmacarthur.net> Date: Wed, 20 Sep 2017 22:28:05 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <3abbb473-6d7f-9b48-d33f-59bd098e0ebf@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=0.4 required=5.0 tests=ALL_TRUSTED,BODY_8BITS, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on plateau.patrickmacarthur.net Subject: Re: [dpdk-dev] [PATCH] vfio: fix close unchecked file descriptor X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 Sep 2017 02:28:12 -0000 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@patrickmacarthur.net >>> Cc: stable@dpdk.org >>> >>> Signed-off-by: Kuba Kozak >>> --- >>>   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