diff --git a/README.md b/README.md index 75e0ca7..2b9ee4d 100644 --- a/README.md +++ b/README.md @@ -7,7 +7,7 @@ An implementation of [openkal](https://github.com/mcpplibs/openkal) for Windows. openkal = "0.25.0" [target.'cfg(windows)'.dependencies] -openkal-windows = "0.21.0" +openkal-windows = "0.21.1" ``` Its purpose is as much to test the specification as to be used. openkal was @@ -15,6 +15,18 @@ drawn against systems that share an ancestry; a third implementation on a system that shares none of it is what turns "this interface is not Unix's shape" from a claim into an observation. +## What 0.21.1 corrects + +**A handle is retired before its value is closed.** A connection, a listener and a +datagram socket were closed first and retired second. The system gives a closed +value to the next handle made in any context, so a file opened by another context +between the two took the value with the generation the socket's handle still had, +and the retire then made the file's handle stale: its stream was no stream and its +reads returned nothing of the file (openkal-linux#53; this implementation has the +same construction). The three releases retire and then close, as a file's and a +directory's do. A generation is read and advanced through the atomic operations. +This is 0.22.1's correction on the 0.21 line. + ## What 0.21 adds (openkal 0.25) **A second name for a file** (`kal_fs_hard_link`) is the link record of diff --git a/mcpp.toml b/mcpp.toml index d92a959..aebc79e 100644 --- a/mcpp.toml +++ b/mcpp.toml @@ -1,7 +1,7 @@ [package] namespace = "mcpplibs" name = "openkal-windows" -version = "0.21.0" +version = "0.21.1" description = "An implementation of openkal for Windows, written on the Win32 interfaces and the object manager beneath them, using no C runtime symbol." license = "Apache-2.0" diff --git a/src/datagram.cpp b/src/datagram.cpp index 5d31cd6..d515d9c 100644 --- a/src/datagram.cpp +++ b/src/datagram.cpp @@ -214,8 +214,8 @@ kal_intptr kal_datagram_recv_from(kal_datagram d, void* buf, kal_uintptr len, void kal_datagram_close(kal_datagram d) { const SOCKET s = socket_of(d); if (bad(s)) return; - if (auto* n = net()) n->close(s); okw::retire(d.h); + if (auto* n = net()) n->close(s); } // Broadcast is not claimed. The system provides it only after SO_BROADCAST has diff --git a/src/handle.h b/src/handle.h index 8062462..a9c50a6 100644 --- a/src/handle.h +++ b/src/handle.h @@ -24,11 +24,18 @@ namespace okw { inline constexpr okw_uptr kSlots = 1u << 16; -inline unsigned* generations() { - static unsigned g[kSlots]; +// A generation is read by the context that opens a handle and advanced by the +// one that releases it, and the two are different contexts whenever a value is +// reused across them, so both go through the atomic operations (0.21.1). +inline volatile long* generations() { + static volatile long g[kSlots]; return g; } +inline unsigned generation(okw_uptr slot) { + return static_cast(atomic::load(&generations()[slot])); +} + inline okw_uptr slot_of(void* h) { const okw_uptr v = reinterpret_cast(h); return (v >> 2) & (kSlots - 1); @@ -50,7 +57,7 @@ inline okw_uptr pack(void* h) { if (h == nullptr || h == INVALID_HANDLE_VALUE) return 0; const okw_uptr v = reinterpret_cast(h); if (!within_bound(h)) return v; // carried without a generation - return (static_cast(generations()[slot_of(h)] & kGenerationMask) << 32) | (v + 1u); + return (static_cast(generation(slot_of(h)) & kGenerationMask) << 32) | (v + 1u); } // Returns the environment's handle, or null if the word does not name a live one. @@ -61,13 +68,19 @@ inline void* unpack(okw_uptr w) { void* h = reinterpret_cast(low - 1u); if (!within_bound(h)) return reinterpret_cast(w); if ((static_cast(w >> 32) & kGenerationMask) - != (generations()[slot_of(h)] & kGenerationMask)) return nullptr; + != (generation(slot_of(h)) & kGenerationMask)) return nullptr; return h; } +// RETIRE, THEN CLOSE (0.21.1). The system gives a closed value to the next +// handle made in any context. A handle retired after its value was closed +// leaves a window in which another context is given the value, packs it with +// the generation the handle being released still has, and is then left with a +// handle this retire makes stale (openkal-linux#53, the same construction: the +// three releases of openkal.net and openkal.datagram closed first). inline void retire(okw_uptr w) { void* h = unpack(w); - if (h && within_bound(h)) ++generations()[slot_of(h)]; + if (h && within_bound(h)) atomic::increment(&generations()[slot_of(h)]); } } // namespace okw diff --git a/src/net.cpp b/src/net.cpp index b2158bd..b825987 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -296,15 +296,15 @@ void kal_net_close(kal_net_conn c) { const SOCKET s = socket_of(c); if (bad(s)) return; okw::note_read_ended(s, false); // the number may be given to another socket - if (auto* n = net()) n->close(s); okw::retire(c.h); + if (auto* n = net()) n->close(s); } void kal_net_close_listener(kal_net_listener l) { const SOCKET s = socket_of(l); if (bad(s)) return; - if (auto* n = net()) n->close(s); okw::retire(l.h); + if (auto* n = net()) n->close(s); } // Both positions hold on this system: it speaks IPv6, and its `shutdown' ends diff --git a/src/win.h b/src/win.h index f78385a..721a157 100644 --- a/src/win.h +++ b/src/win.h @@ -79,9 +79,11 @@ extern "C" long long _InterlockedExchange64(volatile long long*, long long); extern "C" long long _InterlockedExchangeAdd64(volatile long long*, long long); extern "C" long long _InterlockedOr64(volatile long long*, long long); extern "C" long _InterlockedIncrement(volatile long*); -#pragma intrinsic(_InterlockedExchange64, _InterlockedExchangeAdd64, _InterlockedOr64, _InterlockedIncrement) +extern "C" long _InterlockedOr(volatile long*, long); +#pragma intrinsic(_InterlockedExchange64, _InterlockedExchangeAdd64, _InterlockedOr64, _InterlockedIncrement, _InterlockedOr) namespace okw::atomic { inline long increment(volatile long* p) { return _InterlockedIncrement(p); } +inline long load(volatile long* p) { return _InterlockedOr(p, 0); } inline long long exchange(volatile long long* p, long long v) { return _InterlockedExchange64(p, v); } inline long long fetch_add(volatile long long* p, long long v) { return _InterlockedExchangeAdd64(p, v); } inline long long load(volatile long long* p) { return _InterlockedOr64(p, 0); } @@ -91,6 +93,7 @@ inline long long fetch_or(volatile long long* p, long long v) { return _Interloc #else namespace okw::atomic { inline long increment(volatile long* p) { return __atomic_add_fetch(p, 1, __ATOMIC_ACQ_REL); } +inline long load(volatile long* p) { return __atomic_load_n(p, __ATOMIC_ACQUIRE); } inline long long exchange(volatile long long* p, long long v) { return __atomic_exchange_n(p, v, __ATOMIC_ACQ_REL); } inline long long fetch_add(volatile long long* p, long long v) { return __atomic_fetch_add(p, v, __ATOMIC_ACQ_REL); } inline long long load(volatile long long* p) { return __atomic_load_n(p, __ATOMIC_ACQUIRE); } diff --git a/tests/a_released_handle_is_stale_before_its_value_is_free.cpp b/tests/a_released_handle_is_stale_before_its_value_is_free.cpp new file mode 100644 index 0000000..068eeb2 --- /dev/null +++ b/tests/a_released_handle_is_stale_before_its_value_is_free.cpp @@ -0,0 +1,104 @@ +// A handle is retired before its value is closed (0.21.1 and 0.22.1; openkal-linux#53, +// the same construction). +// +// The system gives a closed value to the next handle made in any context. Until +// then a connection, a listener and a datagram socket were closed first and +// retired second: a file opened by another context between the two took the +// value with the generation the socket's handle still had, and the retire then +// made the file's handle stale, so its stream was no stream and its reads +// returned nothing of the file. +// +// One context accepts connections and closes them, one connects and closes, and +// this one opens, reads and closes a file. Every read returns the file. +#include +#include +#include +#include +#include +#include +#include +import mcpplibs.mtest; + +using namespace mcpplibs::mtest; + +namespace { + +constexpr char kName[] = "okw_retire_probe.txt"; +constexpr char kText[] = "the file's bytes"; // 16 + +volatile kal_u32 g_stop = 0; +volatile long g_served = 0; +kal_net_listener g_listener{}; +kal_endpoint g_at{}; + +void accepting(void*) { + for (;;) { + kal_net_conn c{}; + if (kal_net_accept(g_listener, &c) != kal_ok) break; + kal_stream_write(kal_net_stream(c), "okw", 3); + kal_net_close(c); + g_served = g_served + 1; + if (g_stop) break; + } +} + +void connecting(void*) { + while (!g_stop) { + kal_net_conn c{}; + if (kal_net_connect(&g_at, &c) == kal_ok) kal_net_close(c); + } + // One more, so the accepting context leaves its wait and sees the word. + kal_net_conn last{}; + if (kal_net_connect(&g_at, &last) == kal_ok) kal_net_close(last); +} + +} // namespace + +UTest a_released_handle_is_stale_before_its_value_is_free{ + "a_released_handle_is_stale_before_its_value_is_free", [] { + // The working directory, which is the first directory the environment supplies. + kal_dir dir{}; char name[4096]; kal_uintptr len = 0; + expect(fatal(kal_fs_preopen(0, &dir, name, sizeof name, &len) == kal_ok)); + { + kal_file f{}; + expect(fatal(kal_fs_open(dir, kName, sizeof kName - 1, + KAL_OPEN_WRITE | KAL_OPEN_CREATE | KAL_OPEN_TRUNCATE, &f) == kal_ok)); + expect(fatal(kal_stream_write(kal_fs_stream(f), kText, 16) == 16)); + kal_fs_close_file(f); + } + + kal_endpoint any{}; + any.addr[0] = 127; any.addr[3] = 1; any.addr_len = 4; + expect(fatal(kal_net_listen(&any, &g_listener) == kal_ok)); + expect(fatal(kal_net_listener_local(g_listener, &g_at) == kal_ok)); + + kal_task a{}, b{}; + expect(fatal(kal_task_start(accepting, nullptr, &a) == kal_ok)); + expect(fatal(kal_task_start(connecting, nullptr, &b) == kal_ok)); + + long opened = 0, stale = 0, other = 0; + const kal_duration until = kal_time_monotonic() + 3'000'000'000ull; + while (kal_time_monotonic() < until) { + for (int i = 0; i < 64; ++i) { + kal_file f{}; + if (kal_fs_open(dir, kName, sizeof kName - 1, KAL_OPEN_READ, &f) != kal_ok) continue; + ++opened; + char buf[32]{}; + const kal_intptr n = kal_stream_read(kal_fs_stream(f), buf, sizeof buf); + if (n < 0) ++stale; + else if (n != 16 || std::memcmp(buf, kText, 16) != 0) ++other; + kal_fs_close_file(f); + } + } + g_stop = 1; + kal_task_join(b); + kal_task_join(a); + kal_net_close_listener(g_listener); + + expect(gt(opened, 0L)); + expect(gt(static_cast(g_served), 0L)) << "no connection was closed beside the opens; the test measured nothing"; + expect(eq(stale, 0L)) << "files whose handle was stale when its stream was taken, of " << opened; + expect(eq(other, 0L)) << "files whose read was not the file, of " << opened; + + kal_fs_remove(dir, kName, sizeof kName - 1); +}};