From 30d0f5bd78ae9b77d9bcbf15242bd2cda4c200e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Przemys=C5=82aw=20Czerpak?= Date: Fri, 13 Nov 2015 10:29:25 +0100 Subject: [PATCH] 2015-11-13 10:29 UTC+0100 Przemyslaw Czerpak (druzus/at/poczta.onet.pl) * src/rtl/filebuf.c ! fixed file handle sharing in POSIX systems when writeonly mode is used ! fixed accessing files in writeonly mode in POSIX systems which use fcntl() locks for DENY_* flag emulation. ! added for POSIX systems protection against potential race condition in MT programs and file open and close operations. Now the whole low level open and close operations are covered by mutex. It means that they are fully serialized so in case of remote file access (i.e. NFS or SMBFS) it may reduce the performance in applications which simultaneously open and closes many different files in different threads but its necessary. For local file access it should not create noticeable scalability problems. In my Linux box Harbour can open and close in MT mode more then 250000 files per second. % use different mutexes for file open/close and file lock operations to not reduce the lock performance by potentially slow open() operation in POSIX systems. ; NOTE: please remember that DENY_* flag emulation in POSIX systems emulates only shared and exclusive modes so if any of FO_DENYREAD, FO_DENYWRITE or FO_EXCLUSIVE flag is used then it effectively works like FO_EXCLUSIVE in POSIX system and is not mandatory for other processes so it blocks only other Harbour applications which use Harbour virtual handles (RDD code, hb_vf*() functions) * src/rtl/filesys.c * do not convert exclusive lock to shared one when file is open in readonly mode on POSIX system using flock() for DENY_* flag emulation. Now HB_USE_BSDLOCKS is enabled only for Linux and it's documented that this implementation allow to use shared and exclusive locks regardless of file open mode. % do not create file name copy in hb_fsExtOpen() when passed parameters does not force file name modification --- ChangeLog.txt | 34 ++++++ src/rtl/filebuf.c | 267 +++++++++++++++++++++++++++------------------- src/rtl/filesys.c | 17 ++- 3 files changed, 206 insertions(+), 112 deletions(-) diff --git a/ChangeLog.txt b/ChangeLog.txt index 2b528fd14c..da048a89ca 100644 --- a/ChangeLog.txt +++ b/ChangeLog.txt @@ -10,6 +10,40 @@ * Change, ! Fix, % Optimization, + Addition, - Removal, ; Comment */ +2015-11-13 10:29 UTC+0100 Przemyslaw Czerpak (druzus/at/poczta.onet.pl) + * src/rtl/filebuf.c + ! fixed file handle sharing in POSIX systems when writeonly mode is used + ! fixed accessing files in writeonly mode in POSIX systems which use + fcntl() locks for DENY_* flag emulation. + ! added for POSIX systems protection against potential race condition in + MT programs and file open and close operations. Now the whole low level + open and close operations are covered by mutex. It means that they are + fully serialized so in case of remote file access (i.e. NFS or SMBFS) + it may reduce the performance in applications which simultaneously open + and closes many different files in different threads but its necessary. + For local file access it should not create noticeable scalability + problems. In my Linux box Harbour can open and close in MT mode more + then 250000 files per second. + % use different mutexes for file open/close and file lock operations to + not reduce the lock performance by potentially slow open() operation + in POSIX systems. + ; NOTE: please remember that DENY_* flag emulation in POSIX systems + emulates only shared and exclusive modes so if any of + FO_DENYREAD, FO_DENYWRITE or FO_EXCLUSIVE flag is used then + it effectively works like FO_EXCLUSIVE in POSIX system and + is not mandatory for other processes so it blocks only + other Harbour applications which use Harbour virtual handles + (RDD code, hb_vf*() functions) + + * src/rtl/filesys.c + * do not convert exclusive lock to shared one when file is open in + readonly mode on POSIX system using flock() for DENY_* flag emulation. + Now HB_USE_BSDLOCKS is enabled only for Linux and it's documented that + this implementation allow to use shared and exclusive locks regardless of + file open mode. + % do not create file name copy in hb_fsExtOpen() when passed parameters + does not force file name modification + 2015-11-05 18:20 UTC+0100 Tamas TEVESZ (ice extreme.hu) * src/3rd/pcre/Makefile ! Fix bundled PCRE build on SunOS; diff --git a/src/rtl/filebuf.c b/src/rtl/filebuf.c index c43bc76402..f91d9523a9 100644 --- a/src/rtl/filebuf.c +++ b/src/rtl/filebuf.c @@ -96,8 +96,8 @@ typedef struct _HB_FILE HB_ULONG device; HB_ULONG inode; int used; + int mode; HB_BOOL shared; - HB_BOOL readonly; HB_FHANDLE hFile; HB_FHANDLE hFileRO; PHB_FLOCK pLocks; @@ -114,6 +114,7 @@ static const HB_FILE_FUNCS * s_fileMethods( void ); #endif static HB_CRITICAL_NEW( s_fileMtx ); +static HB_CRITICAL_NEW( s_lockMtx ); static PHB_FILE s_openFiles = NULL; @@ -150,7 +151,7 @@ static PHB_FILE hb_fileFind( HB_ULONG device, HB_ULONG inode ) return NULL; } -static PHB_FILE hb_fileNew( HB_FHANDLE hFile, HB_BOOL fShared, HB_BOOL fReadonly, +static PHB_FILE hb_fileNew( HB_FHANDLE hFile, HB_BOOL fShared, int iMode, HB_ULONG device, HB_ULONG inode, HB_BOOL fBind ) { PHB_FILE pFile = hb_fileFind( device, inode ); @@ -158,13 +159,13 @@ static PHB_FILE hb_fileNew( HB_FHANDLE hFile, HB_BOOL fShared, HB_BOOL fReadonly if( ! pFile ) { pFile = ( PHB_FILE ) hb_xgrabz( sizeof( HB_FILE ) ); - pFile->pFuncs = s_fileMethods(); - pFile->device = device; - pFile->inode = inode; - pFile->hFile = hFile; - pFile->hFileRO = FS_ERROR; - pFile->shared = fShared; - pFile->readonly = fReadonly; + pFile->pFuncs = s_fileMethods(); + pFile->device = device; + pFile->inode = inode; + pFile->hFile = hFile; + pFile->hFileRO = FS_ERROR; + pFile->shared = fShared; + pFile->mode = iMode; if( fBind ) { @@ -469,27 +470,53 @@ static PHB_FILE s_fileExtOpen( PHB_FILE_FUNCS pFuncs, const char * pszFileName, PHB_ITEM pError ) { PHB_FILE pFile = NULL; - #if defined( HB_OS_UNIX ) - HB_BOOL fResult, fSeek = HB_FALSE; + HB_BOOL fSeek = HB_FALSE; # if defined( HB_USE_LARGEFILE64 ) struct stat64 statbuf; # else struct stat statbuf; # endif #endif - HB_BOOL fShared, fReadonly; + HB_BOOL fResult, fShared; + int iMode; HB_FHANDLE hFile; char * pszFile; HB_SYMBOL_UNUSED( pFuncs ); fShared = ( nExFlags & ( FO_DENYREAD | FO_DENYWRITE | FO_EXCLUSIVE ) ) == 0; - fReadonly = ( nExFlags & ( FO_READ | FO_WRITE | FO_READWRITE ) ) == FO_READ; + iMode = ( int ) ( nExFlags & ( FO_READ | FO_WRITE | FO_READWRITE ) ); pszFile = hb_fsExtName( pszFileName, pDefExt, nExFlags, pPaths ); hb_vmUnlock(); -#if defined( HB_OS_UNIX ) +#if ! defined( HB_OS_UNIX ) + fResult = HB_TRUE; +#else +# if defined( HB_USE_SHARELOCKS ) && ! defined( HB_USE_BSDLOCKS ) + if( nExFlags & FXO_SHARELOCK ) + { + if( iMode == FO_WRITE && fShared ) + { + if( access( ( char * ) pszFile, R_OK ) == 0 || + access( ( char * ) pszFile, F_OK ) != 0 ) + { + nExFlags = ( nExFlags ^ FO_WRITE ) | FO_READWRITE; + iMode = FO_READWRITE; + } + else + nExFlags ^= FXO_SHARELOCK; + } + else if( iMode == FO_READ && ! fShared ) + { + nExFlags &= ~ ( HB_FATTR ) ( FO_DENYREAD | FO_DENYWRITE | FO_EXCLUSIVE ); + fShared = HB_TRUE; + } + } +# endif + + hb_threadEnterCriticalSection( &s_fileMtx ); + # if defined( HB_USE_LARGEFILE64 ) fResult = stat64( ( char * ) pszFile, &statbuf ) == 0; # else @@ -499,57 +526,49 @@ static PHB_FILE s_fileExtOpen( PHB_FILE_FUNCS pFuncs, const char * pszFileName, if( fResult ) { - hb_threadEnterCriticalSection( &s_fileMtx ); - pFile = hb_fileFind( statbuf.st_dev, statbuf.st_ino ); + pFile = hb_fileFind( ( HB_ULONG ) statbuf.st_dev, + ( HB_ULONG ) statbuf.st_ino ); if( pFile ) { if( ! fShared || ! pFile->shared || ( nExFlags & FXO_TRUNCATE ) != 0 ) + { fResult = HB_FALSE; - else if( ! fReadonly && pFile->readonly ) pFile = NULL; - else - pFile->used++; - - if( ( nExFlags & FXO_NOSEEKPOS ) == 0 ) - { -# if defined( HB_OS_VXWORKS ) - fSeek = ! S_ISFIFO( statbuf.st_mode ); -# else - fSeek = ! S_ISFIFO( statbuf.st_mode ) && ! S_ISSOCK( statbuf.st_mode ); -# endif } - } - hb_threadLeaveCriticalSection( &s_fileMtx ); - } - - if( pFile ) - { - if( ! fResult ) - { - hb_fsSetError( ( nExFlags & FXO_TRUNCATE ) ? 5 : 32 ); - pFile = NULL; - } - else if( nExFlags & FXO_COPYNAME ) - hb_strncpy( ( char * ) pszFileName, pszFile, HB_PATH_MAX - 1 ); - - if( pError ) - { - hb_errPutFileName( pError, pszFile ); - if( ! fResult ) + else if( pFile->mode != FO_READWRITE && pFile->mode != iMode ) { - hb_errPutOsCode( pError, hb_fsError() ); - hb_errPutGenCode( pError, ( HB_ERRCODE ) ( ( nExFlags & FXO_TRUNCATE ) ? EG_CREATE : EG_OPEN ) ); + iMode = FO_READWRITE; + pFile = NULL; + } + else + { + pFile->used++; + if( ( nExFlags & FXO_NOSEEKPOS ) == 0 ) + { +# if defined( HB_OS_VXWORKS ) + fSeek = ! S_ISFIFO( statbuf.st_mode ); +# else + fSeek = ! S_ISFIFO( statbuf.st_mode ) && ! S_ISSOCK( statbuf.st_mode ); +# endif + } } } } else -#endif + fResult = HB_TRUE; + + if( fResult && pFile == NULL ) +#endif /* HB_OS_UNIX */ { - hFile = hb_fsExtOpen( pszFileName, pDefExt, nExFlags, pPaths, pError ); + hFile = hb_fsExtOpen( pszFileName, NULL, + nExFlags & ~ ( HB_FATTR ) ( FXO_DEFAULTS | FXO_COPYNAME ), + NULL, NULL ); if( hFile != FS_ERROR ) { HB_ULONG device = 0, inode = 0; -#if defined( HB_OS_UNIX ) +#if ! defined( HB_OS_UNIX ) + hb_threadEnterCriticalSection( &s_fileMtx ); +#else # if defined( HB_USE_LARGEFILE64 ) if( fstat64( hFile, &statbuf ) == 0 ) # else @@ -567,19 +586,30 @@ static PHB_FILE s_fileExtOpen( PHB_FILE_FUNCS pFuncs, const char * pszFileName, # endif } } -#endif +#endif /* HB_OS_UNIX */ - hb_threadEnterCriticalSection( &s_fileMtx ); - pFile = hb_fileNew( hFile, fShared, fReadonly, device, inode, HB_TRUE ); + pFile = hb_fileNew( hFile, fShared, iMode, device, inode, HB_TRUE ); if( pFile->hFile != hFile ) { - if( pFile->hFileRO == FS_ERROR && ! fReadonly && pFile->readonly ) + if( pFile->mode != FO_READWRITE && iMode == FO_READWRITE ) { + HB_FHANDLE hTemp = pFile->hFileRO; pFile->hFileRO = pFile->hFile; pFile->hFile = hFile; - pFile->readonly = HB_FALSE; - hFile = FS_ERROR; + pFile->mode = iMode; + hFile = hTemp; } + + if( ! fShared || ! pFile->shared || pFile->mode != FO_READWRITE ) + { + fResult = HB_FALSE; + if( pFile->hFileRO == FS_ERROR && pFile->uiLocks != 0 ) + { + pFile->hFileRO = hFile; + hFile = FS_ERROR; + } + } + if( pFile->uiLocks == 0 ) { #if ! defined( HB_USE_SHARELOCKS ) || defined( HB_USE_BSDLOCKS ) @@ -595,33 +625,56 @@ static PHB_FILE s_fileExtOpen( PHB_FILE_FUNCS pFuncs, const char * pszFileName, hFile = FS_ERROR; #if defined( HB_USE_SHARELOCKS ) && ! defined( HB_USE_BSDLOCKS ) /* TOFIX: possible race condition */ - hb_fsLockLarge( hFile, HB_SHARELOCK_POS, HB_SHARELOCK_SIZE, + hb_fsLockLarge( pFile->hFile, HB_SHARELOCK_POS, HB_SHARELOCK_SIZE, FL_LOCK | FLX_SHARED ); #endif } } + if( !fResult ) + { + if( pFile ) + { + --pFile->used; + pFile = NULL; + } + if( hFile != FS_ERROR ) + { + /* TOFIX: possible race condition in MT mode, + * close() is not safe due to existing locks + * which are removed. + */ + hb_fsClose( hFile ); + } + } } - else - hFile = FS_ERROR; +#if ! defined( HB_OS_UNIX ) hb_threadLeaveCriticalSection( &s_fileMtx ); - - if( hFile != FS_ERROR ) - { - /* TOFIX: possible race condition in MT mode, - * close() is not safe due to existing locks - * which are removed. - */ - hb_fsClose( hFile ); - } +#endif } } - hb_xfree( pszFile ); #if defined( HB_OS_UNIX ) + hb_threadLeaveCriticalSection( &s_fileMtx ); if( pFile && fSeek ) pFile = hb_fileposNew( pFile ); - #endif + + if( ! fResult ) + hb_fsSetError( ( nExFlags & FXO_TRUNCATE ) ? 5 : 32 ); + if( ( nExFlags & FXO_COPYNAME ) != 0 && pFile ) + hb_strncpy( ( char * ) pszFileName, pszFile, HB_PATH_MAX - 1 ); + if( pError ) + { + hb_errPutFileName( pError, pszFile ); + if( ! fResult ) + { + hb_errPutOsCode( pError, hb_fsError() ); + hb_errPutGenCode( pError, ( HB_ERRCODE ) ( ( nExFlags & FXO_TRUNCATE ) ? EG_CREATE : EG_OPEN ) ); + } + } + + hb_xfree( pszFile ); + hb_vmLock(); return pFile; @@ -629,9 +682,8 @@ static PHB_FILE s_fileExtOpen( PHB_FILE_FUNCS pFuncs, const char * pszFileName, static void s_fileClose( PHB_FILE pFile ) { - HB_FHANDLE hFile = FS_ERROR, hFileRO = FS_ERROR; - hb_vmUnlock(); + hb_fsSetError( 0 ); hb_threadEnterCriticalSection( &s_fileMtx ); if( --pFile->used == 0 ) @@ -647,25 +699,21 @@ static void s_fileClose( PHB_FILE pFile ) s_openFiles = NULL; } } - - hFile = pFile->hFile; - hFileRO = pFile->hFileRO; + if( pFile->hFile != FS_ERROR ) + hb_fsClose( pFile->hFile ); + if( pFile->hFileRO != FS_ERROR ) + hb_fsClose( pFile->hFileRO ); if( pFile->pLocks ) hb_xfree( pFile->pLocks ); hb_xfree( pFile ); } + else + pFile = NULL; hb_threadLeaveCriticalSection( &s_fileMtx ); hb_vmLock(); - - hb_fsSetError( 0 ); - - if( hFile != FS_ERROR ) - hb_fsClose( hFile ); - if( hFileRO != FS_ERROR ) - hb_fsClose( hFileRO ); } static HB_BOOL s_fileLock( PHB_FILE pFile, HB_FOFFSET nStart, HB_FOFFSET nLen, @@ -676,36 +724,38 @@ static HB_BOOL s_fileLock( PHB_FILE pFile, HB_FOFFSET nStart, HB_FOFFSET nLen, hb_vmUnlock(); if( ( iType & FL_MASK ) == FL_UNLOCK ) { - hb_threadEnterCriticalSection( &s_fileMtx ); + hb_threadEnterCriticalSection( &s_lockMtx ); fResult = hb_fileUnlock( pFile, &fLockFS, nStart, nLen ); - hb_threadLeaveCriticalSection( &s_fileMtx ); + hb_threadLeaveCriticalSection( &s_lockMtx ); if( fLockFS ) { hb_fsLockLarge( pFile->hFile, nStart, nLen, ( HB_USHORT ) iType ); - hb_threadEnterCriticalSection( &s_fileMtx ); + hb_threadEnterCriticalSection( &s_lockMtx ); hb_fileUnlock( pFile, NULL, nStart, nLen ); - hb_threadLeaveCriticalSection( &s_fileMtx ); + hb_threadLeaveCriticalSection( &s_lockMtx ); } else hb_fsSetError( fResult ? 0 : 33 ); } else { - hb_threadEnterCriticalSection( &s_fileMtx ); + hb_threadEnterCriticalSection( &s_lockMtx ); fResult = hb_fileSetLock( pFile, &fLockFS, nStart, nLen ); - hb_threadLeaveCriticalSection( &s_fileMtx ); + hb_threadLeaveCriticalSection( &s_lockMtx ); if( fLockFS ) { #if defined( HB_OS_UNIX ) - if( pFile->readonly ) + if( pFile->mode == FO_READ ) iType |= FLX_SHARED; + else if( pFile->mode == FO_WRITE ) + iType &= ~FLX_SHARED; #endif fResult = hb_fsLockLarge( pFile->hFile, nStart, nLen, ( HB_USHORT ) iType ); if( ! fResult ) { - hb_threadEnterCriticalSection( &s_fileMtx ); + hb_threadEnterCriticalSection( &s_lockMtx ); hb_fileUnlock( pFile, NULL, nStart, nLen ); - hb_threadLeaveCriticalSection( &s_fileMtx ); + hb_threadLeaveCriticalSection( &s_lockMtx ); } } else @@ -724,9 +774,9 @@ static int s_fileLockTest( PHB_FILE pFile, HB_FOFFSET nStart, HB_FOFFSET nLen, hb_vmUnlock(); - hb_threadEnterCriticalSection( &s_fileMtx ); + hb_threadEnterCriticalSection( &s_lockMtx ); fLocked = hb_fileTestLock( pFile, nStart, nLen ); - hb_threadLeaveCriticalSection( &s_fileMtx ); + hb_threadLeaveCriticalSection( &s_lockMtx ); if( fLocked ) { #if defined( HB_OS_UNIX ) @@ -1078,7 +1128,7 @@ HB_BOOL hb_fileRegisterFull( const HB_FILE_FUNCS * pFuncs ) HB_BOOL fResult = HB_FALSE; hb_vmUnlock(); - hb_threadEnterCriticalSection( &s_fileMtx ); + hb_threadEnterCriticalSection( &s_lockMtx ); if( s_iFileTypes < HB_FILE_TYPE_MAX ) { @@ -1087,7 +1137,7 @@ HB_BOOL hb_fileRegisterFull( const HB_FILE_FUNCS * pFuncs ) fResult = HB_TRUE; } - hb_threadLeaveCriticalSection( &s_fileMtx ); + hb_threadLeaveCriticalSection( &s_lockMtx ); hb_vmLock(); return fResult; @@ -1491,15 +1541,13 @@ HB_BYTE * hb_fileLoadData( PHB_FILE pFile, HB_SIZE nMaxSize, HB_SIZE * pnSize ) { HB_BYTE * pFileBuf = NULL; - HB_SIZE nSize = 0; + HB_SIZE nSize = 0, nRead, nBufSize; HB_FOFFSET nFileSize = hb_fileSize( pFile ); if( nFileSize == FS_ERROR || ( nFileSize == 0 && hb_fsError() == HB_FILE_ERR_UNSUPPORTED ) ) { - HB_SIZE nRead, nBufSize = 0; - - for( ;; ) + for( nBufSize = 0;; ) { if( nBufSize == nSize ) { @@ -1518,16 +1566,21 @@ HB_BYTE * hb_fileLoadData( PHB_FILE pFile, HB_SIZE nMaxSize, nSize += nRead; } } - else + else if( nFileSize > 0 ) { - nSize = ( HB_SIZE ) nFileSize; - if( nMaxSize > 0 && nSize > nMaxSize ) - nSize = nMaxSize; + nBufSize = ( HB_SIZE ) nFileSize; + if( nMaxSize > 0 && nBufSize > nMaxSize ) + nBufSize = nMaxSize; - pFileBuf = ( HB_BYTE * ) hb_xgrab( nSize + 1 ); - nSize = hb_fileReadAt( pFile, pFileBuf, nSize, 0 ); - if( nSize == ( HB_SIZE ) FS_ERROR ) - nSize = 0; + pFileBuf = ( HB_BYTE * ) hb_xgrab( nBufSize + 1 ); + do + { + nRead = hb_fileReadAt( pFile, pFileBuf + nSize, nBufSize - nSize, nSize ); + if( nRead == 0 || nRead == ( HB_SIZE ) FS_ERROR ) + break; + nSize += nRead; + } + while( nSize < nBufSize ); } if( nSize > 0 ) diff --git a/src/rtl/filesys.c b/src/rtl/filesys.c index 2eae6dbe2a..ccf395ab98 100644 --- a/src/rtl/filesys.c +++ b/src/rtl/filesys.c @@ -4501,7 +4501,8 @@ HB_FHANDLE hb_fsExtOpen( const char * pszFileName, const char * pDefExt, { HB_FHANDLE hFile; HB_USHORT uiFlags; - char * szPath; + const char * szPath; + char * szFree = NULL; HB_TRACE( HB_TR_DEBUG, ( "hb_fsExtOpen(%s, %s, %u, %p, %p)", pszFileName, pDefExt, nExFlags, pPaths, pError ) ); @@ -4520,7 +4521,11 @@ HB_FHANDLE hb_fsExtOpen( const char * pszFileName, const char * pDefExt, hb_errGetFileName( pError ); #endif - szPath = hb_fsExtName( pszFileName, pDefExt, nExFlags, pPaths ); + if( pDefExt || pPaths || pError || + ( nExFlags & ( FXO_DEFAULTS | FXO_COPYNAME ) ) != 0 ) + szPath = szFree = hb_fsExtName( pszFileName, pDefExt, nExFlags, pPaths ); + else + szPath = pszFileName; uiFlags = ( HB_USHORT ) ( nExFlags & 0xff ); if( nExFlags & ( FXO_TRUNCATE | FXO_APPEND | FXO_UNIQUE ) ) @@ -4539,11 +4544,11 @@ HB_FHANDLE hb_fsExtOpen( const char * pszFileName, const char * pDefExt, hFile = hb_fsOpen( szPath, uiFlags ); #if defined( HB_USE_SHARELOCKS ) - if( hFile != FS_ERROR && nExFlags & FXO_SHARELOCK ) + if( hFile != FS_ERROR && ( nExFlags & FXO_SHARELOCK ) != 0 ) { #if defined( HB_USE_BSDLOCKS ) int iLock, iResult; - if( ( uiFlags & ( FO_READ | FO_WRITE | FO_READWRITE ) ) == FO_READ || + if( /* ( uiFlags & ( FO_READ | FO_WRITE | FO_READWRITE ) ) == FO_READ || */ ( uiFlags & ( FO_DENYREAD | FO_DENYWRITE | FO_EXCLUSIVE ) ) == 0 ) iLock = LOCK_SH | LOCK_NB; else @@ -4609,7 +4614,9 @@ HB_FHANDLE hb_fsExtOpen( const char * pszFileName, const char * pDefExt, if( nExFlags & FXO_COPYNAME && hFile != FS_ERROR ) hb_strncpy( ( char * ) pszFileName, szPath, HB_PATH_MAX - 1 ); - hb_xfree( szPath ); + if( szFree ) + hb_xfree( szFree ); + return hFile; }