From 3a52e0f36289eb0440afec778131199cd9e0bec7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Ludwig?= Date: Sat, 3 Mar 2018 18:31:26 +0100 Subject: [PATCH 1/3] Fix cast from HANDLE to descriptor in the WinAPI driver. Avoids truncation to 32-bit for 64-bit builds. Also improves robustness of the directory watcher shutdown procedure. --- source/eventcore/drivers/winapi/driver.d | 2 +- source/eventcore/drivers/winapi/events.d | 2 +- source/eventcore/drivers/winapi/files.d | 18 +++++++++++------- source/eventcore/drivers/winapi/watchers.d | 10 +++++++--- 4 files changed, 20 insertions(+), 12 deletions(-) diff --git a/source/eventcore/drivers/winapi/driver.d b/source/eventcore/drivers/winapi/driver.d index 91d0992..06ba834 100644 --- a/source/eventcore/drivers/winapi/driver.d +++ b/source/eventcore/drivers/winapi/driver.d @@ -21,7 +21,7 @@ import eventcore.drivers.winapi.watchers; import core.sys.windows.windows; static assert(HANDLE.sizeof <= FD.BaseType.sizeof); -static assert(FD(cast(int)INVALID_HANDLE_VALUE) == FD.init); +static assert(FD(cast(size_t)INVALID_HANDLE_VALUE) == FD.init); final class WinAPIEventDriver : EventDriver { diff --git a/source/eventcore/drivers/winapi/events.d b/source/eventcore/drivers/winapi/events.d index 23884e6..c39049a 100644 --- a/source/eventcore/drivers/winapi/events.d +++ b/source/eventcore/drivers/winapi/events.d @@ -142,6 +142,6 @@ final class WinAPIEventDriverEvents : EventDriverEvents { private static HANDLE idToHandle(EventID event) @trusted { - return cast(HANDLE)cast(int)event; + return cast(HANDLE)cast(size_t)event; } } diff --git a/source/eventcore/drivers/winapi/files.d b/source/eventcore/drivers/winapi/files.d index 6ba6159..09492d7 100644 --- a/source/eventcore/drivers/winapi/files.d +++ b/source/eventcore/drivers/winapi/files.d @@ -43,24 +43,28 @@ final class WinAPIEventDriverFiles : EventDriverFiles { BOOL ret = SetEndOfFile(handle); if (!ret) { CloseHandle(handle); - return FileFD.init; + return FileFD.invalid; } } - return adopt(cast(int)handle); + return adoptInternal(handle); } override FileFD adopt(int system_handle) { - auto handle = () @trusted { return cast(HANDLE)system_handle; } (); + return adoptInternal(() @trusted { return cast(HANDLE)system_handle; } ()); + } + + private FileFD adoptInternal(HANDLE handle) + { DWORD f; if (!() @trusted { return GetHandleInformation(handle, &f); } ()) - return FileFD.init; + return FileFD.invalid; auto s = m_core.setupSlot!FileSlot(handle); s.read.handle = s.write.handle = handle; - return FileFD(system_handle); + return FileFD(cast(size_t)handle); } override void close(FileFD file) @@ -182,7 +186,7 @@ final class WinAPIEventDriverFiles : EventDriverFiles { auto slot = () @trusted { return cast(FileSlot.Direction!RO*)overlapped.hEvent; } (); assert(slot !is null); HANDLE h = slot.handle; - auto id = FileFD(cast(int)h); + auto id = FileFD(cast(size_t)h); if (!slot.callback) { // request was already cancelled @@ -208,6 +212,6 @@ final class WinAPIEventDriverFiles : EventDriverFiles { private static HANDLE idToHandle(FileFD id) @trusted { - return cast(HANDLE)cast(int)id; + return cast(HANDLE)cast(size_t)id; } } diff --git a/source/eventcore/drivers/winapi/watchers.d b/source/eventcore/drivers/winapi/watchers.d index 6d1d60b..1928bce 100644 --- a/source/eventcore/drivers/winapi/watchers.d +++ b/source/eventcore/drivers/winapi/watchers.d @@ -35,7 +35,7 @@ final class WinAPIEventDriverWatchers : EventDriverWatchers { if (handle == INVALID_HANDLE_VALUE) return WatcherID.invalid; - auto id = WatcherID(cast(int)handle); + auto id = WatcherID(cast(size_t)handle); auto slot = m_core.setupSlot!WatcherSlot(handle); slot.directory = path; @@ -80,6 +80,7 @@ final class WinAPIEventDriverWatchers : EventDriverWatchers { try theAllocator.dispose(slot.watcher.buffer); catch (Exception e) assert(false, "Freeing directory watcher buffer failed."); } (); + slot.watcher.buffer = null; core.freeSlot(handle); })) { @@ -91,6 +92,7 @@ final class WinAPIEventDriverWatchers : EventDriverWatchers { // completion callback if (slot.refCount == 1) { () @trusted { CancelIoEx(handle, &slot.watcher.overlapped); } (); + slot.watcher.callback = null; core.removeWaiter(); } @@ -105,7 +107,7 @@ final class WinAPIEventDriverWatchers : EventDriverWatchers { import std.path : dirName, baseName, buildPath; auto handle = overlapped.hEvent; // *file* handle - auto id = WatcherID(cast(int)handle); + auto id = WatcherID(cast(size_t)handle); auto gslot = () @trusted { return &WinAPIEventDriver.threadInstance.core.m_handles[handle]; } (); auto slot = () @trusted { return &gslot.watcher(); } (); @@ -118,6 +120,8 @@ final class WinAPIEventDriverWatchers : EventDriverWatchers { return; } + if (!slot.callback) return; + // NOTE: cbTransferred can be 0 if the buffer overflowed ubyte[] result = slot.buffer[0 .. cbTransferred]; while (result.length) { @@ -177,5 +181,5 @@ final class WinAPIEventDriverWatchers : EventDriverWatchers { return true; } - static private HANDLE idToHandle(WatcherID id) @trusted { return cast(HANDLE)cast(int)id; } + static private HANDLE idToHandle(WatcherID id) @trusted { return cast(HANDLE)cast(size_t)id; } } From fbd69273770a0eb8af1962fd2f8ba16f5f379f2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Ludwig?= Date: Sat, 3 Mar 2018 18:36:07 +0100 Subject: [PATCH 2/3] Improve robustness of file I/O callbacks for the WinAPI driver. Explicitly gets the read/write slot from the handle map from within the callback instead of dereferencing the pointer assigned when starting the operation. This makes sure that no dangling reference is accessed. Instead, if the slot got reused before the callback is invoked (which is a bug), an assertion will be triggered if the slot now has a different handle type. --- source/eventcore/drivers/winapi/core.d | 12 +++++--- source/eventcore/drivers/winapi/files.d | 39 ++++++++++++++----------- 2 files changed, 30 insertions(+), 21 deletions(-) diff --git a/source/eventcore/drivers/winapi/core.d b/source/eventcore/drivers/winapi/core.d index b78c87f..b3f4dde 100644 --- a/source/eventcore/drivers/winapi/core.d +++ b/source/eventcore/drivers/winapi/core.d @@ -219,22 +219,20 @@ private struct HandleSlot { package struct FileSlot { static struct Direction(bool RO) { - OVERLAPPED overlapped; - WinAPIEventDriverCore core; + OVERLAPPED_FILE overlapped; FileIOCallback callback; ulong offset; size_t bytesTransferred; IOMode mode; static if (RO) const(ubyte)[] buffer; else ubyte[] buffer; - HANDLE handle; // set to INVALID_HANDLE_VALUE when closed void invokeCallback(IOStatus status, size_t bytes_transferred) @safe nothrow { auto cb = this.callback; this.callback = null; assert(cb !is null); - cb(FileFD(cast(int)this.handle), status, bytes_transferred); + cb(overlapped.handle, status, bytes_transferred); } } Direction!false read; @@ -248,3 +246,9 @@ package struct WatcherSlot { bool recursive; FileChangesCallback callback; } + +package struct OVERLAPPED_FILE { + OVERLAPPED overlapped; + WinAPIEventDriverCore driver; + FileFD handle; +} diff --git a/source/eventcore/drivers/winapi/files.d b/source/eventcore/drivers/winapi/files.d index 09492d7..5f42f8d 100644 --- a/source/eventcore/drivers/winapi/files.d +++ b/source/eventcore/drivers/winapi/files.d @@ -62,7 +62,11 @@ final class WinAPIEventDriverFiles : EventDriverFiles { return FileFD.invalid; auto s = m_core.setupSlot!FileSlot(handle); - s.read.handle = s.write.handle = handle; + + s.read.overlapped.driver = m_core; + s.read.overlapped.handle = FileFD(cast(size_t)handle); + s.write.overlapped.driver = m_core; + s.write.overlapped.handle = FileFD(cast(size_t)handle); return FileFD(cast(size_t)handle); } @@ -71,9 +75,9 @@ final class WinAPIEventDriverFiles : EventDriverFiles { { auto h = idToHandle(file); auto slot = () @trusted { return &m_core.m_handles[h].file(); } (); - if (slot.read.handle != INVALID_HANDLE_VALUE) { + if (slot.read.overlapped.handle != FileFD.invalid) { CloseHandle(h); - slot.read.handle = slot.write.handle = INVALID_HANDLE_VALUE; + slot.read.overlapped.handle = slot.write.overlapped.handle = FileFD.invalid; } } @@ -99,7 +103,6 @@ final class WinAPIEventDriverFiles : EventDriverFiles { slot.offset = offset; slot.buffer = buffer; slot.mode = mode; - slot.core = m_core; slot.callback = on_write_finish; m_core.addWaiter(); startIO!(WriteFileEx, true)(h, slot); @@ -118,7 +121,6 @@ final class WinAPIEventDriverFiles : EventDriverFiles { slot.offset = offset; slot.buffer = buffer; slot.mode = mode; - slot.core = m_core; slot.callback = on_read_finish; m_core.addWaiter(); startIO!(ReadFileEx, false)(h, slot); @@ -154,7 +156,7 @@ final class WinAPIEventDriverFiles : EventDriverFiles { { import std.algorithm.comparison : min; - with (slot.overlapped) { + with (slot.overlapped.overlapped) { Internal = 0; InternalHigh = 0; Offset = cast(uint)(slot.offset & 0xFFFFFFFF); @@ -163,8 +165,8 @@ final class WinAPIEventDriverFiles : EventDriverFiles { } auto nbytes = min(slot.buffer.length, DWORD.max); - if (!() @trusted { return fun(h, &slot.buffer[0], nbytes, &slot.overlapped, &onIOFinished!(fun, RO)); } ()) { - slot.core.removeWaiter(); + if (!() @trusted { return fun(h, &slot.buffer[0], nbytes, &slot.overlapped.overlapped, &onIOFinished!(fun, RO)); } ()) { + slot.overlapped.driver.removeWaiter(); slot.invokeCallback(IOStatus.error, slot.bytesTransferred); } } @@ -173,20 +175,23 @@ final class WinAPIEventDriverFiles : EventDriverFiles { { if (slot.callback) { m_core.removeWaiter(); - () @trusted { CancelIoEx(h, &slot.overlapped); } (); + () @trusted { CancelIoEx(h, &slot.overlapped.overlapped); } (); slot.callback = null; slot.buffer = null; } } private static extern(Windows) - void onIOFinished(alias fun, bool RO)(DWORD error, DWORD bytes_transferred, OVERLAPPED* overlapped) + void onIOFinished(alias fun, bool RO)(DWORD error, DWORD bytes_transferred, OVERLAPPED* _overlapped) { - - auto slot = () @trusted { return cast(FileSlot.Direction!RO*)overlapped.hEvent; } (); + auto ctx = () @trusted { return cast(OVERLAPPED_FILE*)_overlapped; } (); + FileFD id = ctx.handle; + auto handle = idToHandle(id); + static if (RO) + auto slot = () @trusted { return &ctx.driver.m_handles[handle].file.write; } (); + else + auto slot = () @trusted { return &ctx.driver.m_handles[handle].file.read; } (); assert(slot !is null); - HANDLE h = slot.handle; - auto id = FileFD(cast(size_t)h); if (!slot.callback) { // request was already cancelled @@ -194,7 +199,7 @@ final class WinAPIEventDriverFiles : EventDriverFiles { } if (error != 0) { - slot.core.removeWaiter(); + ctx.driver.removeWaiter(); slot.invokeCallback(IOStatus.error, slot.bytesTransferred + bytes_transferred); return; } @@ -203,10 +208,10 @@ final class WinAPIEventDriverFiles : EventDriverFiles { slot.offset += bytes_transferred; if (slot.bytesTransferred >= slot.buffer.length || slot.mode != IOMode.all) { - slot.core.removeWaiter(); + ctx.driver.removeWaiter(); slot.invokeCallback(IOStatus.ok, slot.bytesTransferred); } else { - startIO!(fun, RO)(h, slot); + startIO!(fun, RO)(handle, slot); } } From 3d2a2656bba2977dd8b9304b8ffdf08dfd686118 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Ludwig?= Date: Sat, 3 Mar 2018 20:04:51 +0100 Subject: [PATCH 3/3] Robustness fix for WinAPIEventDriverWatchers. Under certain (rare) circumstances, the receive buffer contains data from a file instead of the expected FILE_NOTIFY_INFORMATION structure. No signs of handle or buffer reuse could be found, so the cause for this remains unclear. Until the cause can be narrowed down, this keeps the issue visible while avoiding to crash the application. --- source/eventcore/drivers/winapi/watchers.d | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/source/eventcore/drivers/winapi/watchers.d b/source/eventcore/drivers/winapi/watchers.d index 1928bce..b4e9467 100644 --- a/source/eventcore/drivers/winapi/watchers.d +++ b/source/eventcore/drivers/winapi/watchers.d @@ -127,6 +127,14 @@ final class WinAPIEventDriverWatchers : EventDriverWatchers { while (result.length) { assert(result.length >= FILE_NOTIFY_INFORMATION._FileName.offsetof); auto fni = () @trusted { return cast(FILE_NOTIFY_INFORMATION*)result.ptr; } (); + if (fni.NextEntryOffset > result.length) { + import std.stdio : stderr; + () @trusted { + try stderr.writeln("ERROR: Invalid directory watcher event received."); + catch (Exception e) {} + } (); + break; + } result = result[fni.NextEntryOffset .. $]; FileChange ch;