examples/vhost_blk: fix the TOCTOU
diff mbox series

Message ID 20191126153214.7952-1-jin.yu@intel.com
State Superseded
Delegated to: Maxime Coquelin
Headers show
Series
  • examples/vhost_blk: fix the TOCTOU
Related show

Checks

Context Check Description
ci/iol-intel-Performance fail Performance Testing issues
ci/Intel-compilation success Compilation OK
ci/travis-robot success Travis build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-compilation fail Compilie Testing issues
ci/checkpatch success coding style OK

Commit Message

Jin Yu Nov. 26, 2019, 3:32 p.m. UTC
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

Bruce Richardson Nov. 26, 2019, 10:25 a.m. UTC | #1
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
Jin Yu Nov. 27, 2019, 2:36 a.m. UTC | #2
> -----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
Bruce Richardson Nov. 27, 2019, 9:35 a.m. UTC | #3
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.
Maxime Coquelin Jan. 14, 2020, 9:32 a.m. UTC | #4
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
>

Patch
diff mbox series

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);
 
 	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;