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] 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); } }