From afeb3c3a83ed0869e4a70b0725e3f85713330c3a Mon Sep 17 00:00:00 2001 From: Jeffrey Altman Date: Tue, 17 Jan 2012 19:43:54 -0500 Subject: [PATCH] Windows: prevent race assigning Fcb in AFSInitFcb() AFSInitFcb() is executed when the ObjectInformation->Fcb pointer is NULL. More than one thread can make that determination at the same time. Use InterlockedCompareExchangePointer() to detect a race and permit cleanup to be performed. Remove the output parameter of AFSInitFcb() to avoid a double assignment. Change-Id: I3870cccd5cd5e95134446523cce3547a2135d5e3 Reviewed-on: http://gerrit.openafs.org/6562 Tested-by: BuildBot Reviewed-by: Jeffrey Altman Tested-by: Jeffrey Altman --- src/WINNT/afsrdr/kernel/lib/AFSCreate.cpp | 94 ++++++++++++++----- src/WINNT/afsrdr/kernel/lib/AFSFcbSupport.cpp | 60 +++++++----- .../afsrdr/kernel/lib/Include/AFSCommon.h | 3 +- 3 files changed, 107 insertions(+), 50 deletions(-) diff --git a/src/WINNT/afsrdr/kernel/lib/AFSCreate.cpp b/src/WINNT/afsrdr/kernel/lib/AFSCreate.cpp index 981404960..5c2f9a2be 100644 --- a/src/WINNT/afsrdr/kernel/lib/AFSCreate.cpp +++ b/src/WINNT/afsrdr/kernel/lib/AFSCreate.cpp @@ -1896,10 +1896,12 @@ AFSProcessCreate( IN PIRP Irp, // Allocate and initialize the Fcb for the file. // - ntStatus = AFSInitFcb( pDirEntry, - Fcb); + ntStatus = AFSInitFcb( pDirEntry); - if( !NT_SUCCESS( ntStatus)) + *Fcb = pObjectInfo->Fcb; + + if( !NT_SUCCESS( ntStatus) && + ntStatus != STATUS_REPARSE) { AFSDbgLogMsg( AFS_SUBSYSTEM_FILE_PROCESSING, @@ -1912,7 +1914,13 @@ AFSProcessCreate( IN PIRP Irp, try_return( ntStatus); } - bAllocatedFcb = TRUE; + if ( ntStatus != STATUS_REPARSE) + { + + bAllocatedFcb = TRUE; + } + + ntStatus = STATUS_SUCCESS; } bReleaseFcb = TRUE; @@ -2252,10 +2260,12 @@ AFSOpenTargetDirectory( IN PIRP Irp, // Allocate and initialize the Fcb for the file. // - ntStatus = AFSInitFcb( ParentDirectoryCB, - Fcb); + ntStatus = AFSInitFcb( ParentDirectoryCB); - if( !NT_SUCCESS( ntStatus)) + *Fcb = pParentObject->Fcb; + + if( !NT_SUCCESS( ntStatus) && + ntStatus != STATUS_REPARSE) { AFSDbgLogMsg( AFS_SUBSYSTEM_FILE_PROCESSING, @@ -2268,7 +2278,13 @@ AFSOpenTargetDirectory( IN PIRP Irp, try_return( ntStatus); } - bAllocatedFcb = TRUE; + if ( ntStatus == STATUS_REPARSE) + { + + bAllocatedFcb = TRUE; + } + + ntStatus = STATUS_SUCCESS; } bReleaseFcb = TRUE; @@ -2579,10 +2595,10 @@ AFSProcessOpen( IN PIRP Irp, if( pObjectInfo->Fcb == NULL) { - ntStatus = AFSInitFcb( DirectoryCB, - &pObjectInfo->Fcb); + ntStatus = AFSInitFcb( DirectoryCB); - if( !NT_SUCCESS( ntStatus)) + if( !NT_SUCCESS( ntStatus) && + ntStatus != STATUS_REPARSE) { AFSDbgLogMsg( AFS_SUBSYSTEM_FILE_PROCESSING, @@ -2595,7 +2611,13 @@ AFSProcessOpen( IN PIRP Irp, try_return( ntStatus); } - bAllocatedFcb = TRUE; + if ( ntStatus != STATUS_REPARSE) + { + + bAllocatedFcb = TRUE; + } + + ntStatus = STATUS_SUCCESS; } else { @@ -3068,10 +3090,12 @@ AFSProcessOverwriteSupersede( IN PDEVICE_OBJECT DeviceObject, if( pObjectInfo->Fcb == NULL) { - ntStatus = AFSInitFcb( DirectoryCB, - Fcb); + ntStatus = AFSInitFcb( DirectoryCB); - if( !NT_SUCCESS( ntStatus)) + *Fcb = pObjectInfo->Fcb; + + if( !NT_SUCCESS( ntStatus) && + ntStatus != STATUS_REPARSE) { AFSDbgLogMsg( AFS_SUBSYSTEM_FILE_PROCESSING, @@ -3084,7 +3108,13 @@ AFSProcessOverwriteSupersede( IN PDEVICE_OBJECT DeviceObject, try_return( ntStatus); } - bAllocatedFcb = TRUE; + if ( ntStatus != STATUS_REPARSE) + { + + bAllocatedFcb = TRUE; + } + + ntStatus = STATUS_SUCCESS; } else { @@ -3476,10 +3506,12 @@ AFSOpenIOCtlFcb( IN PIRP Irp, // Allocate and initialize the Fcb for the file. // - ntStatus = AFSInitFcb( pParentObjectInfo->Specific.Directory.PIOCtlDirectoryCB, - Fcb); + ntStatus = AFSInitFcb( pParentObjectInfo->Specific.Directory.PIOCtlDirectoryCB); - if( !NT_SUCCESS( ntStatus)) + *Fcb = pParentObjectInfo->Specific.Directory.PIOCtlDirectoryCB->ObjectInformation->Fcb; + + if( !NT_SUCCESS( ntStatus) && + ntStatus != STATUS_REPARSE) { AFSDbgLogMsg( AFS_SUBSYSTEM_FILE_PROCESSING, @@ -3491,7 +3523,13 @@ AFSOpenIOCtlFcb( IN PIRP Irp, try_return( ntStatus); } - bAllocatedFcb = TRUE; + if ( ntStatus != STATUS_REPARSE) + { + + bAllocatedFcb = TRUE; + } + + ntStatus = STATUS_SUCCESS; } else { @@ -3734,10 +3772,12 @@ AFSOpenSpecialShareFcb( IN PIRP Irp, // Allocate and initialize the Fcb for the file. // - ntStatus = AFSInitFcb( DirectoryCB, - Fcb); + ntStatus = AFSInitFcb( DirectoryCB); - if( !NT_SUCCESS( ntStatus)) + *Fcb = DirectoryCB->ObjectInformation->Fcb; + + if( !NT_SUCCESS( ntStatus) && + ntStatus != STATUS_REPARSE) { AFSDbgLogMsg( AFS_SUBSYSTEM_PIPE_PROCESSING, @@ -3749,7 +3789,13 @@ AFSOpenSpecialShareFcb( IN PIRP Irp, try_return( ntStatus); } - bAllocateFcb = TRUE; + if ( ntStatus != STATUS_REPARSE) + { + + bAllocateFcb = TRUE; + } + + ntStatus = STATUS_SUCCESS; } else { diff --git a/src/WINNT/afsrdr/kernel/lib/AFSFcbSupport.cpp b/src/WINNT/afsrdr/kernel/lib/AFSFcbSupport.cpp index 73b9702a5..0ea85fac5 100644 --- a/src/WINNT/afsrdr/kernel/lib/AFSFcbSupport.cpp +++ b/src/WINNT/afsrdr/kernel/lib/AFSFcbSupport.cpp @@ -51,8 +51,7 @@ // NTSTATUS -AFSInitFcb( IN AFSDirectoryCB *DirEntry, - IN OUT AFSFcb **Fcb) +AFSInitFcb( IN AFSDirectoryCB *DirEntry) { NTSTATUS ntStatus = STATUS_SUCCESS; @@ -128,6 +127,7 @@ AFSInitFcb( IN AFSDirectoryCB *DirEntry, sizeof( AFSNonPagedFcb)); pNPFcb->Size = sizeof( AFSNonPagedFcb); + pNPFcb->Type = AFS_NON_PAGED_FCB; // @@ -167,14 +167,6 @@ AFSInitFcb( IN AFSDirectoryCB *DirEntry, pFcb->NPFcb = pNPFcb; - // - // Initialize some fields in the Fcb - // - - pFcb->ObjectInformation = pObjectInfo; - - pObjectInfo->Fcb = pFcb; - // // Set type specific information // @@ -280,26 +272,52 @@ AFSInitFcb( IN AFSDirectoryCB *DirEntry, } // - // And return the Fcb + // Initialize some fields in the Fcb // - *Fcb = pFcb; + if ( InterlockedCompareExchangePointer( (PVOID *)&pObjectInfo->Fcb, pFcb, NULL) != NULL) + { + + AFSDbgLogMsg( AFS_SUBSYSTEM_FILE_PROCESSING, + AFS_TRACE_LEVEL_WARNING, + "AFSInitFcb Raced Fcb %08lX pFcb %08lX Name %wZ\n", + pObjectInfo->Fcb, + pFcb, + &DirEntry->NameInformation.FileName); + + AFSDbgLogMsg( AFS_SUBSYSTEM_LOCK_PROCESSING, + AFS_TRACE_LEVEL_VERBOSE, + "AFSInitFcb Acquiring Fcb lock %08lX EXCL %08lX\n", + &pObjectInfo->Fcb->NPFcb->Resource, + PsGetCurrentThread()); + + AFSAcquireExcl( &pObjectInfo->Fcb->NPFcb->Resource, + TRUE); + + try_return( ntStatus = STATUS_REPARSE); + } + + pFcb->ObjectInformation = pObjectInfo; AFSDbgLogMsg( AFS_SUBSYSTEM_FCB_REF_COUNTING, AFS_TRACE_LEVEL_VERBOSE, "AFSInitFcb Initialized Fcb %08lX Name %wZ\n", - pFcb, + &pObjectInfo->Fcb, &DirEntry->NameInformation.FileName); try_exit: - if( !NT_SUCCESS( ntStatus)) + if( ntStatus != STATUS_SUCCESS) { - AFSDbgLogMsg( AFS_SUBSYSTEM_FILE_PROCESSING, - AFS_TRACE_LEVEL_ERROR, - "AFSInitFcb Failed to initialize fcb Status %08lX\n", - ntStatus); + if ( !NT_SUCCESS( ntStatus)) + { + + AFSDbgLogMsg( AFS_SUBSYSTEM_FILE_PROCESSING, + AFS_TRACE_LEVEL_ERROR, + "AFSInitFcb Failed to initialize fcb Status %08lX\n", + ntStatus); + } if( pFcb != NULL) { @@ -330,12 +348,6 @@ try_exit: AFSExFreePool( pNPFcb); } - - if( Fcb != NULL) - { - - *Fcb = NULL; - } } } diff --git a/src/WINNT/afsrdr/kernel/lib/Include/AFSCommon.h b/src/WINNT/afsrdr/kernel/lib/Include/AFSCommon.h index 9b57ff90d..197bb6b84 100644 --- a/src/WINNT/afsrdr/kernel/lib/Include/AFSCommon.h +++ b/src/WINNT/afsrdr/kernel/lib/Include/AFSCommon.h @@ -494,8 +494,7 @@ AFSClose( IN PDEVICE_OBJECT DeviceObject, // NTSTATUS -AFSInitFcb( IN AFSDirectoryCB *DirEntry, - IN OUT AFSFcb **Fcb); +AFSInitFcb( IN AFSDirectoryCB *DirEntry); NTSTATUS AFSInitVolume( IN GUID *AuthGroup, -- 2.39.5