diff mbox series

examples/vhost_blk: fix the TOCTOU

Message ID 20191126153214.7952-1-jin.yu@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series examples/vhost_blk: fix the TOCTOU |

Checks

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

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
>
diff mbox series

Patch

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;