From 76e33082d12eaeaaf87df53986b46a508c7ad68d Mon Sep 17 00:00:00 2001 From: Jeffrey Altman Date: Tue, 2 Apr 2013 14:13:57 -0400 Subject: [PATCH] Windows: AFSDeleteDirEntry set input to NULL AFSDeleteDirEntry() frees the memory allocated to the DirectoryCB. To ensure that an invalid memory pointer is not accidentally used by the caller after the memory is freed, use InterlockedCompareExchangePointer() to set the input parameter to NULL prior to destroying the DirectoryCB. Change-Id: I2e92d4277d1f9baee164bfb941821aa11a1ad738 Reviewed-on: http://gerrit.openafs.org/9721 Reviewed-by: Peter Scott Tested-by: BuildBot Reviewed-by: Jeffrey Altman Tested-by: Jeffrey Altman --- src/WINNT/afsrdr/kernel/lib/AFSClose.cpp | 2 +- .../afsrdr/kernel/lib/AFSCommSupport.cpp | 12 ++-- src/WINNT/afsrdr/kernel/lib/AFSFileInfo.cpp | 2 +- src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp | 6 +- .../afsrdr/kernel/lib/AFSNameSupport.cpp | 71 +++++++++++-------- src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp | 2 +- .../afsrdr/kernel/lib/Include/AFSCommon.h | 2 +- 7 files changed, 56 insertions(+), 41 deletions(-) diff --git a/src/WINNT/afsrdr/kernel/lib/AFSClose.cpp b/src/WINNT/afsrdr/kernel/lib/AFSClose.cpp index 1dd061ffc6..cddd8fb041 100644 --- a/src/WINNT/afsrdr/kernel/lib/AFSClose.cpp +++ b/src/WINNT/afsrdr/kernel/lib/AFSClose.cpp @@ -418,7 +418,7 @@ AFSClose( IN PDEVICE_OBJECT LibDeviceObject, // AFSDeleteDirEntry( pParentObjectInfo, - pDirCB); + &pDirCB); AFSAcquireShared( &pObjectInfo->NonPagedInfo->ObjectInfoLock, TRUE); diff --git a/src/WINNT/afsrdr/kernel/lib/AFSCommSupport.cpp b/src/WINNT/afsrdr/kernel/lib/AFSCommSupport.cpp index 85285dcbf3..f9edbbfcaa 100644 --- a/src/WINNT/afsrdr/kernel/lib/AFSCommSupport.cpp +++ b/src/WINNT/afsrdr/kernel/lib/AFSCommSupport.cpp @@ -364,7 +364,7 @@ AFSEnumerateDirectory( IN GUID *AuthGroup, pCurrentDirEntry->FileId.Unique)); AFSDeleteDirEntry( ObjectInfoCB, - pDirNode); + &pDirNode); } else { @@ -534,7 +534,7 @@ AFSEnumerateDirectory( IN GUID *AuthGroup, // AFSDeleteDirEntry( ObjectInfoCB, - pDirNode); + &pDirNode); pCurrentDirEntry = (AFSDirEnumEntry *)((char *)pCurrentDirEntry + ulEntryLength); @@ -1201,7 +1201,7 @@ AFSVerifyDirectoryContent( IN AFSObjectInfoCB *ObjectInfoCB, pCurrentDirEntry->FileId.Unique)); AFSDeleteDirEntry( ObjectInfoCB, - pDirNode); + &pDirNode); } else { @@ -1373,7 +1373,7 @@ AFSVerifyDirectoryContent( IN AFSObjectInfoCB *ObjectInfoCB, // AFSDeleteDirEntry( ObjectInfoCB, - pDirNode); + &pDirNode); pCurrentDirEntry = (AFSDirEnumEntry *)((char *)pCurrentDirEntry + ulEntryLength); @@ -1732,7 +1732,7 @@ AFSNotifyFileCreate( IN GUID *AuthGroup, pResultCB->DirEnum.FileId.Unique)); AFSDeleteDirEntry( ParentObjectInfo, - pDirNode); + &pDirNode); } else { @@ -2341,7 +2341,7 @@ AFSNotifyHardLink( IN AFSObjectInfoCB *ObjectInfo, pResultCB->DirEnum.FileId.Unique)); AFSDeleteDirEntry( TargetParentObjectInfo, - pDirNode); + &pDirNode); } else { diff --git a/src/WINNT/afsrdr/kernel/lib/AFSFileInfo.cpp b/src/WINNT/afsrdr/kernel/lib/AFSFileInfo.cpp index 39103e7c73..aae6d8c9c4 100644 --- a/src/WINNT/afsrdr/kernel/lib/AFSFileInfo.cpp +++ b/src/WINNT/afsrdr/kernel/lib/AFSFileInfo.cpp @@ -3316,7 +3316,7 @@ AFSSetRenameInfo( IN PIRP Irp) &pTargetDirEntry->NameInformation.FileName)); AFSDeleteDirEntry( pTargetParentObject, - pTargetDirEntry); + &pTargetDirEntry); } pTargetDirEntry = NULL; diff --git a/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp b/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp index 3524907aa0..2a86401ed6 100644 --- a/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp +++ b/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp @@ -3582,7 +3582,7 @@ AFSValidateDirectoryCache( IN AFSObjectInfoCB *ObjectInfo, &pCurrentDirEntry->NameInformation.FileName)); AFSDeleteDirEntry( ObjectInfo, - pCurrentDirEntry); + &pCurrentDirEntry); } else { @@ -3715,7 +3715,7 @@ AFSValidateDirectoryCache( IN AFSObjectInfoCB *ObjectInfo, ObjectInfo->FileId.Unique)); AFSDeleteDirEntry( ObjectInfo, - pCurrentDirEntry); + &pCurrentDirEntry); } else { @@ -4909,7 +4909,7 @@ AFSResetDirectoryContent( IN AFSObjectInfoCB *ObjectInfoCB) &pCurrentDirEntry->NameInformation.FileName)); AFSDeleteDirEntry( ObjectInfoCB, - pCurrentDirEntry); + &pCurrentDirEntry); } else { diff --git a/src/WINNT/afsrdr/kernel/lib/AFSNameSupport.cpp b/src/WINNT/afsrdr/kernel/lib/AFSNameSupport.cpp index aaae87dc8a..64f27607c6 100644 --- a/src/WINNT/afsrdr/kernel/lib/AFSNameSupport.cpp +++ b/src/WINNT/afsrdr/kernel/lib/AFSNameSupport.cpp @@ -1986,7 +1986,7 @@ AFSLocateNameEntry( IN GUID *AuthGroup, // AFSDeleteDirEntry( pParentObjectInfo, - pDirEntry); + &pDirEntry); AFSAcquireShared( &pCurrentObject->NonPagedInfo->ObjectInfoLock, TRUE); @@ -2477,7 +2477,7 @@ AFSCreateDirEntry( IN GUID *AuthGroup, lCount)); AFSDeleteDirEntry( ParentObjectInfo, - pDirNode); + &pDirNode); lCount = InterlockedIncrement( &pExistingDirNode->DirOpenReferenceCount); @@ -2521,7 +2521,7 @@ AFSCreateDirEntry( IN GUID *AuthGroup, pDirNode->ObjectInformation->FileId.Unique)); AFSDeleteDirEntry( ParentObjectInfo, - pExistingDirNode); + &pExistingDirNode); } else { @@ -2755,72 +2755,83 @@ AFSInsertDirectoryNode( IN AFSObjectInfoCB *ParentObjectInfo, void AFSDeleteDirEntry( IN AFSObjectInfoCB *ParentObjectInfo, - IN AFSDirectoryCB *DirEntry) + IN AFSDirectoryCB **ppDirEntry) { LONG lCount; + AFSDirectoryCB *pDirEntry; __Enter { + pDirEntry = (AFSDirectoryCB *) InterlockedCompareExchangePointer( (PVOID *)ppDirEntry, + NULL, + *ppDirEntry); + + if ( pDirEntry == NULL) + { + + try_return( NOTHING); + } + AFSDbgTrace(( AFS_SUBSYSTEM_CLEANUP_PROCESSING | AFS_SUBSYSTEM_DIRENTRY_REF_COUNTING, AFS_TRACE_LEVEL_VERBOSE, - "AFSDeleteDirEntry Deleting dir entry in parent %p Entry %p object %p %wZ RefCount %d\n", + "AFSDeletepDirEntry Deleting dir entry in parent %p Entry %p object %p %wZ RefCount %d\n", ParentObjectInfo, - DirEntry, - DirEntry->ObjectInformation, - &DirEntry->NameInformation.FileName, - DirEntry->DirOpenReferenceCount)); + pDirEntry, + pDirEntry->ObjectInformation, + &pDirEntry->NameInformation.FileName, + pDirEntry->DirOpenReferenceCount)); - ASSERT( DirEntry->DirOpenReferenceCount == 0); + ASSERT( pDirEntry->DirOpenReferenceCount == 0); AFSRemoveDirNodeFromParent( ParentObjectInfo, - DirEntry, + pDirEntry, TRUE); // // Free up the name buffer if it was reallocated // - if( BooleanFlagOn( DirEntry->Flags, AFS_DIR_RELEASE_NAME_BUFFER)) + if( BooleanFlagOn( pDirEntry->Flags, AFS_DIR_RELEASE_NAME_BUFFER)) { - AFSExFreePoolWithTag( DirEntry->NameInformation.FileName.Buffer, 0); + AFSExFreePoolWithTag( pDirEntry->NameInformation.FileName.Buffer, 0); } - if( BooleanFlagOn( DirEntry->Flags, AFS_DIR_RELEASE_TARGET_NAME_BUFFER)) + if( BooleanFlagOn( pDirEntry->Flags, AFS_DIR_RELEASE_TARGET_NAME_BUFFER)) { - AFSExFreePoolWithTag( DirEntry->NameInformation.TargetName.Buffer, 0); + AFSExFreePoolWithTag( pDirEntry->NameInformation.TargetName.Buffer, 0); } - if ( DirEntry->ObjectInformation != NULL) + if ( pDirEntry->ObjectInformation != NULL) { - if( BooleanFlagOn( DirEntry->Flags, AFS_DIR_ENTRY_DELETED) && - DirEntry->ObjectInformation->Links == 0) + if( BooleanFlagOn( pDirEntry->Flags, AFS_DIR_ENTRY_DELETED) && + pDirEntry->ObjectInformation->Links == 0) { - SetFlag( DirEntry->ObjectInformation->Flags, AFS_OBJECT_FLAGS_DELETED); + SetFlag( pDirEntry->ObjectInformation->Flags, AFS_OBJECT_FLAGS_DELETED); } // // Dereference the object for this dir entry // - lCount = AFSObjectInfoDecrement( DirEntry->ObjectInformation, + lCount = AFSObjectInfoDecrement( pDirEntry->ObjectInformation, AFS_OBJECT_REFERENCE_DIRENTRY); AFSDbgTrace(( AFS_SUBSYSTEM_OBJECT_REF_COUNTING, AFS_TRACE_LEVEL_VERBOSE, - "AFSDeleteDirEntry Decrement count on object %p Cnt %d\n", - DirEntry->ObjectInformation, + "AFSDeletepDirEntry Decrement count on object %p Cnt %d\n", + pDirEntry->ObjectInformation, lCount)); } - ExDeleteResourceLite( &DirEntry->NonPaged->Lock); + ExDeleteResourceLite( &pDirEntry->NonPaged->Lock); - AFSExFreePoolWithTag( DirEntry->NonPaged, AFS_DIR_ENTRY_NP_TAG); + AFSExFreePoolWithTag( pDirEntry->NonPaged, AFS_DIR_ENTRY_NP_TAG); // // Free up the dir entry @@ -2828,10 +2839,14 @@ AFSDeleteDirEntry( IN AFSObjectInfoCB *ParentObjectInfo, AFSDbgTrace(( AFS_SUBSYSTEM_DIRENTRY_ALLOCATION, AFS_TRACE_LEVEL_VERBOSE, - "AFSDeleteDirEntry AFS_DIR_ENTRY_TAG deallocating %p\n", - DirEntry)); + "AFSDeletepDirEntry AFS_DIR_ENTRY_TAG deallocating %p\n", + pDirEntry)); - AFSExFreePoolWithTag( DirEntry, AFS_DIR_ENTRY_TAG); + AFSExFreePoolWithTag( pDirEntry, AFS_DIR_ENTRY_TAG); + +try_exit: + + NOTHING; } } @@ -4325,7 +4340,7 @@ AFSCheckCellName( IN GUID *AuthGroup, { AFSDeleteDirEntry( &AFSGlobalRoot->ObjectInformation, - pDirNode); + &pDirNode); AFSReleaseResource( AFSGlobalRoot->ObjectInformation.Specific.Directory.DirectoryNodeHdr.TreeLock); diff --git a/src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp b/src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp index 2310268e54..490438d0d7 100644 --- a/src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp +++ b/src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp @@ -1322,7 +1322,7 @@ AFSExamineObjectInfo( IN AFSObjectInfoCB * pCurrentObject, pCurrentDirEntry->ObjectInformation)); AFSDeleteDirEntry( pCurrentObject, - pCurrentDirEntry); + &pCurrentDirEntry); pCurrentDirEntry = pNextDirEntry; } diff --git a/src/WINNT/afsrdr/kernel/lib/Include/AFSCommon.h b/src/WINNT/afsrdr/kernel/lib/Include/AFSCommon.h index 8e6b8ab95b..50aee252de 100644 --- a/src/WINNT/afsrdr/kernel/lib/Include/AFSCommon.h +++ b/src/WINNT/afsrdr/kernel/lib/Include/AFSCommon.h @@ -591,7 +591,7 @@ AFSInsertDirectoryNode( IN AFSObjectInfoCB *ParentObjectInfo, void AFSDeleteDirEntry( IN AFSObjectInfoCB *ParentObjectInfo, - IN AFSDirectoryCB *DirEntry); + IN AFSDirectoryCB **ppDirEntry); NTSTATUS AFSRemoveDirNodeFromParent( IN AFSObjectInfoCB *ParentObjectInfo,