[0.4.7][SHELL32] Fix a directory handle leak when browsing folders

A bit of history: in SVN r71528 == git
4322caeede
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

cherry picked from commit 0.4.9-dev-144-g
da8a41b97b

Joachim Henze: I suspect that this patch will introduce
CORE-15703 but I think it's an improvement even if it would.
This commit is contained in:
Joachim Henze 2020-10-06 16:24:44 +02:00
parent 3abdd259bb
commit 2d9dabac60

View file

@ -59,7 +59,6 @@ typedef struct {
OVERLAPPED overlapped; /* Overlapped structure */ OVERLAPPED overlapped; /* Overlapped structure */
BYTE *buffer; /* Async buffer to fill */ BYTE *buffer; /* Async buffer to fill */
BYTE *backBuffer; /* Back buffer to swap buffer into */ BYTE *backBuffer; /* Back buffer to swap buffer into */
struct _NOTIFICATIONLIST * pParent;
} SHChangeNotifyEntryInternal, *LPNOTIFYREGISTER; } SHChangeNotifyEntryInternal, *LPNOTIFYREGISTER;
#else #else
typedef SHChangeNotifyEntry *LPNOTIFYREGISTER; typedef SHChangeNotifyEntry *LPNOTIFYREGISTER;
@ -76,9 +75,6 @@ typedef struct _NOTIFICATIONLIST
LONG wEventMask; /* subscribed events */ LONG wEventMask; /* subscribed events */
DWORD dwFlags; /* client flags */ DWORD dwFlags; /* client flags */
ULONG id; ULONG id;
#ifdef __REACTOS__
volatile LONG wQueuedCount;
#endif
} NOTIFICATIONLIST, *LPNOTIFICATIONLIST; } NOTIFICATIONLIST, *LPNOTIFICATIONLIST;
#ifdef __REACTOS__ #ifdef __REACTOS__
@ -161,22 +157,9 @@ static const char * NodeName(const NOTIFICATIONLIST *item)
static void DeleteNode(LPNOTIFICATIONLIST item) static void DeleteNode(LPNOTIFICATIONLIST item)
{ {
UINT i; UINT i;
#ifdef __REACTOS__
LONG queued;
#endif
TRACE("item=%p\n", item); 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 */ /* remove item from list */
list_remove( &item->entry ); list_remove( &item->entry );
@ -253,7 +236,6 @@ SHChangeNotifyRegister(
item->cidl = cItems; item->cidl = cItems;
#ifdef __REACTOS__ #ifdef __REACTOS__
item->apidl = SHAlloc(sizeof(SHChangeNotifyEntryInternal) * cItems); item->apidl = SHAlloc(sizeof(SHChangeNotifyEntryInternal) * cItems);
item->wQueuedCount = 0;
#else #else
item->apidl = SHAlloc(sizeof(SHChangeNotifyEntry) * cItems); item->apidl = SHAlloc(sizeof(SHChangeNotifyEntry) * cItems);
#endif #endif
@ -268,13 +250,11 @@ SHChangeNotifyRegister(
item->apidl[i].buffer = SHAlloc(BUFFER_SIZE); item->apidl[i].buffer = SHAlloc(BUFFER_SIZE);
item->apidl[i].backBuffer = SHAlloc(BUFFER_SIZE); item->apidl[i].backBuffer = SHAlloc(BUFFER_SIZE);
item->apidl[i].overlapped.hEvent = &item->apidl[i]; item->apidl[i].overlapped.hEvent = &item->apidl[i];
item->apidl[i].pParent = item;
if (fSources & SHCNRF_InterruptLevel) if (fSources & SHCNRF_InterruptLevel)
{ {
if (_OpenDirectory( &item->apidl[i] )) if (_OpenDirectory( &item->apidl[i] ))
{ {
InterlockedIncrement(&item->wQueuedCount);
QueueUserAPC( _AddDirectoryProc, m_hThread, (ULONG_PTR) &item->apidl[i] ); QueueUserAPC( _AddDirectoryProc, m_hThread, (ULONG_PTR) &item->apidl[i] );
} }
} }
@ -738,8 +718,18 @@ _NotificationCompletion(DWORD dwErrorCode, // completion code
if (dwErrorCode == ERROR_INVALID_FUNCTION) if (dwErrorCode == ERROR_INVALID_FUNCTION)
{ {
WARN("Directory watching not supported\n"); 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 #endif
/* This likely means overflow, so force whole directory refresh. */ /* This likely means overflow, so force whole directory refresh. */
@ -755,11 +745,7 @@ _NotificationCompletion(DWORD dwErrorCode, // completion code
item->pidl, item->pidl,
NULL); NULL);
#ifdef __REACTOS__
goto quit;
#else
return; return;
#endif
} }
/* /*
@ -776,21 +762,12 @@ _NotificationCompletion(DWORD dwErrorCode, // completion code
_BeginRead(item); _BeginRead(item);
_ProcessNotification(item); _ProcessNotification(item);
#ifdef __REACTOS__
quit:
InterlockedDecrement(&item->pParent->wQueuedCount);
DeleteNode(item->pParent);
#endif
} }
static VOID _BeginRead(LPNOTIFYREGISTER item ) static VOID _BeginRead(LPNOTIFYREGISTER item )
{ {
TRACE("_BeginRead %p \n", item->hDirectory); TRACE("_BeginRead %p \n", item->hDirectory);
#ifdef __REACTOS__
InterlockedIncrement(&item->pParent->wQueuedCount);
#endif
/* This call needs to be reissued after every APC. */ /* This call needs to be reissued after every APC. */
if (!ReadDirectoryChangesW(item->hDirectory, // handle to directory if (!ReadDirectoryChangesW(item->hDirectory, // handle to directory
item->buffer, // read results buffer item->buffer, // read results buffer
@ -800,22 +777,13 @@ static VOID _BeginRead(LPNOTIFYREGISTER item )
NULL, // bytes returned NULL, // bytes returned
&item->overlapped, // overlapped buffer &item->overlapped, // overlapped buffer
_NotificationCompletion)) // completion routine _NotificationCompletion)) // completion routine
#ifdef __REACTOS__ ERR("ReadDirectoryChangesW failed. (%p, %p, %p, %p, %p) Code: %u \n",
{
#endif
ERR("ReadDirectoryChangesW failed. (%p, %p, %p, %p, %p, %p) Code: %u \n",
item, item,
item->pParent,
item->hDirectory, item->hDirectory,
item->buffer, item->buffer,
&item->overlapped, &item->overlapped,
_NotificationCompletion, _NotificationCompletion,
GetLastError()); GetLastError());
#ifdef __REACTOS__
InterlockedDecrement(&item->pParent->wQueuedCount);
DeleteNode(item->pParent);
}
#endif
} }
DWORD _MapAction(DWORD dwAction, BOOL isDir) DWORD _MapAction(DWORD dwAction, BOOL isDir)