Re: [PATCH] xfsdump: fix race condition between lseek() and read()/write()

[Eryu Guan <eguan@redhat.com>]

On Wed, Jul 12, 2017 at 03:56:00PM -0500, Eric Sandeen wrote:
> Hm, but isn't this a problem?

/me rewinds my mind by 15 months and confirms it is a problem :)

> 
> On 04/21/2016 08:06 AM, Eryu Guan wrote:
> > diff --git a/inventory/inv_core.c b/inventory/inv_core.c
> > index a17c2c9..42d0ac4 100644
> > --- a/inventory/inv_core.c
> > +++ b/inventory/inv_core.c
> > @@ -121,19 +121,10 @@ get_invtrecord( int fd, void *buf, size_t bufsz, off64_t off,
> >  	if ( dolock ) 
> >  		INVLOCK( fd, LOCK_SH );
> >  
> > -	if ( lseek( fd, (off_t)off, whence ) < 0 ) {
> 
> whence can be _SET or _CUR depending on the caller, but with this
> patch whence is no longer used, and -
> 
> > -		INV_PERROR( _("Error in reading inventory record "
> > -			      "(lseek failed): ") );
> > -		if ( dolock ) 
> > -			INVLOCK( fd, LOCK_UN );
> > -		return -1;
> > -	}
> > -	
> > -	nread = read( fd, buf, bufsz );
> > -
> > +	nread = pread(fd, buf, bufsz, (off_t)off);
> 
> This is only equivalent to SET, no?  With the other changes, perhaps we're

Yes, it's only equivalent to SET.

> no longer called via GET_REC_SEEKCUR - but in that case, the whence arg should
> be removed?  Except for that pesky test code which explicitly passes SEEK_CUR ...

Agreed, the whence arg can be removed, the test code requires some
updates though.

I'm now testing a new version, I'll post it once it passes '-g dump'
tests.

Thanks for the review!

Eryu

> 
> 
> -Eric
> 
> >  	if (  nread != (int) bufsz ) {
> >  		INV_PERROR( _("Error in reading inventory record :") );
> > -		if ( dolock ) 
> > +		if ( dolock )
> >  			INVLOCK( fd, LOCK_UN );
> >  		return -1;
> >  	}
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html