https://git.reactos.org/?p=reactos.git;a=commitdiff;h=da8a41b97bef45f28a1c85...
commit da8a41b97bef45f28a1c85c17b99b1bfe22b4008 Author: Pierre Schweitzer pierre@reactos.org AuthorDate: Tue Mar 6 20:22:50 2018 +0100 Commit: Pierre Schweitzer pierre@reactos.org CommitDate: Tue Mar 6 20:30:21 2018 +0100
[SHELL32] Fix a directory handle leak when browsing folders
A bit of history: in r71528, I tried to fix our explorer often crashing while browsing directories. It was linked to the fact that a notification result may arrive while the notification structure had already been deleted.
The fix for this was actually broken and was leading to a double leak: the notification structure was leaked. But also the handle to the directory that had been browsed! This means that the directory couldn't be modified anymore as a leaked handle to it was still open.
Actually, when notifications are cancel, the kernel properly calls the notification routine, but with a specific error code. So the correct fix is to stop handling that notification when we receive this error code. This is the correct fix with no leaks.
This commit is a complete r71528 revert with the appropriate fix.
CORE-10941 CORE-12843 --- dll/win32/shell32/wine/changenotify.c | 56 ++++++++--------------------------- 1 file changed, 12 insertions(+), 44 deletions(-)
diff --git a/dll/win32/shell32/wine/changenotify.c b/dll/win32/shell32/wine/changenotify.c index 0157297e1b..05c7284a87 100644 --- a/dll/win32/shell32/wine/changenotify.c +++ b/dll/win32/shell32/wine/changenotify.c @@ -59,7 +59,6 @@ typedef struct { OVERLAPPED overlapped; /* Overlapped structure */ BYTE *buffer; /* Async buffer to fill */ BYTE *backBuffer; /* Back buffer to swap buffer into */ - struct _NOTIFICATIONLIST * pParent; } SHChangeNotifyEntryInternal, *LPNOTIFYREGISTER; #else typedef SHChangeNotifyEntry *LPNOTIFYREGISTER; @@ -76,9 +75,6 @@ typedef struct _NOTIFICATIONLIST LONG wEventMask; /* subscribed events */ DWORD dwFlags; /* client flags */ ULONG id; -#ifdef __REACTOS__ - volatile LONG wQueuedCount; -#endif } NOTIFICATIONLIST, *LPNOTIFICATIONLIST;
#ifdef __REACTOS__ @@ -161,22 +157,9 @@ static const char * NodeName(const NOTIFICATIONLIST *item) static void DeleteNode(LPNOTIFICATIONLIST item) { UINT i; -#ifdef __REACTOS__ - LONG queued; -#endif
TRACE("item=%p\n", item);
-#ifdef __REACTOS__ - queued = InterlockedCompareExchange(&item->wQueuedCount, 0, 0); - if (queued != 0) - { - ERR("Not freeing, still %d queued events\n", queued); - return; - } - TRACE("Freeing for real! %p (%d) \n", item, item->cidl); -#endif - /* remove item from list */ list_remove( &item->entry );
@@ -253,7 +236,6 @@ SHChangeNotifyRegister( item->cidl = cItems; #ifdef __REACTOS__ item->apidl = SHAlloc(sizeof(SHChangeNotifyEntryInternal) * cItems); - item->wQueuedCount = 0; #else item->apidl = SHAlloc(sizeof(SHChangeNotifyEntry) * cItems); #endif @@ -268,13 +250,11 @@ SHChangeNotifyRegister( item->apidl[i].buffer = SHAlloc(BUFFER_SIZE); item->apidl[i].backBuffer = SHAlloc(BUFFER_SIZE); item->apidl[i].overlapped.hEvent = &item->apidl[i]; - item->apidl[i].pParent = item;
if (fSources & SHCNRF_InterruptLevel) { if (_OpenDirectory( &item->apidl[i] )) { - InterlockedIncrement(&item->wQueuedCount); QueueUserAPC( _AddDirectoryProc, m_hThread, (ULONG_PTR) &item->apidl[i] ); } } @@ -738,8 +718,18 @@ _NotificationCompletion(DWORD dwErrorCode, // completion code if (dwErrorCode == ERROR_INVALID_FUNCTION) { WARN("Directory watching not supported\n"); - goto quit; + return; + } + + /* Also, if the notify operation was canceled (like, user moved to another + * directory), then, don't requeue notification + */ + if (dwErrorCode == ERROR_OPERATION_ABORTED) + { + TRACE("Notification aborted\n"); + return; } + #endif
/* This likely means overflow, so force whole directory refresh. */ @@ -755,11 +745,7 @@ _NotificationCompletion(DWORD dwErrorCode, // completion code item->pidl, NULL);
-#ifdef __REACTOS__ - goto quit; -#else return; -#endif }
/* @@ -776,21 +762,12 @@ _NotificationCompletion(DWORD dwErrorCode, // completion code _BeginRead(item);
_ProcessNotification(item); - -#ifdef __REACTOS__ -quit: - InterlockedDecrement(&item->pParent->wQueuedCount); - DeleteNode(item->pParent); -#endif }
static VOID _BeginRead(LPNOTIFYREGISTER item ) { TRACE("_BeginRead %p \n", item->hDirectory);
-#ifdef __REACTOS__ - InterlockedIncrement(&item->pParent->wQueuedCount); -#endif /* This call needs to be reissued after every APC. */ if (!ReadDirectoryChangesW(item->hDirectory, // handle to directory item->buffer, // read results buffer @@ -800,22 +777,13 @@ static VOID _BeginRead(LPNOTIFYREGISTER item ) NULL, // bytes returned &item->overlapped, // overlapped buffer _NotificationCompletion)) // completion routine -#ifdef __REACTOS__ - { -#endif - ERR("ReadDirectoryChangesW failed. (%p, %p, %p, %p, %p, %p) Code: %u \n", + ERR("ReadDirectoryChangesW failed. (%p, %p, %p, %p, %p) Code: %u \n", item, - item->pParent, item->hDirectory, item->buffer, &item->overlapped, _NotificationCompletion, GetLastError()); -#ifdef __REACTOS__ - InterlockedDecrement(&item->pParent->wQueuedCount); - DeleteNode(item->pParent); - } -#endif }
DWORD _MapAction(DWORD dwAction, BOOL isDir)
-
Pierre Schweitzer