From: Patrick MacArthur <patrick@patrickmacarthur.net>
To: "Burakov, Anatoly" <anatoly.burakov@intel.com>,
Kuba Kozak <kubax.kozak@intel.com>
Cc: dev@dpdk.org, stable@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] vfio: fix close unchecked file descriptor
Date: Wed, 20 Sep 2017 22:28:05 -0400 [thread overview]
Message-ID: <56ae6eed-85a5-0dcb-22d1-8567f0d5d342@patrickmacarthur.net> (raw)
In-Reply-To: <3abbb473-6d7f-9b48-d33f-59bd098e0ebf@intel.com>
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 <kubax.kozak@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
next prev parent reply other threads:[~2017-09-21 2:28 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-20 9:59 Kuba Kozak
2017-09-20 10:15 ` Burakov, Anatoly
2017-09-20 14:34 ` Patrick MacArthur
2017-09-20 14:39 ` Burakov, Anatoly
2017-09-21 2:28 ` Patrick MacArthur [this message]
2017-09-21 9:49 ` Kozak, KubaX
2017-09-21 13:49 ` [dpdk-dev] [PATCH v2] " Michal Jastrzebski
2017-09-21 16:07 ` Patrick MacArthur
2017-10-05 21:42 ` Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56ae6eed-85a5-0dcb-22d1-8567f0d5d342@patrickmacarthur.net \
--to=patrick@patrickmacarthur.net \
--cc=anatoly.burakov@intel.com \
--cc=dev@dpdk.org \
--cc=kubax.kozak@intel.com \
--cc=stable@dpdk.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).