examples/vhost_blk: fix the TOCTOU
Checks
Commit Message
Fix the time of check time of use warning in example code
Coverity issue: 350589 158663
Fixes: c19beb3f38cd ("examples/vhost_blk: introduce vhost storage sample")
Cc: stable@dpdk.org
Signed-off-by: Jin Yu <jin.yu@intel.com>
---
examples/vhost_blk/vhost_blk.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
Comments
On Tue, Nov 26, 2019 at 11:32:14PM +0800, Jin Yu wrote:
> Fix the time of check time of use warning in example code
>
> Coverity issue: 350589 158663
> Fixes: c19beb3f38cd ("examples/vhost_blk: introduce vhost storage sample")
> Cc: stable@dpdk.org
>
> Signed-off-by: Jin Yu <jin.yu@intel.com>
> ---
> examples/vhost_blk/vhost_blk.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/examples/vhost_blk/vhost_blk.c b/examples/vhost_blk/vhost_blk.c
> index 3182a488b..bcb4ebb0b 100644
> --- a/examples/vhost_blk/vhost_blk.c
> +++ b/examples/vhost_blk/vhost_blk.c
> @@ -993,11 +993,7 @@ vhost_blk_ctrlr_construct(const char *ctrlr_name)
> }
> snprintf(dev_pathname, sizeof(dev_pathname), "%s/%s", path, ctrlr_name);
>
> - if (access(dev_pathname, F_OK) != -1) {
> - if (unlink(dev_pathname) != 0)
> - rte_exit(EXIT_FAILURE, "Cannot remove %s.\n",
> - dev_pathname);
> - }
> + unlink(dev_pathname);
>
The original code did an exit if the delete failed, do you intend there to
be a behaviour change here? You can probably get the same behaviour if you
check the errno on an unlink failure, e.g. ENOENT means file doesn't exist.
If not having the app exit on unlink failure is reasonable behaviour then
ignore this comment.
Regards,
/Bruce
> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Tuesday, November 26, 2019 6:26 PM
> To: Yu, Jin <jin.yu@intel.com>
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; Bie, Tiwei
> <tiwei.bie@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>;
> dev@dpdk.org; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] examples/vhost_blk: fix the TOCTOU
>
> On Tue, Nov 26, 2019 at 11:32:14PM +0800, Jin Yu wrote:
> > Fix the time of check time of use warning in example code
> >
> > Coverity issue: 350589 158663
> > Fixes: c19beb3f38cd ("examples/vhost_blk: introduce vhost storage
> > sample")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Jin Yu <jin.yu@intel.com>
> > ---
> > examples/vhost_blk/vhost_blk.c | 9 ++-------
> > 1 file changed, 2 insertions(+), 7 deletions(-)
> >
> > diff --git a/examples/vhost_blk/vhost_blk.c
> > b/examples/vhost_blk/vhost_blk.c index 3182a488b..bcb4ebb0b 100644
> > --- a/examples/vhost_blk/vhost_blk.c
> > +++ b/examples/vhost_blk/vhost_blk.c
> > @@ -993,11 +993,7 @@ vhost_blk_ctrlr_construct(const char *ctrlr_name)
> > }
> > snprintf(dev_pathname, sizeof(dev_pathname), "%s/%s", path,
> > ctrlr_name);
> >
> > - if (access(dev_pathname, F_OK) != -1) {
> > - if (unlink(dev_pathname) != 0)
> > - rte_exit(EXIT_FAILURE, "Cannot remove %s.\n",
> > - dev_pathname);
> > - }
> > + unlink(dev_pathname);
> >
>
> The original code did an exit if the delete failed, do you intend there to be a
> behaviour change here? You can probably get the same behaviour if you
> check the errno on an unlink failure, e.g. ENOENT means file doesn't exist.
>
> If not having the app exit on unlink failure is reasonable behaviour then
> ignore this comment.
I considered it. I think it's ok to ignore the errno of unlink failure. This code just want
to remove the file. There are two situations. The first one is that file doesn't exist the unlink
fails and it's ok to ignore. The second one is that unlink fails to remove file but the next bind()
would fail too so I think it's ok to ignore too.
>
> Regards,
> /Bruce
On Wed, Nov 27, 2019 at 02:36:01AM +0000, Yu, Jin wrote:
> > -----Original Message-----
> > From: Bruce Richardson <bruce.richardson@intel.com>
> > Sent: Tuesday, November 26, 2019 6:26 PM
> > To: Yu, Jin <jin.yu@intel.com>
> > Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; Bie, Tiwei
> > <tiwei.bie@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>;
> > dev@dpdk.org; stable@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] examples/vhost_blk: fix the TOCTOU
> >
> > On Tue, Nov 26, 2019 at 11:32:14PM +0800, Jin Yu wrote:
> > > Fix the time of check time of use warning in example code
> > >
> > > Coverity issue: 350589 158663
> > > Fixes: c19beb3f38cd ("examples/vhost_blk: introduce vhost storage
> > > sample")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Jin Yu <jin.yu@intel.com>
> > > ---
> > > examples/vhost_blk/vhost_blk.c | 9 ++-------
> > > 1 file changed, 2 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/examples/vhost_blk/vhost_blk.c
> > > b/examples/vhost_blk/vhost_blk.c index 3182a488b..bcb4ebb0b 100644
> > > --- a/examples/vhost_blk/vhost_blk.c
> > > +++ b/examples/vhost_blk/vhost_blk.c
> > > @@ -993,11 +993,7 @@ vhost_blk_ctrlr_construct(const char *ctrlr_name)
> > > }
> > > snprintf(dev_pathname, sizeof(dev_pathname), "%s/%s", path,
> > > ctrlr_name);
> > >
> > > - if (access(dev_pathname, F_OK) != -1) {
> > > - if (unlink(dev_pathname) != 0)
> > > - rte_exit(EXIT_FAILURE, "Cannot remove %s.\n",
> > > - dev_pathname);
> > > - }
> > > + unlink(dev_pathname);
> > >
> >
> > The original code did an exit if the delete failed, do you intend there to be a
> > behaviour change here? You can probably get the same behaviour if you
> > check the errno on an unlink failure, e.g. ENOENT means file doesn't exist.
> >
> > If not having the app exit on unlink failure is reasonable behaviour then
> > ignore this comment.
>
> I considered it. I think it's ok to ignore the errno of unlink failure. This code just want
> to remove the file. There are two situations. The first one is that file doesn't exist the unlink
> fails and it's ok to ignore. The second one is that unlink fails to remove file but the next bind()
> would fail too so I think it's ok to ignore too.
> >
Ok, thanks for clarifying.
On 11/27/19 3:36 AM, Yu, Jin wrote:
>> -----Original Message-----
>> From: Bruce Richardson <bruce.richardson@intel.com>
>> Sent: Tuesday, November 26, 2019 6:26 PM
>> To: Yu, Jin <jin.yu@intel.com>
>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; Bie, Tiwei
>> <tiwei.bie@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>;
>> dev@dpdk.org; stable@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] examples/vhost_blk: fix the TOCTOU
>>
>> On Tue, Nov 26, 2019 at 11:32:14PM +0800, Jin Yu wrote:
>>> Fix the time of check time of use warning in example code
>>>
>>> Coverity issue: 350589 158663
>>> Fixes: c19beb3f38cd ("examples/vhost_blk: introduce vhost storage
>>> sample")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Jin Yu <jin.yu@intel.com>
>>> ---
>>> examples/vhost_blk/vhost_blk.c | 9 ++-------
>>> 1 file changed, 2 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/examples/vhost_blk/vhost_blk.c
>>> b/examples/vhost_blk/vhost_blk.c index 3182a488b..bcb4ebb0b 100644
>>> --- a/examples/vhost_blk/vhost_blk.c
>>> +++ b/examples/vhost_blk/vhost_blk.c
>>> @@ -993,11 +993,7 @@ vhost_blk_ctrlr_construct(const char *ctrlr_name)
>>> }
>>> snprintf(dev_pathname, sizeof(dev_pathname), "%s/%s", path,
>>> ctrlr_name);
>>>
>>> - if (access(dev_pathname, F_OK) != -1) {
>>> - if (unlink(dev_pathname) != 0)
>>> - rte_exit(EXIT_FAILURE, "Cannot remove %s.\n",
>>> - dev_pathname);
>>> - }
>>> + unlink(dev_pathname);
>>>
>>
>> The original code did an exit if the delete failed, do you intend there to be a
>> behaviour change here? You can probably get the same behaviour if you
>> check the errno on an unlink failure, e.g. ENOENT means file doesn't exist.
>>
>> If not having the app exit on unlink failure is reasonable behaviour then
>> ignore this comment.
>
> I considered it. I think it's ok to ignore the errno of unlink failure. This code just want
> to remove the file. There are two situations. The first one is that file doesn't exist the unlink
> fails and it's ok to ignore. The second one is that unlink fails to remove file but the next bind()
> would fail too so I think it's ok to ignore too.
That's fine by me, but please could you mention it in the commit
message?
>> Regards,
>> /Bruce
>
@@ -993,11 +993,7 @@ vhost_blk_ctrlr_construct(const char *ctrlr_name)
}
snprintf(dev_pathname, sizeof(dev_pathname), "%s/%s", path, ctrlr_name);
- if (access(dev_pathname, F_OK) != -1) {
- if (unlink(dev_pathname) != 0)
- rte_exit(EXIT_FAILURE, "Cannot remove %s.\n",
- dev_pathname);
- }
+ unlink(dev_pathname);
if (rte_vhost_driver_register(dev_pathname, 0) != 0) {
fprintf(stderr, "socket %s already exists\n", dev_pathname);
@@ -1040,8 +1036,7 @@ signal_handler(__rte_unused int signum)
{
struct vhost_blk_ctrlr *ctrlr;
- if (access(dev_pathname, F_OK) == 0)
- unlink(dev_pathname);
+ unlink(dev_pathname);
if (g_should_stop != -1) {
g_should_stop = 1;