0.26.1: a handle is retired before its descriptor is closed; one release closes; a released handle's stream is no stream

README.md

@@ -8,9 +8,30 @@ for Linux, written on the kernel's own system-call interface.
 openkal = "0.25.0"
 
 [target.'cfg(os = "linux")'.dependencies]
-openkal-linux = "0.26.0"
+openkal-linux = "0.26.1"
 ```
 
+## What 0.26.1 corrects
+
+**A handle is retired before its descriptor is closed.** A connection, a listener
+and a datagram socket were closed first and retired second. The kernel gives a
+closed number to the next open in any context, so a file opened by another context
+between the two took the number with the generation the socket's handle still had,
+and the retire then made the file's handle stale: its stream was answered as zero,
+which is standard input, its reads returned nothing of the file, and its descriptor
+was never closed (#53; measured there at about 150 files a second on four
+processors, and 94 in 3.4 million opens by this package's test). Every release is
+now one function, which retires and then closes.
+
+**One release closes.** The generation is advanced by a compare-and-exchange from
+the value the handle carries, so a second release of the same word, in the same
+context or another, closes nothing. It is read and advanced through the atomic
+operations.
+
+**A released handle's stream is no stream.** `kal_fs_stream` and `kal_net_stream`
+answer a handle that names no live descriptor with a word no descriptor has, and a
+transfer upon it reports `kal_err_invalid`. They answered zero.
+
 ## What 0.26 adds (openkal 0.25)
 
 **A second name for a file** (`kal_fs_hard_link`) is `linkat` with no flags, so a

mcpp.toml

@@ -1,7 +1,7 @@
 [package]
 namespace   = "mcpplibs"
 name        = "openkal-linux"
-version     = "0.26.0"
+version     = "0.26.1"
 description = "The reference implementation of openkal for Linux, written on the kernel's own system-call interface so that it can be placed beneath a C library as well as above one."
 license     = "Apache-2.0"
 

src/datagram.cpp

@@ -135,12 +135,7 @@ kal_intptr kal_datagram_recv_from(kal_datagram d, void* buf, kal_uintptr len,
     }
 }
 
-void kal_datagram_close(kal_datagram d) {
-    const int fd = fd_of(d);
-    if (fd < 0) return;
-    okl::sys(okl::nr_close, fd);
-    okl::retire(d.h);
-}
+void kal_datagram_close(kal_datagram d) { okl::release(d.h); }
 
 // Broadcast is not claimed. The kernel provides it only after SO_BROADCAST has
 // been set, and this interface has no operation that would set it; a word

src/fs.cpp

@@ -594,21 +594,17 @@ void kal_fs_close_dir(kal_dir d) {
     // directory whose enumerations are not this one's.
     listings_end(fd);
     okl::rights::forget(fd);
-    okl::retire(d.h); okl::sys(okl::nr_close, fd);
+    okl::release(d.h);
 }
 
-void kal_fs_close_file(kal_file f) {
-    const int fd = okl::unpack(f.h);
-    if (fd >= 0) { okl::retire(f.h); okl::sys(okl::nr_close, fd); }
-}
+void kal_fs_close_file(kal_file f) { okl::release(f.h); }
 
 // A file's stream is the file. The descriptor is what openkal.stream's handle
 // holds on this implementation, so no conversion is required and none is
 // performed: the two interfaces agree because both are descriptor-shaped here,
 // which is a property of this implementation and not of the specification.
 kal_stream kal_fs_stream(kal_file f) {
-    const int fd = okl::unpack(f.h);
-    return kal_stream{ fd < 0 ? 0u : static_cast<kal_uintptr>(fd) };
+    return kal_stream{ okl::stream_word(f.h) };
 }
 
 // The greatest length of a name this implementation accepts.

src/handle.h

@@ -17,9 +17,16 @@ inline unsigned* generations() {
     return g;
 }
 
+// A generation is read by the context that opens a descriptor and advanced by
+// the one that releases it, and the two are different contexts whenever a
+// number is reused across them, so both go through the atomic operations.
+inline unsigned generation(int fd) {
+    return __atomic_load_n(&generations()[fd], __ATOMIC_RELAXED);
+}
+
 inline okl_uptr pack(int fd) {
     if (fd < 0 || fd >= kMaxDescriptor) return 0;
-    return (static_cast<okl_uptr>(generations()[fd]) << 32)
+    return (static_cast<okl_uptr>(generation(fd)) << 32)
          | (static_cast<okl_uptr>(fd) + 1u);
 }
 
@@ -39,13 +46,43 @@ inline okl_uptr pack(int fd) {
 inline int unpack(okl_uptr h) {
     const int fd = static_cast<int>(h & 0xffffffffu) - 1;
     if (fd < 0 || fd >= kMaxDescriptor) return -1;
-    if (static_cast<unsigned>(h >> 32) != generations()[fd]) return -1;
+    if (static_cast<unsigned>(h >> 32) != generation(fd)) return -1;
     return fd;
 }
 
-inline void retire(okl_uptr h) {
+// RETIRE, THEN CLOSE. The kernel gives a closed number to the next open in any
+// context. A handle retired after its descriptor was closed leaves a window in
+// which another context opens the number, packs it with the generation the
+// handle being released still has, and is then left with a handle this retire
+// makes stale: its stream reads and writes nothing of its own, and its
+// descriptor is never closed (openkal-linux#53). Every release in this
+// implementation is `release(h)`, which keeps the order in one place.
+inline bool retire(okl_uptr h) {
+    const int fd = static_cast<int>(h & 0xffffffffu) - 1;
+    if (fd < 0 || fd >= kMaxDescriptor) return false;
+    // One release of a handle advances its generation; a second release of the
+    // same word, in this context or another, finds it advanced and closes
+    // nothing, so it cannot close a descriptor opened since.
+    unsigned held = static_cast<unsigned>(h >> 32);
+    return __atomic_compare_exchange_n(&generations()[fd], &held, held + 1u, false,
+                                       __ATOMIC_RELAXED, __ATOMIC_RELAXED);
+}
+
+inline void release(okl_uptr h) {
+    const int fd = static_cast<int>(h & 0xffffffffu) - 1;
+    if (retire(h)) sys(nr_close, fd);
+}
+
+// What a stream enquiry answers for a handle that does not name a live
+// descriptor. A stream is a bare descriptor here (stream.cpp), so zero is
+// standard input: a released handle answered with zero was read from and
+// written to as standard input. No descriptor has this value, and every
+// transfer upon it reports `kal_err_invalid`.
+inline constexpr okl_uptr kNoStream = ~static_cast<okl_uptr>(0);
+
+inline okl_uptr stream_word(okl_uptr h) {
     const int fd = unpack(h);
-    if (fd >= 0) ++generations()[fd];
+    return fd < 0 ? kNoStream : static_cast<okl_uptr>(fd);
 }
 
 }  // namespace okl

src/net.cpp

@@ -207,8 +207,7 @@ kal_stream kal_net_stream(kal_net_conn c) {
     // operations take whatever the environment's transfer calls take, and a
     // packed word is not that. It carries its type, so a handle crossing
     // between two interfaces is not a word either of them has to interpret.
-    const int fd = fd_of(c);
-    return kal_stream{ fd < 0 ? 0u : static_cast<kal_uintptr>(fd) };
+    return kal_stream{ okl::stream_word(c.h) };
 }
 
 int kal_net_peer(kal_net_conn c, kal_endpoint* out) {
@@ -244,19 +243,9 @@ int kal_net_shutdown(kal_net_conn c, int direction) {
     return kal_ok;
 }
 
-void kal_net_close(kal_net_conn c) {
-    const int fd = fd_of(c);
-    if (fd < 0) return;
-    okl::sys(okl::nr_close, fd);
-    okl::retire(c.h);
-}
+void kal_net_close(kal_net_conn c) { okl::release(c.h); }
 
-void kal_net_close_listener(kal_net_listener l) {
-    const int fd = fd_of(l);
-    if (fd < 0) return;
-    okl::sys(okl::nr_close, fd);
-    okl::retire(l.h);
-}
+void kal_net_close_listener(kal_net_listener l) { okl::release(l.h); }
 
 // Both positions hold on this kernel. IPv6 is configurable out of a build, but
 // the socket call then reports it at the point of the attempt, and a word that

tests/a_released_handle_is_stale_before_its_number_is_free.cpp

@@ -0,0 +1,122 @@
+// A handle is retired before its descriptor is closed (0.26.1, openkal-linux#53).
+//
+// The kernel gives a closed number to the next open in any context. Until
+// 0.26.1 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
+// number with the generation the socket's handle still had, and the retire then
+// made the file's handle stale. Its stream was answered as zero, standard
+// input, so its reads returned nothing of the file and its descriptor was never
+// closed. Measured by the report on four processors: about 150 such files a
+// second.
+//
+// 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 <atomic>
+#include <chrono>
+#include <cstring>
+#include <thread>
+import openkal.types;
+import openkal.fs;
+import openkal.net;
+import openkal.stream;
+import mcpplibs.mtest;
+
+using namespace mcpplibs::mtest;
+
+namespace {
+
+// The working directory, which is the first directory the environment supplies.
+kal_dir root() {
+    kal_dir d{}; char n[4096]; kal_uintptr l = 0;
+    return kal_fs_preopen(0, &d, n, sizeof n, &l) == kal_ok ? d : kal_dir{};
+}
+
+constexpr char kName[] = "okl_retire_probe.txt";
+constexpr char kText[] = "the file's bytes";   // 16
+
+}  // namespace
+
+UTest a_released_handle_is_stale_before_its_number_is_free{
+    "a_released_handle_is_stale_before_its_number_is_free", [] {
+    const kal_dir dir = root();
+    expect(fatal(dir.h != 0)) << "the environment supplies the working directory";
+    {
+        kal_file f{};
+        expect(fatal(kal_fs_open(dir, kName, sizeof kName - 1,
+                                 (kal::fs::open::write | kal::fs::open::create | kal::fs::open::truncate).bits,
+                                 &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;
+    kal_net_listener lis{};
+    expect(fatal(kal_net_listen(&any, &lis) == kal_ok));
+    kal_endpoint at{};
+    expect(fatal(kal_net_listener_local(lis, &at) == kal_ok));
+
+    std::atomic<bool> stop{false};
+    std::atomic<long> served{0};
+    std::thread accepting([&] {
+        for (;;) {
+            kal_net_conn c{};
+            if (kal_net_accept(lis, &c) != kal_ok) break;
+            kal_stream_write(kal_net_stream(c), "okl", 3);
+            kal_net_close(c);
+            served.fetch_add(1, std::memory_order_relaxed);
+            if (stop.load(std::memory_order_relaxed)) break;
+        }
+    });
+    std::thread connecting([&] {
+        while (!stop.load(std::memory_order_relaxed)) {
+            kal_net_conn c{};
+            if (kal_net_connect(&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(&at, &last) == kal_ok) kal_net_close(last);
+    });
+
+    long opened = 0, stale = 0, short_reads = 0;
+    const auto until = std::chrono::steady_clock::now() + std::chrono::seconds(3);
+    while (std::chrono::steady_clock::now() < until) {
+        for (int i = 0; i < 256; ++i) {
+            kal_file f{};
+            if (kal_fs_open(dir, kName, sizeof kName - 1, kal::fs::open::read.bits, &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) ++short_reads;
+            kal_fs_close_file(f);
+        }
+    }
+    stop.store(true, std::memory_order_relaxed);
+    connecting.join();
+    accepting.join();
+    kal_net_close_listener(lis);
+
+    expect(gt(opened, 0L));
+    expect(gt(served.load(), 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(short_reads, 0L)) << "files whose read was not the file, of " << opened;
+
+    // A released handle's stream is no stream: a transfer upon it is refused,
+    // and is not a transfer upon standard input.
+    kal_file f{};
+    expect(fatal(kal_fs_open(dir, kName, sizeof kName - 1, kal::fs::open::read.bits, &f) == kal_ok));
+    kal_fs_close_file(f);
+    char one = 0;
+    expect(eq(kal_stream_read(kal_fs_stream(f), &one, 1), static_cast<kal_intptr>(-kal_err_invalid)));
+    // And a second release of it closes nothing: the number may already be
+    // another's.
+    kal_file g{};
+    expect(fatal(kal_fs_open(dir, kName, sizeof kName - 1, kal::fs::open::read.bits, &g) == kal_ok));
+    kal_fs_close_file(f);
+    char buf[32]{};
+    expect(eq(kal_stream_read(kal_fs_stream(g), buf, sizeof buf), static_cast<kal_intptr>(16)));
+    kal_fs_close_file(g);
+
+    kal_fs_remove(dir, kName, sizeof kName - 1);
+}};