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

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:

quoted

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 -

quoted

-		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

quoted

 	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