0.21.1: a handle is retired before its value is closed (0.22.1's correction on the 0.21 line)

README.md

@@ -7,14 +7,26 @@ 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
 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

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"
 

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

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<unsigned>(atomic::load(&generations()[slot]));
+}
+
 inline okw_uptr slot_of(void* h) {
     const okw_uptr v = reinterpret_cast<okw_uptr>(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<okw_uptr>(h);
     if (!within_bound(h)) return v;                    // carried without a generation
-    return (static_cast<okw_uptr>(generations()[slot_of(h)] & kGenerationMask) << 32) | (v + 1u);
+    return (static_cast<okw_uptr>(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<void*>(low - 1u);
     if (!within_bound(h)) return reinterpret_cast<void*>(w);
     if ((static_cast<unsigned>(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

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

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

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 <cstring>
+#include <openkal/fs.h>
+#include <openkal/net.h>
+#include <openkal/stream.h>
+#include <openkal/task.h>
+#include <openkal/time.h>
+#include <openkal/types.h>
+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<long>(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);
+}};