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 +- src/WINNT/afsrdr/kernel/lib/AFSCommSupport.cpp | 12 ++--- src/WINNT/afsrdr/kernel/lib/AFSFileInfo.cpp | 2 +- src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp | 6 +-- src/WINNT/afsrdr/kernel/lib/AFSNameSupport.cpp | 71 +++++++++++++++---------- src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp | 2 +- src/WINNT/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 1dd061f..cddd8fb 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 85285dc..f9edbbf 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 39103e7..aae6d8c 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 3524907..2a86401 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 aaae87d..64f2760 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( pDirEntry, AFS_DIR_ENTRY_TAG); - AFSExFreePoolWithTag( DirEntry, 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 2310268..490438d 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 8e6b8ab..50aee25 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, -- 1.9.4