0.7.1 --- a bounded transfer waited upon the descriptor below the one it moved (#19)

`kal_timeout_read` and `kal_timeout_write` decoded their `kal_stream` argument
with `okl::unpack`, which is the decoder for an OWNED handle. A stream handle is
not one: `stream.cpp` states that these are this environment's own descriptors
and `kal_stream_read` and `kal_stream_write` take them as such.

THE DECODE DID NOT FAIL, WHICH IS WHY THE CALL SITE READ AS CORRECT. `handle.h`
packs an owned handle as `(generation << 32) | (fd + 1)`, so a bare descriptor N
has exactly the shape of a packed handle naming N-1, and `unpack` accepts it
whenever the generation recorded for N-1 is still zero. That is every index at
which no owned handle has yet been released. The wait was therefore performed
upon descriptor N-1 while the transfer that followed was performed upon N.

Measured with the packing arithmetic alone: `unpack(1) = 0`, `unpack(2) = 1`,
`unpack(5) = 4`. Of the three standard streams only `kal_stdin`, whose handle is
zero, fails to decode; `kal_stdout` decoded to descriptor 0 and `kal_stderr` to
1.

The former reading tested `unpack` and fell back when it failed, on the stated
ground that the standard streams are not packed. The ground is correct and the
conclusion drawn from it was not: no stream handle is packed here, so the
fallback was the path that should always have been taken.

WHAT A CALLER OBSERVED. Waiting upon a descriptor that never becomes ready is an
expiry, so a bounded read of a stream with bytes waiting reported
`kal_err_again`; waiting upon one that is ready lets the transfer proceed and
block, so an operation that states a bound could wait without one. Which of the
two occurred is a function of what happened to occupy the descriptor below, so
the same program answered differently run alone and run after something else,
and the index healed permanently once an owned handle at N-1 had been released.

Reported through openkal-musl in #13 as
`std::filesystem::copy_file` reporting `Resource temporarily unavailable` for
two ordinary files. libc++ opens the source with `O_NONBLOCK`, which is
meaningless for a regular file on a kernel and which openkal-musl routes through
this interface, so every byte of a file copy passed through the wait above.

`kal_timeout_accept` and `kal_timeout_recv_from` keep the decode and are
correct: a listener and a datagram are owned. The four call sites divide exactly
along the borrowed/owned line and the two that were wrong were the two holding a
borrowed handle.

--- the criterion -----------------------------------------------------------

`tests/conformance_v08.cpp` gains `timeout_stream_section`, and it is the first
section `main` runs. Nothing in this file reached the two operations before: the
existing observations are of an accept, whose handle is owned and was decoded
correctly, and of a zero-length write, which returns before it waits.

Sixteen channels rather than one, because under the defect the wait was upon
descriptor N-1 and whether that expires depends on what occupies N-1. First
rather than last, because the mistaken decode corrected itself for every index
at which an owned handle had already been released, so the same observation
placed after the listener and datagram sections would have held upon the defect.

Confirmed by putting the old implementation back:

    0 of 16 bounded reads transferred
    FAIL: a bounded read of a stream that has bytes waiting transfers them
    conformance_v08 ... FAIL (exit 1, 0.04s)

and with this change, 7 suites passed, 0 failed.

`src/handle.h` records beside `unpack` that a word which was never packed is
accepted silently, and that a stream handle must not reach it.

Author: Sunrisepeak

README.md

@@ -8,7 +8,7 @@ for Linux, written on the kernel's own system-call interface.
 openkal = "0.9.0"
 
 [target.'cfg(os = "linux")'.dependencies]
-openkal-linux = "0.7.0"
+openkal-linux = "0.7.1"
 ```
 
 ## Why it does not use a C library

mcpp.toml

@@ -1,7 +1,7 @@
 [package]
 namespace   = "mcpplibs"
 name        = "openkal-linux"
-version     = "0.7.0"
+version     = "0.7.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/handle.h

@@ -24,6 +24,18 @@ inline okl_uptr pack(int fd) {
 }
 
 // Returns the descriptor, or -1 if the word does not name a live one.
+//
+// THIS ACCEPTS A WORD THAT WAS NEVER PACKED, AND SILENTLY. A bare descriptor N
+// has the shape of a packed handle naming N-1 whose generation is still zero,
+// so this returns N-1 for it rather than -1. Nothing here can tell the two
+// apart: the word is one machine word and carries no tag.
+//
+// The consequence is that a handle of the OTHER discipline must never reach
+// this function. openkal.stream's handles are bare descriptors (stream.cpp
+// states why), and src/timeout.cpp used to pass one here and wait upon the
+// descriptor below the one it then transferred upon. Owned handles --- kal_file,
+// kal_dir, kal_net_listener, kal_net_conn, kal_datagram --- are the whole of
+// this function's domain.
 inline int unpack(okl_uptr h) {
     const int fd = static_cast<int>(h & 0xffffffffu) - 1;
     if (fd < 0 || fd >= kMaxDescriptor) return -1;

src/timeout.cpp

@@ -52,6 +52,33 @@ int await(int fd, short events, kal_u64 ns) {
     }
 }
 
+// A STREAM HANDLE IS A DESCRIPTOR AND IS NOT DECODED. stream.cpp states it in
+// terms, kal_stream_read and kal_stream_write take it as one, and so must the
+// wait that precedes them: the wait and the transfer that follows it have to
+// name the same object or the wait answers about something else.
+//
+// THIS FILE USED TO DECODE IT AS AN OWNED HANDLE, AND THE DECODE SUCCEEDED.
+// handle.h packs an owned handle as (generation << 32) | (fd + 1), so a bare
+// descriptor N has exactly the shape of a packed handle naming N-1, and
+// okl::unpack accepts it whenever generation N-1 is still zero -- which is
+// every index at which no owned handle has yet been released. The wait was
+// therefore performed upon descriptor N-1 and the transfer upon N.
+//
+// The former reading tested `unpack` and fell back when it failed, on the
+// stated ground that the standard streams are not packed. That ground is
+// correct and the conclusion drawn from it was not: NO stream handle is packed
+// here, and of the three standard ones only kal_stdin, whose handle is zero,
+// fails to decode. kal_stdout decoded to descriptor 0 and kal_stderr to 1.
+//
+// What a caller observed was an expiry for a stream that had bytes waiting, or
+// a wait without bound inside an operation that states one, according to what
+// happened to occupy the descriptor below. Both are functions of the process's
+// descriptor history, so the same program answered differently when run alone
+// and when run after something else -- and the index healed for good once an
+// owned handle at N-1 had been released, because that advances the generation
+// and makes the decode fail correctly.
+int stream_fd(kal_stream s) { return static_cast<int>(s.h); }
+
 }  // namespace
 
 extern "C" {
@@ -61,26 +88,24 @@ kal_intptr kal_timeout_read(kal_stream s, void* buf, kal_uintptr len, kal_u64 ns
     // would turn a call that always succeeds into one that can expire.
     if (len == 0) return 0;
 
-    const int fd = okl::unpack(s.h);
-    // THE STANDARD STREAMS ARE NOT PACKED HANDLES. openkal.stream reports them
-    // as the descriptors themselves, so a word that does not unpack is taken to
-    // be one of those rather than being refused.
-    const int use = (fd >= 0) ? fd : static_cast<int>(s.h);
-
-    if (const int rc = await(use, okl::poll_in, ns); rc != kal_ok) return -rc;
+    if (const int rc = await(stream_fd(s), okl::poll_in, ns); rc != kal_ok) return -rc;
     return kal_stream_read(s, buf, len);
 }
 
 kal_intptr kal_timeout_write(kal_stream s, const void* buf, kal_uintptr len, kal_u64 ns) {
     if (len == 0) return 0;
 
-    const int fd  = okl::unpack(s.h);
-    const int use = (fd >= 0) ? fd : static_cast<int>(s.h);
-
-    if (const int rc = await(use, okl::poll_out, ns); rc != kal_ok) return -rc;
+    if (const int rc = await(stream_fd(s), okl::poll_out, ns); rc != kal_ok) return -rc;
     return kal_stream_write(s, buf, len);
 }
 
+// THE TWO OPERATIONS BELOW DO DECODE, AND THAT IS NOT AN INCONSISTENCY WITH THE
+// TWO ABOVE. A listener and a datagram are owned: their handles are made by
+// okl::pack and released by okl::retire, so the decode is the operation that
+// recovers the descriptor and its failure is how a released handle is refused.
+// A stream is borrowed and carries no generation. The four call sites divide
+// exactly along that line, and the two that were wrong were the two that had a
+// borrowed handle in hand.
 int kal_timeout_accept(kal_net_listener l, kal_u64 ns, kal_net_conn* out) {
     if (out == nullptr) return kal_err_invalid;
     const int fd = okl::unpack(l.h);

tests/conformance_v08.cpp

@@ -190,6 +190,75 @@ void space_section() {
     kal_process_close(p.p);
 }
 
+// A BOUNDED TRANSFER WAITS UPON THE STREAM IT THEN TRANSFERS UPON.
+//
+// This is the observation that was missing while the two operations it examines
+// waited upon the descriptor below the one they transferred upon. Nothing else
+// in this file reached them: the two observations in timeout_section below are
+// of an accept, whose handle is owned and was decoded correctly, and of a
+// zero-length write, which returns before it waits.
+//
+// WHY IT MAKES SIXTEEN CHANNELS AND NOT ONE. Under the defect the wait was
+// performed upon descriptor N-1, and whether that expires depends on what
+// occupies N-1. For a channel made after another, N-1 is the previous channel's
+// writing end, upon which input is never reported, so the read expires while
+// bytes sit in the stream that was asked for. One channel would be answering
+// about whichever descriptor happened to precede it; sixteen make the answer a
+// property of the implementation rather than of the process that ran it.
+//
+// AND WHY IT RUNS FIRST. The mistaken decode succeeded only while the
+// generation recorded for N-1 was still zero, so it corrected itself for every
+// index at which an owned handle had already been released. Placed after the
+// sections that create and release listeners and datagrams, this would have
+// examined reused descriptors and held upon the defect.
+//
+// The count is reported rather than the first failure. Sixteen distinguishes a
+// correct implementation from one that answers about a neighbouring descriptor;
+// the first failure alone would not say which.
+void timeout_stream_section() {
+    constexpr int n = 16;
+    kal_stream mine[n]{}, theirs[n]{};
+    int made = 0;
+
+    for (; made < n; ++made) {
+        if (kal_process_channel(&mine[made], &theirs[made]) != kal_ok) break;
+        const char byte = 'x';
+        if (kal_stream_write(theirs[made], &byte, 1) != 1) break;
+    }
+    check(made == n, "sixteen channels are created and each is written to");
+
+    int transferred = 0;
+    for (int i = 0; i < made; ++i) {
+        char buf[1] = {};
+        // One millisecond. Every one of these streams has a byte waiting, so a
+        // correct implementation does not reach the bound at all; the bound is
+        // present so that a wait upon the wrong descriptor ends the run rather
+        // than hanging it.
+        if (kal::timeout::read(mine[i], buf, 1, 1000000) == 1 && buf[0] == 'x')
+            ++transferred;
+    }
+    if (transferred != made)
+        std::printf("  %d of %d bounded reads transferred\n", transferred, made);
+    check(transferred == made,
+          "a bounded read of a stream that has bytes waiting transfers them");
+
+    // The mirror. A stream with nothing in it must expire, and must expire
+    // rather than transfer a byte belonging to another stream.
+    kal_stream empty_mine{}, empty_theirs{};
+    if (kal_process_channel(&empty_mine, &empty_theirs) == kal_ok) {
+        char buf[1] = {};
+        check(kal::timeout::read(empty_mine, buf, 1, 1000000) == -kal_err_again,
+              "a bounded read of a stream with nothing waiting expires");
+        kal_process_channel_close(empty_theirs);
+        kal_process_channel_close(empty_mine);
+    }
+
+    for (int i = 0; i < made; ++i) {
+        kal_process_channel_close(theirs[i]);
+        kal_process_channel_close(mine[i]);
+    }
+}
+
 // openkal.timeout
 void timeout_section() {
     check(kal_timeout_granularity() > 0,
@@ -257,6 +326,12 @@ void process_additions_section() {
 }  // namespace
 
 int main() {
+    // FIRST, AND THE ORDER IS LOAD-BEARING. The comment above this section
+    // records why: it examines a defect that corrects itself for any descriptor
+    // index at which an owned handle has already been released, so running it
+    // after the sections that make and release listeners would hold upon the
+    // defect it exists to detect.
+    timeout_stream_section();
     process_additions_section();
     terminal_section();
     net_section();