0.6.1 --- the same bounded-transfer defect, and the tests that had never run (#15)

* 0.6.1 --- the same bounded-transfer defect as openkal-linux, and the tests that
would have caught it

`kal_timeout_read` and `kal_timeout_write` decoded their `kal_stream` argument
with `okm::unpack`, which is the decoder for an OWNED handle. A stream handle is
not one: 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. The wait was therefore
performed upon descriptor N-1 while the transfer that followed was performed
upon N. Of the three standard streams only `kal_stdin`, whose handle is zero,
failed to decode; `kal_stdout` decoded to descriptor 0 and `kal_stderr` to 1.

This is openkal-linux's defect, in this file, unchanged. It was found there
first, through openkal-musl, and the two files carry the same four call sites
with the same two wrong. `kal_timeout_accept` and `kal_timeout_recv_from` keep
the decode and are correct: a listener and a datagram are owned. The four sites
divide exactly along the borrowed/owned line.

--- and the reason it was invisible here ------------------------------------

THIS PACKAGE'S OWN TESTS HAD NEVER RUN. `tests/` has held five suites since the
implementation was written and no workflow invoked them; what ran was the
specification's suite, which is a different instrument. The evidence that they
had never run is in the suites themselves: all five announced their success as
`openkal-linux: ...`, and `conformance_env_time.cpp` did not compile at all,
naming `kal::time::prop_wall_available`, which openkal 0.9 spells
`kal::time::wall_available`.

`mcpp test` is now a step, taken from openkal-linux unchanged, including its
assertion that every suite ran: a suite that discovered nothing reports success.
The five suites are corrected to name this implementation, and the stale
property name is fixed.

`tests/conformance_timeout.cpp` is new and is the observation that was missing.
There was no test of openkal.timeout in this package at all, and the
specification's suite examines it through observations that a wait upon the
wrong descriptor satisfies: a bounded read of the standard input is permitted to
expire, so an implementation that expires for the wrong reason is
indistinguishable there from one that is right.

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 in
`main`, because the mistaken decode corrected itself for every index at which an
owned handle had already been released.

--- measured -----------------------------------------------------------------

This system has no runner here, so the six suites were compiled and run against
openkal-linux, which provides the same interface: 6 passed, 0 failed. The new
suite was then run against openkal-linux with the defect put back:

    0 of 16 bounded reads transferred
    FAIL: a bounded read of a stream that has bytes waiting transfers them

Continuous integration on this branch is the criterion for this system.

`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.

* The suite that had never run finds a second thing: a retry that could not retry

Continuous integration on the previous commit ran `tests/` here for the first
time. Five of the six suites held, including the new one; `conformance_process_task`
did not:

    conformance_process_task ... FAIL (exit 1, 0.04s)
    FAIL: the status it finished with is reported

The observation is correct and the test was wrong. It starts a program that
succeeds and reads its status, and it chose which program by spawning
`bin/true` and RETRYING with `usr/bin/true` if the spawn failed. That retry can
never run: `kal_process_spawn` reports whether the DUPLICATE was made, and the
image is replaced afterwards, inside a copy the caller no longer is. A path that
does not exist therefore produces `kal_ok` and a duplicate that finishes with
127. The first candidate was always taken and the second was unreachable.

This system has `/usr/bin/true` and no `/bin/true`, so the status observed was
127. openkal-linux carries the same test and the same dead retry, and holds only
because the first candidate exists there.

THE OBSERVATION AFTER IT HELD THROUGHOUT, UPON A PROGRAM THAT NEVER RAN. It read
`status != 0` for the program that fails, and 127 satisfies that. So of the two
observations, one failed for the right reason and the other passed for the wrong
one, from a single cause.

What changes:

  * the candidate is chosen by asking `kal_fs_info` whether the name refers to
    anything, which is an enquiry that can distinguish the two cases;
  * `status != 0` becomes `status == 1`, the status `false` actually returns, so
    a program that was never replaced no longer satisfies it;
  * the failing observation prints the status it saw. `127' names an image that
    was not replaced and `0' names one that ran, and without the number the two
    arrive as the same line;
  * the shell candidate is located the same way, for the same reason.

Measured against openkal-linux, which provides the same interface: the suite
holds. Continuous integration on this system is the criterion.

Author: Sunrisepeak

.github/workflows/ci.yml

@@ -215,6 +215,32 @@ jobs:
           }
           subst mcpp.toml .spec
 
+      # THIS PACKAGE'S OWN TESTS HAD NEVER RUN.
+      #
+      # tests/ has held five suites since this implementation was written and no
+      # workflow invoked them. What ran was the specification's suite, which is a
+      # different instrument: it examines what every implementation must do, and
+      # these examine what this one does with the system beneath it. A defect
+      # visible only to the second kind was therefore invisible here, which is
+      # what happened to openkal.timeout -- there was no observation of it in
+      # either place that a wait upon the wrong descriptor did not satisfy.
+      #
+      # The step is openkal-linux's, unchanged, including the assertion that
+      # every suite ran: a suite that discovered nothing reports success.
+      - name: This package's own tests
+        run: |
+          set -euo pipefail
+          mcpp test 2>&1 | tee tests.log
+          # The list is derived from the files present rather than written out
+          # here: a hand-written list names the suites that existed when it was
+          # written, and one added afterwards escapes the assertion silently.
+          missing=0
+          for f in tests/*.cpp; do
+            name="$(basename "$f" .cpp)"
+            grep -q "^$name \.\.\. ok" tests.log || { echo "did not run or did not pass: $name" >&2; missing=1; }
+          done
+          test "$missing" -eq 0
+
       # The other architecture, as far as this system allows it to be reached.
       #
       # The system-call numbers agree between the two --- measured, in the

README.md

@@ -8,7 +8,7 @@ written on the kernel's own calls.
 openkal = "0.9.0"
 
 [target.'cfg(os = "macos")'.dependencies]
-openkal-macos = "0.6.0"
+openkal-macos = "0.6.1"
 ```
 
 Its purpose is as much to test the specification as to be used. A specification

mcpp.toml

@@ -1,7 +1,7 @@
 [package]
 namespace   = "mcpplibs"
 name        = "openkal-macos"
-version     = "0.6.0"
+version     = "0.6.1"
 description = "An implementation of openkal for macOS, written on the kernel's own calls. Its purpose is as much to test the specification as to be used."
 license     = "Apache-2.0"
 

src/handle.h

@@ -24,6 +24,18 @@ inline okm_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(okm_uptr h) {
     const int fd = static_cast<int>(h & 0xffffffffu) - 1;
     if (fd < 0 || fd >= kMaxDescriptor) return -1;

src/timeout.cpp

@@ -59,6 +59,36 @@ int await(int fd, short events, kal_u64 ns) {
     return kal_ok;
 }
 
+// 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
+// okm::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.
+//
+// Found in openkal-linux, whose timeout.cpp is this file's counterpart and
+// carried the same four call sites with the same two wrong.
+int stream_fd(kal_stream s) { return static_cast<int>(s.h); }
+
 }  // namespace
 
 extern "C" {
@@ -68,28 +98,28 @@ 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 = okm::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, static_cast<short>(okm::poll_in), ns); rc != kal_ok)
+    if (const int rc = await(stream_fd(s), static_cast<short>(okm::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  = okm::unpack(s.h);
-    const int use = (fd >= 0) ? fd : static_cast<int>(s.h);
-
-    if (const int rc = await(use, static_cast<short>(okm::poll_out), ns); rc != kal_ok)
+    if (const int rc = await(stream_fd(s), static_cast<short>(okm::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
+// okm::pack and released by okm::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 = okm::unpack(l.h);

tests/conformance_env_time.cpp

@@ -46,11 +46,11 @@ int main() {
 
     // The wall source is claimed by this implementation, so it must report a
     // time after the specification was written rather than zero.
-    check(kal::time::has(kal::time::prop_wall_available), "the wall source is claimed");
+    check(kal::time::has(kal::time::wall_available), "the wall source is claimed");
     check(kal::time::wall() > 1700000000ull * 1000000000ull,
           "the wall source reports a plausible time");
 
-    const char ok[] = "openkal-linux: env and time conformance\n";
+    const char ok[] = "openkal-macos: env and time conformance\n";
     kal::write(kal::out(), ok, sizeof(ok) - 1);
     return failures == 0 ? 0 : 1;
 }

tests/conformance_fs.cpp

@@ -100,7 +100,7 @@ int main() {
     check(kal_fs_remove(root, "okl_dir", 7) == kal_ok, "the directory is removed");
     check(kal_fs_remove(root, "okl_probe.txt", 13) == kal_ok, "the file is removed");
 
-    const char ok[] = "openkal-linux: file system conformance\n";
+    const char ok[] = "openkal-macos: file system conformance\n";
     kal::write(kal::out(), ok, sizeof(ok) - 1);
     return failures == 0 ? 0 : 1;
 }

tests/conformance_memory.cpp

@@ -16,7 +16,7 @@ int main() {
     // cannot be used.
     if (kal::alloc(0, 8) != nullptr) return 1;
 
-    const char ok[] = "openkal-linux: memory conformance\n";
+    const char ok[] = "openkal-macos: memory conformance\n";
     kal::write(kal::out(), ok, sizeof(ok) - 1);
     return 0;
 }

tests/conformance_process_task.cpp

@@ -6,6 +6,20 @@ import openkal.stream;
 
 namespace {
 int failures = 0;
+
+void say(const char* s) {
+    kal_uintptr n = 0; while (s[n]) ++n;
+    kal::write(kal::err(), s, n);
+}
+
+void say_num(int v) {
+    if (v < 0) { kal::write(kal::err(), "-", 1); v = -v; }
+    char b[12]; int i = 12;
+    if (v == 0) b[--i] = '0';
+    while (v > 0) { b[--i] = static_cast<char>('0' + v % 10); v /= 10; }
+    kal::write(kal::err(), b + i, static_cast<kal_uintptr>(12 - i));
+}
+
 void check(bool ok, const char* what) {
     if (ok) return;
     ++failures;
@@ -57,17 +71,52 @@ int main() {
         const char* false_paths[] = { "bin/false", "usr/bin/false" };
         const kal_uintptr false_lens[] = { 9, 13 };
 
+        // THE CANDIDATE IS CHOSEN BY ASKING, AND IT USED TO BE CHOSEN BY
+        // SPAWNING AND RETRYING. That retry could not work and never ran:
+        // kal_process_spawn reports whether the DUPLICATE was made, and the
+        // program is replaced afterwards, inside a copy the caller no longer
+        // is. A path that does not exist therefore produces kal_ok and a
+        // duplicate that finishes with 127, so the first candidate was always
+        // taken and the second was unreachable code.
+        //
+        // On a system holding /usr/bin/true and no /bin/true the consequence
+        // was `status == 127' at the observation below, which is what this
+        // system reported the first time these suites were ever run. The
+        // observation after it -- that a non-zero status is reported as such --
+        // held throughout, upon a program that was never started.
+        auto locate = [&](const char* const* paths, const kal_uintptr* lens_) -> int {
+            for (int i = 0; i < 2; ++i) {
+                kal_node_info info{}; info.self_size = sizeof info;
+                if (kal_fs_info(slash, paths[i], lens_[i], 0, kal::fs::field::kind, &info) != kal_ok)
+                    continue;
+                if (info.kind != kal_node_absent) return i;
+            }
+            return -1;
+        };
+
+        const int t = locate(true_paths, true_lens);
+        const int fpath = locate(false_paths, false_lens);
+        check(t >= 0, "a program that succeeds is found");
+        check(fpath >= 0, "a program that fails is found");
+
         kal_process p{};
         const char* argv[] = { "openkal" };
         const kal_uintptr lens[] = { 7 };
         int rc = kal_err_invalid;
-        for (int i = 0; i < 2 && rc != kal_ok; ++i)
-            rc = kal_process_spawn(slash, true_paths[i], true_lens[i], argv, lens, 1,
+        if (t >= 0)
+            rc = kal_process_spawn(slash, true_paths[t], true_lens[t], argv, lens, 1,
                                    nullptr, nullptr, 0, nullptr, &p);
         check(rc == kal_ok, "a program is started");
         if (rc == kal_ok) {
             int status = -1, terminated = -1;
             check(kal_process_wait(p, &status, &terminated) == kal_ok, "the program is waited for");
+            // The observed status is reported when it is wrong. `127' names an
+            // image that was not replaced and `0' names one that ran; without
+            // the number the two arrive as the same line.
+            if (!(status == 0 && terminated == 0)) {
+                say("  status="); say_num(status);
+                say(" terminated="); say_num(terminated); say("\n");
+            }
             check(status == 0 && terminated == 0, "the status it finished with is reported");
             kal_process_close(p);
         }
@@ -78,13 +127,19 @@ int main() {
         kal_process q{};
         const char* qargv[] = { "openkal" };
         int qrc = kal_err_invalid;
-        for (int i = 0; i < 2 && qrc != kal_ok; ++i)
-            qrc = kal_process_spawn(slash, false_paths[i], false_lens[i], qargv, lens, 1,
+        if (fpath >= 0)
+            qrc = kal_process_spawn(slash, false_paths[fpath], false_lens[fpath], qargv, lens, 1,
                                     nullptr, nullptr, 0, nullptr, &q);
+        check(qrc == kal_ok, "the program that fails is started");
         if (qrc == kal_ok) {
             int status = -1, terminated = -1;
             kal_process_wait(q, &status, &terminated);
-            check(status != 0, "a non-zero status is reported as such");
+            // NOT MERELY NON-ZERO. 127 is what a duplicate reports when the
+            // image was never replaced, so `!= 0' is satisfied by a program
+            // that did not run -- which is precisely how this observation held
+            // while the one above did not.
+            check(status == 1 && terminated == 0,
+                  "the status of a program that fails is its own and not 127");
             kal_process_close(q);
         }
 
@@ -105,12 +160,18 @@ int main() {
         const char* script = "test \"$0\" = openkal-observed-argv0";
         kal_uintptr script_len = 0; while (script[script_len]) ++script_len;
 
+        // Located rather than retried, for the reason recorded above: the retry
+        // this replaces could not distinguish a path that does not exist from
+        // one that does, so it always took the first.
+        const int sh = locate(sh_paths, sh_lens);
+        check(sh >= 0, "a shell is found");
+
         kal_process r{};
         const char*       rargv[] = { "openkal-observed-argv0", "-c", script };
         const kal_uintptr rlens[] = { 22, 2, script_len };
         int rrc = kal_err_invalid;
-        for (int i = 0; i < 2 && rrc != kal_ok; ++i)
-            rrc = kal_process_spawn(slash, sh_paths[i], sh_lens[i], rargv, rlens, 3,
+        if (sh >= 0)
+            rrc = kal_process_spawn(slash, sh_paths[sh], sh_lens[sh], rargv, rlens, 3,
                                     nullptr, nullptr, 0, nullptr, &r);
         check(rrc == kal_ok, "a shell is started");
         if (rrc == kal_ok) {
@@ -153,7 +214,7 @@ int main() {
     kal_task_yield();
     check(kal_task_current() != 0, "the calling context has an identity");
 
-    const char ok[] = "openkal-linux: process and task conformance\n";
+    const char ok[] = "openkal-macos: process and task conformance\n";
     kal::write(kal::out(), ok, sizeof(ok) - 1);
     return failures == 0 ? 0 : 1;
 }

tests/conformance_stream.cpp

@@ -32,7 +32,7 @@ int main() {
     // A write transfers the whole buffer or reports why it could not. The
     // specification excludes a successful partial transfer, so a conforming
     // result reports either the full count or a non-zero error.
-    const char msg[] = "openkal-linux: conformance\n";
+    const char msg[] = "openkal-macos: conformance\n";
     // ⭐ ONE SIGNED WORD: the count, or the negated condition when no byte
     // moved. A caller never inspects two things to learn one thing.
     const kal_intptr r = kal::write(kal::out(), msg, sizeof(msg) - 1);