patches for DPDK stable branches
 help / color / Atom feed
* Re: [dpdk-stable] [dpdk-dev] [PATCH] examples/vhost_blk: fix the TOCTOU
  2019-11-26 15:32 [dpdk-stable] [PATCH] examples/vhost_blk: fix the TOCTOU Jin Yu
@ 2019-11-26 10:25 ` " Bruce Richardson
  2019-11-27  2:36   ` Yu, Jin
  0 siblings, 1 reply; 4+ messages in thread
From: Bruce Richardson @ 2019-11-26 10:25 UTC (permalink / raw)
  To: Jin Yu; +Cc: Maxime Coquelin, Tiwei Bie, Zhihong Wang, dev, stable

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [dpdk-stable] [PATCH] examples/vhost_blk: fix the TOCTOU
@ 2019-11-26 15:32 Jin Yu
  2019-11-26 10:25 ` [dpdk-stable] [dpdk-dev] " Bruce Richardson
  0 siblings, 1 reply; 4+ messages in thread
From: Jin Yu @ 2019-11-26 15:32 UTC (permalink / raw)
  To: Maxime Coquelin, Tiwei Bie, Zhihong Wang; +Cc: dev, Jin Yu, stable

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] examples/vhost_blk: fix the TOCTOU
  2019-11-26 10:25 ` [dpdk-stable] [dpdk-dev] " Bruce Richardson
@ 2019-11-27  2:36   ` Yu, Jin
  2019-11-27  9:35     ` Bruce Richardson
  0 siblings, 1 reply; 4+ messages in thread
From: Yu, Jin @ 2019-11-27  2:36 UTC (permalink / raw)
  To: Richardson, Bruce; +Cc: Maxime Coquelin, Bie, Tiwei, Wang, Zhihong, dev, stable

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] examples/vhost_blk: fix the TOCTOU
  2019-11-27  2:36   ` Yu, Jin
@ 2019-11-27  9:35     ` Bruce Richardson
  0 siblings, 0 replies; 4+ messages in thread
From: Bruce Richardson @ 2019-11-27  9:35 UTC (permalink / raw)
  To: Yu, Jin; +Cc: Maxime Coquelin, Bie, Tiwei, Wang, Zhihong, dev, stable

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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-26 15:32 [dpdk-stable] [PATCH] examples/vhost_blk: fix the TOCTOU Jin Yu
2019-11-26 10:25 ` [dpdk-stable] [dpdk-dev] " Bruce Richardson
2019-11-27  2:36   ` Yu, Jin
2019-11-27  9:35     ` Bruce Richardson

patches for DPDK stable branches

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ http://inbox.dpdk.org/stable \
		stable@dpdk.org
	public-inbox-index stable


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.stable


AGPL code for this site: git clone https://public-inbox.org/ public-inbox