From 553ffb9e4ce48bf1e83db547c3553834504b71eb Mon Sep 17 00:00:00 2001 From: Paul Hammant Date: Sun, 9 Aug 2026 11:11:30 +0100 Subject: [PATCH 1/4] fix(fs): name the path and the cause in fs.read / fs.read_binary errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both collapsed every failure into a bare constant — "cannot read file" for read_binary, "cannot open file" / "cannot read file" for read — with no path and no errno. Six distinct causes, including sandbox denial and silent truncation, were indistinguishable to the caller, so no Aether program could treat a missing optional file as benign while treating a permission error as fatal. Reported from the aeb line: a 79-target parallel build reported "cannot read file" with no path, which made an intermittent content-hash failure undiagnosable from the caller's side. Before / after, on the ask's own repro: missing : 'cannot read file' -> '/tmp/not-here: No such file or directory' directory : 'cannot read file' -> '/tmp: Is a directory' no-perm : 'cannot read file' -> '/etc/shadow: Permission denied' fs_read_binary_raw now records WHY it is about to return NULL — the reason and errno land in thread-local state that the tuple wrapper reads back immediately after. Its signature is unchanged (it is public and has other callers), and the (bytes, length, err) arity is unchanged, so every existing caller keeps working; the ask explicitly scoped the arity break out. A sandbox refusal gets its own wording — "blocked by sandbox policy (no fs_read grant for this path)" — because it is a POLICY decision, not an I/O error. The file may be present and perfectly readable, and reporting it as a filesystem failure sends whoever is debugging a grant list looking at the disk. fs.read is composed in Aether from file_open_raw + file_read_all_raw, so it cannot capture errno at the failing step itself. New fs_error_message(path, fallback) formats the current errno with the path for exactly that case. Also fixes a silent-truncation bug found while testing this. In file_read_all_raw the seekable fast path never checked ferror, so any failed read returned an EMPTY STRING AS SUCCESS. Reading a directory is the everyday case: fopen("/tmp","r") succeeds on Linux and ftell reports a positive size, so control lands there, fread fails with EISDIR, and fs.read returned ("", "") — success, no content, no error. That is the same class as #1116, which fixed only the streaming branch. A short read with EOF and no error flag (the file shrank mid-read) still returns what was read, as before. Lifetime: the error string is borrowed from thread-local storage and is valid until the thread's next failed read. Documented on both wrappers, because it is a real caller-facing contract — it caught out the first draft of the test, which compared two live messages and was really comparing one buffer with itself. Not done, per the ask's own scoping: no (bytes, length, kind, err) arity change, no raw errno exposed to Aether, no change to the success path or its @heap ownership contract. Tests: tests/regression/test_fs_read_error_detail.ae asserts that failures name the path, that distinct causes give distinct messages, that the old constants are gone, and that the success paths — including the #1116 /proc streaming case — still work. It deliberately does not assert exact libc wording, which varies by platform and locale. Verified valgrind-clean on the success path; the 1-byte-per-call leak remaining on the error path is #1461 (the empty-string tuple slot), confirmed present on pristine main with the same test. Co-Authored-By: Claude Opus 4.8 --- asks/fs-read-binary-loses-error-reason.md | 206 ++++++++++++++ std/fs/aether_fs.c | 256 +++++++++++++++++- std/fs/aether_fs.h | 7 + std/fs/module.ae | 31 ++- tests/regression/test_fs_read_error_detail.ae | 148 ++++++++++ 5 files changed, 636 insertions(+), 12 deletions(-) create mode 100644 asks/fs-read-binary-loses-error-reason.md create mode 100644 tests/regression/test_fs_read_error_detail.ae diff --git a/asks/fs-read-binary-loses-error-reason.md b/asks/fs-read-binary-loses-error-reason.md new file mode 100644 index 00000000..ebb2eaaf --- /dev/null +++ b/asks/fs-read-binary-loses-error-reason.md @@ -0,0 +1,206 @@ +# `fs.read_binary` collapses every failure into `"cannot read file"` + +**From:** the aeb line (2026-08-09) · **Found while:** root-causing an +intermittent aeb build bug where a target reported a cache miss and rebuilt +nothing. The proximate cause was a failed content hash — and the error string +Aether handed back could not say which file failed, or why. + +**Affects:** `v0.506.0` (current main, `b2401b50`). Verified present at HEAD. + +## Summary + +`fs.read_binary(path)` returns `(bytes, length, err)`. On **any** failure the +`err` is the constant string: + +``` +cannot read file +``` + +No `errno`. No path. No kind. Six distinct failure modes — including +**sandbox denial** and **silent truncation** — are indistinguishable to the +caller. + +The machinery to fix this already exists in the same file: the issue-#392 +structured-error pilot (`AETHER_FS_KIND_*`) covers `fs.copy` / `move` / +`realpath` / `chmod`. `read_binary` was simply left out of it. + +## Where the information is lost + +`std/fs/aether_fs.c:1339` — `fs_read_binary_raw` funnels everything to a bare +`NULL`, discarding `errno` at each step: + +```c +char* fs_read_binary_raw(const char* path, int* out_len) { + if (out_len) *out_len = 0; + if (!path) return NULL; + if (!aether_sandbox_check("fs_read", path)) return NULL; // (1) sandbox denial + + FILE* fp = fopen(path, "rb"); + if (!fp) return NULL; // (2) ENOENT/EACCES/EMFILE/EISDIR + + if (fseek(fp, 0, SEEK_END) != 0) { fclose(fp); return NULL; } // (3) not seekable + long size = ftell(fp); + if (size < 0) { fclose(fp); return NULL; } + if (fseek(fp, 0, SEEK_SET) != 0) { fclose(fp); return NULL; } + + size_t alloc_cap = (size_t)size + 1; + char* buf = (char*)aether_caps_malloc(alloc_cap); + if (!buf) { fclose(fp); return NULL; } // (4) OOM / cap exceeded + + size_t read = (size > 0) ? fread(buf, 1, (size_t)size, fp) : 0; + fclose(fp); + if (read != (size_t)size) { // (5) short read / truncation + aether_caps_free(buf, alloc_cap); return NULL; + } + ... +} +``` + +`fs_read_binary_tuple` (`:1421-1427`) then synthesises one constant for all of +them: + +```c + if (!buf) { + out._0 = (void*)string_empty(); + out._1 = 0; + out._2 = "cannot read file"; // <-- every case above lands here + return out; + } +``` + +Note the asymmetry: the very next branch (`:1438`) *does* distinguish +`"allocation failed"` for the wrapper alloc. So the tuple already carries +distinct reasons where the code bothered to produce them. + +`fs.read` (`std/fs/module.ae:378`) has the same shape — +`"cannot open file"` vs `"cannot read file"`, no errno, no path. + +## Why the sandbox case is the worst of them + +Failure (1) is `aether_sandbox_check("fs_read", path)` — a *policy* refusal, +not an I/O error. A program running under `spawn_sandboxed` that reads a +non-granted path is told "cannot read file", which reads as a missing or +corrupt file. That is actively misleading: the file is present and readable, +the grant is missing. Anyone debugging a sandbox policy is sent looking at the +filesystem instead of at their grants. + +## Impact on the caller (concrete, this is how it was found) + +aeb's content-addressed build cache hashes a target's transitive import +closure. `lib/cache/module.ae:221`: + +```aether +hash_file(p: string) { + bytes, length, rerr = fs.read_binary(p) + if string.length(rerr) > 0 { return "", rerr } + return cryptography.sha256_hex(bytes, length) +} +``` + +Any unreadable member of a 27-file closure aborts the key computation, and aeb +falls through to a rebuild. That part is aeb's design and is fine. What is not +fine is that the resulting diagnostic — for a 79-target parallel build — is +`"cannot read file"` with **no path**. There is no way to attribute the failure +to a file, so an intermittent hashing failure under concurrency is +undiagnosable from the caller's side. aeb has its own bug to fix here (it +currently swallows `rerr` entirely), but fixing that only surfaces a constant +string. + +Reproduced deliberately (aeb, aether-ui tree): `chmod 000` one closure member → +the target reports a cache miss and rebuilds, with no indication which file was +unreadable. + +This generalises well past aeb: **no Aether program can currently distinguish +"file absent" from "permission denied" from "I/O error" from "sandbox denied" +on a binary read.** Any tool that wants to treat a missing optional file as +benign but a permission error as fatal cannot do so today. + +## Minimal repro + +```aether +import std.fs +import std.string + +probe(label: string, p: string) { + bytes, len, err = fs.read_binary(p) + println("${label}: err='${err}' len=${len}") +} + +main() { + probe("missing ", "/tmp/definitely-not-here-12345") + probe("directory ", "/tmp") + probe("no-perm ", "/etc/shadow") + return 0 +} +``` + +Actual (v0.506.0) — three different causes, one string: + +``` +missing : err='cannot read file' len=0 +directory : err='cannot read file' len=0 +no-perm : err='cannot read file' len=0 +``` + +Wanted: something that names the cause and the path, e.g. + +``` +missing : err='/tmp/definitely-not-here-12345: no such file or directory' len=0 +directory : err='/tmp: is a directory' len=0 +no-perm : err='/etc/shadow: permission denied' len=0 +``` + +## Ask + +1. **Extend the #392 structured-error pilot to `fs.read_binary`.** The kind + constants already exist in `std/fs/aether_fs.h:15-27` + (`NOT_FOUND`, `PERMISSION_DENIED`, `IS_DIR`, `IO`, `INVALID`, …). Capture + `errno` at each early-return in `fs_read_binary_raw` rather than collapsing + to `NULL`, and map it with the same `errno`→kind helper `fs_copy` uses + (`aether_fs.c:1487`). + +2. **Put the path in the message.** Even without a kind code, `": + permission denied"` would have made this a five-minute diagnosis instead of + a multi-hour one. This alone would resolve most of the pain. + +3. **Give the sandbox denial its own kind/message**, distinct from an I/O + error — a policy refusal is not a filesystem failure. `PERMISSION_DENIED` + with a message that says *sandbox* would do. + +4. **Same treatment for `fs.read`** (`std/fs/module.ae:378`), which has the + identical flaw. Worth doing together — callers pick between them by + text-vs-binary, not by error quality, and the asymmetry is a trap. + +### On compatibility + +The `(bytes, length, err)` arity does **not** need to change. Callers test +`string.length(err) > 0`, which keeps working if the message merely becomes +informative. That makes (2) a safe, standalone first step even if the full +kind-code work in (1) lands later. + +If a kind code is added, the natural shape mirrors `fs.copy` — +`(bytes, length, kind, err)` — but that is an arity break and should be a +deliberate, separately-versioned decision, not a drive-by. **The ask here is +satisfied by (2) + (3) alone**; (1) and (4) are the fuller fix. + +## Not being asked + +- No change to the success path or its `@heap` ownership contract (documented + at `std/fs/module.ae:240-250`) — that part works and the aliasing rules there + are subtle enough to leave alone. +- Not asking for the four-extern split-accessor path + (`fs_try_read_binary` + getters) to grow errors; the tuple shape is the + canonical entry point per #271/#273, and it is the one aeb uses. +- Not asking for `errno` to be exposed as a raw integer to Aether. A kind + constant plus a human-readable message is the right altitude. + +## Related + +- Issue **#392** — the structured-error pilot this asks to extend + (`fs.copy`/`move`/`realpath`/`chmod` already have `(…, kind, message)`). +- Issues **#271** / **#273** — the tuple-return consolidation that produced + `fs_read_binary_tuple` and its `"cannot read file"` constant. +- aeb-side companion bug (being fixed independently, in the aeb repo): the + cache-key path discards this `err` instead of reporting it, and records a + cache-miss marker *before* the build runs, so a hashing failure renders as a + normal rebuild. See `aeb/asks/fanout-reports-miss-but-skips-rebuild.md`. diff --git a/std/fs/aether_fs.c b/std/fs/aether_fs.c index c5f7f1fa..cd9c3084 100644 --- a/std/fs/aether_fs.c +++ b/std/fs/aether_fs.c @@ -61,6 +61,10 @@ const char* fs_get_block_transport(void) { return ""; } char* fs_read_binary_raw(const char* p, int* n) { (void)p; if (n) *n = 0; return NULL; } +const char* fs_error_message(const char* path, const char* fallback) { + (void)path; + return (fallback && *fallback) ? fallback : "fs unavailable"; +} int fs_try_read_binary(const char* p) { (void)p; return 0; } const char* fs_get_read_binary(void) { return NULL; } int fs_get_read_binary_length(void) { return 0; } @@ -277,7 +281,28 @@ char* file_read_all_raw(File* file) { * string_concat. */ char* buffer = (char*)aether_caps_malloc((size_t)size + 1); if (!buffer) return NULL; + errno = 0; size_t read = fread(buffer, 1, (size_t)size, fp); + /* Report a failed or short read instead of returning what we got. + * The streaming path below has always had this ferror check; this + * fast path did not, so any failure here surfaced as a successful + * read of an EMPTY string. Reading a directory is the everyday + * case: on Linux fopen("/tmp","r") succeeds and ftell reports a + * positive size, so control lands here, fread fails with EISDIR, + * and fs.read returned ("", "") — success, no content, no error. + * Same silent-truncation class as #1116, which fixed only the + * streaming branch. */ + if (read != (size_t)size) { + if (!ferror(fp) && feof(fp)) { + /* Genuinely shorter than advertised — the file shrank + * between ftell and fread. Keep what we read rather than + * failing; NUL-terminate at the real length. */ + buffer[read] = '\0'; + return buffer; + } + aether_caps_free(buffer, (size_t)size + 1); + return NULL; + } buffer[read] = '\0'; return buffer; } @@ -1336,18 +1361,208 @@ int64_t fs_get_block_size_bytes(void) { return s_blk_size; } int fs_get_block_removable(void) { return s_blk_removable; } const char* fs_get_block_transport(void) { return s_blk_transport; } +/* ── Why did the last read fail? ──────────────────────────────────────────── + * + * fs_read_binary_raw returns a bare `char*`, so a failure carries no reason — + * which is how six distinct causes (including sandbox denial and silent + * truncation) all surfaced to Aether as the single string "cannot read file", + * with no path and no errno. + * + * Rather than change that signature (it is public and has other callers), the + * reason is recorded here and read back by the tuple wrapper immediately after. + * Thread-local for the same reason s_last_os_error is: concurrent readers must + * not see each other's failures. The message buffer is TLS-owned and borrowed + * by the caller, matching the existing `out._2` contract — the tuple's message + * slot is a `const char*` that Aether never frees. + */ +#define AETHER_FS_READ_FAIL_NONE 0 +#define AETHER_FS_READ_FAIL_INVALID 1 /* NULL path */ +#define AETHER_FS_READ_FAIL_SANDBOX 2 /* policy refusal, NOT an I/O error */ +#define AETHER_FS_READ_FAIL_OPEN 3 /* fopen failed — errno is the detail */ +#define AETHER_FS_READ_FAIL_SEEK 4 /* not seekable (pipe, socket, /proc) */ +#define AETHER_FS_READ_FAIL_ALLOC 5 /* OOM or #343 resource cap */ +#define AETHER_FS_READ_FAIL_IO 6 /* fread set the error flag */ +#define AETHER_FS_READ_FAIL_TRUNCATED 7 /* short read, no error: file shrank */ +#define AETHER_FS_READ_FAIL_UNAVAIL 8 /* built without filesystem support */ + +static AETHER_FS_TLS int s_read_fail_why = AETHER_FS_READ_FAIL_NONE; +static AETHER_FS_TLS int s_read_fail_errno = 0; +static AETHER_FS_TLS char s_read_fail_msg[512]; + +/* Defined below with the rest of the #1378 structured-error machinery; declared + * here so the read path can keep fs_last_os_error() in step without moving an + * existing definition. */ +static AETHER_FS_TLS int s_last_os_error; + +/* Thread-safe strerror into a caller buffer. Plain strerror() shares a static + * buffer, which is exactly wrong for a runtime that spawns actor, scheduler, + * worker and HTTP threads. The three portable spellings disagree about both + * name and return type, hence the ladder: + * - Windows: strerror_s(buf, len, err) -> errno_t + * - GNU: strerror_r(err, buf, len) -> char* (may not use buf!) + * - POSIX/XSI: strerror_r(err, buf, len) -> int + * Always returns a valid NUL-terminated string. */ +static const char* aether_fs_strerror(int err, char* buf, size_t len) { + if (!buf || len == 0) return "unknown error"; + buf[0] = '\0'; +#if defined(_WIN32) + if (strerror_s(buf, len, err) != 0) snprintf(buf, len, "error %d", err); + return buf; +#elif defined(__GLIBC__) && defined(_GNU_SOURCE) + /* GNU strerror_r may return a pointer to an internal string and leave buf + * untouched — use whatever it hands back, not buf. */ + return strerror_r(err, buf, len); +#else + if (strerror_r(err, buf, len) != 0) snprintf(buf, len, "error %d", err); + return buf; +#endif +} + +static void aether_fs_read_fail_reset(void) { + s_read_fail_why = AETHER_FS_READ_FAIL_NONE; + s_read_fail_errno = 0; + s_read_fail_msg[0] = '\0'; +} + +static void aether_fs_read_fail_set(int why, int err) { + s_read_fail_why = why; + s_read_fail_errno = err; + if (err) s_last_os_error = err; /* keep fs_last_os_error() consistent */ +} + +/* Render the recorded failure as ": ". + * + * The path is what made this ask worth filing: a 79-target parallel build + * reporting "cannot read file" with no path is undiagnosable. Long paths are + * truncated from the LEFT ("...ail/of/the/path: reason") because the tail — + * the filename — is the part that identifies the file. + * + * Returns a borrowed pointer into TLS, valid until the next failed read on + * this thread. Never NULL. */ +static const char* aether_fs_read_fail_message(const char* path) { + const char* reason; + char errbuf[128]; + + switch (s_read_fail_why) { + case AETHER_FS_READ_FAIL_INVALID: + return "cannot read file: null path"; + case AETHER_FS_READ_FAIL_SANDBOX: + reason = "blocked by sandbox policy (no fs_read grant for this path)"; + break; + case AETHER_FS_READ_FAIL_ALLOC: + reason = "cannot allocate a buffer for the file " + "(out of memory, or the resource cap refused it)"; + break; + case AETHER_FS_READ_FAIL_SEEK: + reason = s_read_fail_errno + ? aether_fs_strerror(s_read_fail_errno, errbuf, sizeof errbuf) + : "not seekable (a pipe, socket or /proc file?)"; + break; + case AETHER_FS_READ_FAIL_TRUNCATED: + reason = "file changed size during the read (short read)"; + break; + case AETHER_FS_READ_FAIL_UNAVAIL: + return "cannot read file: built without filesystem support"; + case AETHER_FS_READ_FAIL_OPEN: + case AETHER_FS_READ_FAIL_IO: + default: + reason = s_read_fail_errno + ? aether_fs_strerror(s_read_fail_errno, errbuf, sizeof errbuf) + : "cannot read file"; + break; + } + + if (!path || !*path) { + snprintf(s_read_fail_msg, sizeof s_read_fail_msg, "%s", reason); + return s_read_fail_msg; + } + + /* Reserve room for ": " + reason + NUL; give the rest to the path. */ + size_t reason_len = strlen(reason); + size_t budget = sizeof s_read_fail_msg; + size_t path_room = (reason_len + 6 < budget) ? budget - reason_len - 6 : 0; + size_t path_len = strlen(path); + + if (path_room >= 4 && path_len > path_room) { + snprintf(s_read_fail_msg, sizeof s_read_fail_msg, "...%s: %s", + path + (path_len - (path_room - 3)), reason); + } else { + snprintf(s_read_fail_msg, sizeof s_read_fail_msg, "%s: %s", path, reason); + } + return s_read_fail_msg; +} + +/* Public: format ": " for callers that are composed in + * Aether and so cannot reach the TLS state above directly. + * + * `fs.read` is the case this exists for: it is built in Aether from + * file_open_raw + file_read_all_raw, so it cannot capture errno at the failing + * step itself. It calls this immediately after the failure, while errno is + * still the failing call's. Falls back to `fallback` when errno is 0 (some + * paths fail without setting it) so the caller always gets a usable sentence. + * + * Returns a borrowed TLS pointer, valid until this thread's next failed read. */ +const char* fs_error_message(const char* path, const char* fallback) { + int err = errno; + aether_fs_read_fail_reset(); + if (err) { + s_read_fail_why = AETHER_FS_READ_FAIL_OPEN; /* => errno rendering */ + s_read_fail_errno = err; + s_last_os_error = err; + } else { + /* No errno to explain it — carry the caller's wording through the same + * ": " shaping so messages stay uniform. */ + s_read_fail_why = AETHER_FS_READ_FAIL_SEEK; + s_read_fail_errno = 0; + if (fallback && *fallback) { + if (!path || !*path) return fallback; + snprintf(s_read_fail_msg, sizeof s_read_fail_msg, "%s: %s", path, fallback); + return s_read_fail_msg; + } + } + return aether_fs_read_fail_message(path); +} + char* fs_read_binary_raw(const char* path, int* out_len) { if (out_len) *out_len = 0; - if (!path) return NULL; - if (!aether_sandbox_check("fs_read", path)) return NULL; + /* Record WHY we are about to return NULL. Every early return below used to + * collapse to a bare NULL, so the tuple wrapper could only ever report the + * constant "cannot read file" — six distinct causes, one string, no path. + * See fs_read_binary_fail_reason() for how this is turned into a message. */ + aether_fs_read_fail_reset(); + if (!path) { + aether_fs_read_fail_set(AETHER_FS_READ_FAIL_INVALID, 0); + return NULL; + } + /* A sandbox refusal is a POLICY decision, not an I/O error: the file may be + * present and perfectly readable. Reporting it as a filesystem failure sends + * whoever is debugging a grant list looking at the filesystem instead. */ + if (!aether_sandbox_check("fs_read", path)) { + aether_fs_read_fail_set(AETHER_FS_READ_FAIL_SANDBOX, 0); + return NULL; + } + errno = 0; FILE* fp = fopen(path, "rb"); - if (!fp) return NULL; + if (!fp) { + aether_fs_read_fail_set(AETHER_FS_READ_FAIL_OPEN, errno); + return NULL; + } - if (fseek(fp, 0, SEEK_END) != 0) { fclose(fp); return NULL; } + errno = 0; + if (fseek(fp, 0, SEEK_END) != 0) { + aether_fs_read_fail_set(AETHER_FS_READ_FAIL_SEEK, errno); + fclose(fp); return NULL; + } long size = ftell(fp); - if (size < 0) { fclose(fp); return NULL; } - if (fseek(fp, 0, SEEK_SET) != 0) { fclose(fp); return NULL; } + if (size < 0) { + aether_fs_read_fail_set(AETHER_FS_READ_FAIL_SEEK, errno); + fclose(fp); return NULL; + } + if (fseek(fp, 0, SEEK_SET) != 0) { + aether_fs_read_fail_set(AETHER_FS_READ_FAIL_SEEK, errno); + fclose(fp); return NULL; + } // Allocate size+1 so we can append a NUL past the end — handy for // callers who know the content is text and want to treat it as a @@ -1356,11 +1571,29 @@ char* fs_read_binary_raw(const char* path, int* out_len) { // unbounded file size, caller-owned return. size_t alloc_cap = (size_t)size + 1; char* buf = (char*)aether_caps_malloc(alloc_cap); - if (!buf) { fclose(fp); return NULL; } + if (!buf) { + /* Distinct from an I/O error: the read never started. Either genuine + * OOM or the #343 resource cap refusing the allocation — telling a + * caller "cannot read file" when their own cap denied a 2 GB read is + * exactly the misdirection this change exists to remove. */ + aether_fs_read_fail_set(AETHER_FS_READ_FAIL_ALLOC, 0); + fclose(fp); return NULL; + } + errno = 0; size_t read = (size > 0) ? fread(buf, 1, (size_t)size, fp) : 0; + int read_errno = errno; + int truncated = ferror(fp) ? 0 : 1; /* short but no error flag => truncation */ fclose(fp); - if (read != (size_t)size) { aether_caps_free(buf, alloc_cap); return NULL; } + if (read != (size_t)size) { + /* Short read. Two very different causes: a real I/O error (ferror set) + * or the file shrinking between ftell and fread — a race that silently + * truncates the caller's data. Neither should read as "cannot read". */ + aether_fs_read_fail_set(truncated ? AETHER_FS_READ_FAIL_TRUNCATED + : AETHER_FS_READ_FAIL_IO, + read_errno); + aether_caps_free(buf, alloc_cap); return NULL; + } buf[size] = '\0'; if (out_len) *out_len = (int)size; @@ -1423,7 +1656,12 @@ _tuple_ptr_int_string fs_read_binary_tuple(const char* path) { // of a null deref. out._0 = (void*)string_empty(); out._1 = 0; - out._2 = "cannot read file"; + /* Was the constant "cannot read file" for every cause. Now names the + * path and the actual reason — see aether_fs_read_fail_message. The + * pointer is borrowed from TLS and stays valid until this thread's + * next failed read, which is the same contract the other messages in + * this tuple already have (they are static literals). */ + out._2 = aether_fs_read_fail_message(path); return out; } AetherString* wrapped = string_new_with_length(buf, (size_t)len); diff --git a/std/fs/aether_fs.h b/std/fs/aether_fs.h index c404f21a..7bf90198 100644 --- a/std/fs/aether_fs.h +++ b/std/fs/aether_fs.h @@ -157,6 +157,13 @@ int64_t fs_get_stat_mtime(void); // this is for the cases that need the exact number. int fs_last_os_error(void); +/* Format ": " from the CURRENT errno, for callers composed in + * Aether that cannot capture errno at the failing step themselves (fs.read). + * Call it immediately after the failure. `fallback` is used when errno is 0. + * Returns a borrowed thread-local pointer, valid until this thread's next + * failed read — copy it if you need to keep it. */ +const char* fs_error_message(const char* path, const char* fallback); + // statvfs (#1117): exact filesystem byte counts for the fs containing `path`. // Same split try/get shape as fs_try_stat. total/free/avail are bytes; avail // is space usable by an unprivileged process (f_bavail). fs_try_statvfs diff --git a/std/fs/module.ae b/std/fs/module.ae index 98a05c30..60722911 100644 --- a/std/fs/module.ae +++ b/std/fs/module.ae @@ -63,7 +63,7 @@ exports( KIND_CROSS_DEVICE, KIND_IO, KIND_INVALID, KIND_LOOP, KIND_NAME_TOO_LONG, KIND_NO_SPACE, KIND_IS_DIR, KIND_NOT_DIR, KIND_UNAVAILABLE, - fs_last_os_error, last_os_error + fs_last_os_error, last_os_error, fs_error_message ) // ---- Structured-error kinds (pilot — issue #392) ---- @@ -173,6 +173,7 @@ extern fs_rename_raw(from: string, to: string) -> int // for this only when the exact number matters, such as telling EAGAIN from // EWOULDBLOCK or putting the number in a log. extern fs_last_os_error() -> int +extern fs_error_message(path: string, fallback: string) -> string last_os_error() -> int { return fs_last_os_error() @@ -375,15 +376,27 @@ open(path: string, mode: string) -> { // Read the entire contents of a file at `path`. Opens, reads, closes. // Returns (content, "") on success, ("", error) on failure. +// +// The error names the path and the cause — ": No such file or +// directory", ": Is a directory", ": Permission denied" — rather +// than the single "cannot open file" this used to return for every cause. +// +// LIFETIME: the error string is BORROWED from thread-local storage and is only +// valid until this thread's next failed read. Print it, or copy it with +// `string.concat(err, "")`, before issuing another read — do NOT hold it across +// one. Do not free it. (The success value IS owned and must be freed.) read(path: string) -> { handle = file_open_raw(path, "r") if handle == 0 { - return "", "cannot open file" + // Was the bare "cannot open file" — no path, no reason, so a missing + // file, a permission error and a sandbox denial were indistinguishable. + // fs_error_message reads errno while it is still this call's. + return "", fs_error_message(path, "cannot open file") } content = file_read_all_raw(handle) if content == 0 { file_close(handle) - return "", "cannot read file" + return "", fs_error_message(path, "cannot read file") } content_copy = string_concat(content, "") file_close(handle) @@ -767,6 +780,18 @@ block_info(dev: string) -> { // silently truncate at the first embedded NUL. The TLS buffer inside // the runtime is released after the copy completes, so the caller // doesn't have to manage lifetime. +// +// On failure the error names the path and the cause — ": No such file +// or directory", ": Permission denied", or, for a sandbox refusal, +// ": blocked by sandbox policy (no fs_read grant for this path)". That +// last one is deliberately distinct: a policy refusal is not a filesystem +// failure, and reporting it as one sends people looking at the disk instead of +// at their grants. +// +// LIFETIME: the error string is BORROWED from thread-local storage, valid only +// until this thread's next failed read. Print it, or copy it with +// `string.concat(err, "")`, before issuing another read. Do not free it. The +// bytes value IS owned and must be freed. read_binary(path: string) -> { // Single tuple-returning extern (#271 + #273). The four-extern // split-accessor pattern (fs_try_read_binary + fs_get_read_binary diff --git a/tests/regression/test_fs_read_error_detail.ae b/tests/regression/test_fs_read_error_detail.ae new file mode 100644 index 00000000..cb09059e --- /dev/null +++ b/tests/regression/test_fs_read_error_detail.ae @@ -0,0 +1,148 @@ +// fs.read / fs.read_binary must say WHICH file failed and WHY. +// +// Both used to collapse every failure into a bare constant — "cannot read file" +// / "cannot open file" — with no path and no errno. Six distinct causes, +// including sandbox denial and silent truncation, were indistinguishable, so a +// caller could not treat a missing optional file as benign while treating a +// permission error as fatal. Reported from the aeb line: a 79-target parallel +// build reported "cannot read file" with no path, making an intermittent +// hashing failure undiagnosable. +// +// This test pins the two properties that matter to a caller: +// 1. the message names the path, and +// 2. distinct causes produce distinct messages. +// +// It deliberately does NOT assert exact libc wording — strerror text varies by +// platform and locale ("No such file or directory" vs "no such file"). Asserting +// it would make this test a portability trap. Distinctness plus the path is the +// contract; the wording is libc's business. +import std.fs +import std.string + +// Every failure message must contain the path and must not be the old constant. +check_names_path(label: string, path: string, err: string) -> int { + if string.length(err) == 0 { + println(" FAIL ${label}: expected an error, got none") + return 1 + } + if string.contains(err, path) != 1 { + println(" FAIL ${label}: message does not name the path: '${err}'") + return 1 + } + if string.equals(err, "cannot read file") == 1 { + println(" FAIL ${label}: still the old collapsed constant") + return 1 + } + if string.equals(err, "cannot open file") == 1 { + println(" FAIL ${label}: still the old collapsed constant") + return 1 + } + println(" PASS ${label}: ${err}") + return 0 +} + +main() { + println("=== fs read errors name the path and the cause ===") + fails = 0 + + missing = "/tmp/aether-definitely-not-here-9471" + // A directory: fopen() succeeds on Linux, so this exercises the read-time + // failure rather than the open-time one. It previously returned ("", "") — + // success with empty content — which is worse than a bad message. + a_dir = "/tmp" + + // --- fs.read --- + println("fs.read:") + c1, e1 = fs.read(missing) + fails = fails + check_names_path("missing file ", missing, e1) + // COPY before the next call. The message is a borrowed pointer into + // thread-local storage, valid only until this thread's next failed read — + // comparing two live messages would compare one buffer with itself. + e1_copy = string.concat(e1, "") + string.free(c1) + + c2, e2 = fs.read(a_dir) + fails = fails + check_names_path("directory ", a_dir, e2) + string.free(c2) + + // The two causes must not produce the same text. + if string.equals(e1_copy, e2) == 1 { + println(" FAIL: missing-file and directory give the SAME message") + fails = fails + 1 + } else { + println(" PASS: distinct causes give distinct messages") + } + + // --- fs.read_binary --- + println("fs.read_binary:") + b1, n1, be1 = fs.read_binary(missing) + fails = fails + check_names_path("missing file ", missing, be1) + be1_copy = string.concat(be1, "") // borrowed TLS — copy before reuse + if n1 != 0 { + println(" FAIL: length should be 0 on failure, got ${n1}") + fails = fails + 1 + } + string.free(b1) + + b2, n2, be2 = fs.read_binary(a_dir) + fails = fails + check_names_path("directory ", a_dir, be2) + string.free(b2) + + if string.equals(be1_copy, be2) == 1 { + println(" FAIL: missing-file and directory give the SAME message") + fails = fails + 1 + } else { + println(" PASS: distinct causes give distinct messages") + } + + // --- the success path must be untouched --- + println("success path:") + ok, oerr = fs.read("VERSION") + if string.length(oerr) != 0 { + println(" FAIL: reading VERSION errored: ${oerr}") + fails = fails + 1 + } else { + if string.length(ok) == 0 { + println(" FAIL: reading VERSION gave empty content") + fails = fails + 1 + } else { + println(" PASS: fs.read(VERSION) still works") + } + } + string.free(ok) + + bok, bn, boerr = fs.read_binary("VERSION") + if string.length(boerr) != 0 || bn == 0 { + println(" FAIL: read_binary(VERSION) errored: '${boerr}' len=${bn}") + fails = fails + 1 + } else { + println(" PASS: fs.read_binary(VERSION) still works") + } + string.free(bok) + + // /proc/self/status is size-0-by-stat and must stream (#1116). The + // fast-path ferror fix must not have broken it. + ps, pserr = fs.read("/proc/self/status") + if string.length(pserr) == 0 { + if string.length(ps) > 100 { + println(" PASS: /proc streaming read still works (#1116)") + } else { + println(" FAIL: /proc read returned ${string.length(ps)} bytes") + fails = fails + 1 + } + } else { + // Not fatal — a non-Linux box has no /proc. + println(" SKIP: /proc/self/status unavailable (${pserr})") + } + string.free(ps) + + string.free(e1_copy) + string.free(be1_copy) + + println("") + if fails == 0 { + println("All PASS") + } else { + println("${fails} FAILURE(S)") + } +} From 83d40d3a46eed3108df5fd6487f2163e9a39cc3d Mon Sep 17 00:00:00 2001 From: Paul Hammant Date: Sun, 9 Aug 2026 11:41:39 +0100 Subject: [PATCH 2/4] fix(fs): make the message truncation provably safe for -Werror=format-truncation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The path/reason budget was computed by hand, which was correct but not *provably* correct: gcc could not see that a 127-byte strerror string plus a path always fits the 512-byte buffer, so -Werror=format-truncation failed the build on six CI jobs (Linux GCC/Clang/Hardened, both Windows, macOS ARM64). My local build did not use -Werror, which is why it passed here. std/fs/aether_fs.c:1490:64: error: '%s' directive output may be truncated writing up to 127 bytes into a region of size between 4 and 510 [-Werror=format-truncation=] Replaced the arithmetic with precision specifiers — "%s%.*s: %.*s" with fixed caps — so each field is bounded by a compile-visible constant and the total cannot exceed the buffer. Same behaviour, provable to the compiler. Verified locally under the three configurations CI uses: gcc -Werror -Wformat-truncation=2, clang -Werror, and the HARDEN=1 flag set (-fstack-protector-all -D_FORTIFY_SOURCE=2 -Wformat-security). All clean. Behaviour unchanged: the regression test still passes, and a 1060-character path renders as a 280-byte message, left-truncated with "..." so the filename — the part that identifies the file — is preserved. Co-Authored-By: Claude Opus 4.8 --- std/fs/aether_fs.c | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/std/fs/aether_fs.c b/std/fs/aether_fs.c index cd9c3084..8d7ade2d 100644 --- a/std/fs/aether_fs.c +++ b/std/fs/aether_fs.c @@ -1477,18 +1477,24 @@ static const char* aether_fs_read_fail_message(const char* path) { return s_read_fail_msg; } - /* Reserve room for ": " + reason + NUL; give the rest to the path. */ - size_t reason_len = strlen(reason); - size_t budget = sizeof s_read_fail_msg; - size_t path_room = (reason_len + 6 < budget) ? budget - reason_len - 6 : 0; + /* Bound BOTH fields with precision specifiers rather than computing a + * budget by hand. `%.*s` caps each one at compile-visible limits, so the + * total can never exceed the buffer and gcc's -Wformat-truncation can see + * that — an earlier hand-rolled version was correct but not *provably* so, + * and failed the -Werror build. + * + * The path is truncated from the LEFT ("...tail/of/path") because the tail + * — the filename — is what identifies the file. */ + enum { REASON_MAX = 200, PATH_MAX_SHOWN = 250 }; size_t path_len = strlen(path); - - if (path_room >= 4 && path_len > path_room) { - snprintf(s_read_fail_msg, sizeof s_read_fail_msg, "...%s: %s", - path + (path_len - (path_room - 3)), reason); - } else { - snprintf(s_read_fail_msg, sizeof s_read_fail_msg, "%s: %s", path, reason); - } + const char* path_shown = path; + const char* ellipsis = ""; + if (path_len > PATH_MAX_SHOWN) { + path_shown = path + (path_len - PATH_MAX_SHOWN); + ellipsis = "..."; + } + snprintf(s_read_fail_msg, sizeof s_read_fail_msg, "%s%.*s: %.*s", + ellipsis, (int)PATH_MAX_SHOWN, path_shown, (int)REASON_MAX, reason); return s_read_fail_msg; } From 1dd64fa9a4b9c82a3dec53e9894cc5d84e734d62 Mon Sep 17 00:00:00 2001 From: Paul Hammant Date: Sun, 9 Aug 2026 11:51:08 +0100 Subject: [PATCH 3/4] =?UTF-8?q?fix(fs):=20one=20definition=20of=20s=5Flast?= =?UTF-8?q?=5Fos=5Ferror=20=E2=80=94=20MinGW=20rejects=20a=20tentative=20?= =?UTF-8?q?=5F=5Fthread?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both Windows CI jobs failed with: std/fs/aether_fs.c:1716:26: error: redefinition of 's_last_os_error' The read-error block needs to write s_last_os_error, but the variable is defined further down the file, so the first version forward-declared it as `static AETHER_FS_TLS int s_last_os_error;` and left the initialised definition in place below. That is a tentative definition, and C's tentative-definition rule does NOT extend to __thread objects on MinGW-GCC — it accepts exactly one definition. glibc/GCC on Linux, Clang, and macOS all accept the pair, which is why this survived three green non-Windows jobs before the Windows ones caught it. Fixed by moving the single definition up to the read-error block and leaving a comment at the old site explaining why it cannot move back. Verified on winbaz (real MSYS2 MINGW64) rather than by another CI round-trip: the redefinition error is gone. The one remaining diagnostic there ('aether_fs_iks_err_partial' defined but not used) reproduces identically on pristine main with the same ad-hoc flags, so it is not from this change — the real Windows CI build does not enable that warning. Behaviour unchanged; the regression test still passes. Co-Authored-By: Claude Opus 4.8 --- std/fs/aether_fs.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/std/fs/aether_fs.c b/std/fs/aether_fs.c index 8d7ade2d..24ee1bdb 100644 --- a/std/fs/aether_fs.c +++ b/std/fs/aether_fs.c @@ -1389,10 +1389,15 @@ static AETHER_FS_TLS int s_read_fail_why = AETHER_FS_READ_FAIL_NONE; static AETHER_FS_TLS int s_read_fail_errno = 0; static AETHER_FS_TLS char s_read_fail_msg[512]; -/* Defined below with the rest of the #1378 structured-error machinery; declared - * here so the read path can keep fs_last_os_error() in step without moving an - * existing definition. */ -static AETHER_FS_TLS int s_last_os_error; +/* #1378: the raw OS code behind the portable kind — see fs_last_os_error(). + * + * DEFINED here rather than forward-declared. A tentative definition + * (`static __thread int x;` followed later by `static __thread int x = 0;`) is + * accepted by glibc/GCC on Linux but rejected by MinGW-GCC with "redefinition + * of 's_last_os_error'", because __thread objects do not get C's + * tentative-definition treatment there. Both Windows CI jobs caught this after + * Linux, Clang and macOS all built clean. */ +static AETHER_FS_TLS int s_last_os_error = 0; /* Thread-safe strerror into a caller buffer. Plain strerror() shares a static * buffer, which is exactly wrong for a runtime that spawns actor, scheduler, @@ -1712,8 +1717,10 @@ typedef struct { * from the kind alone, which is deliberately coarse and portable. Recorded at * the single translation site below so it can never drift from the kind it * accompanies. Thread-local, like the stat accessors, so concurrent callers do - * not read each other's value. */ -static AETHER_FS_TLS int s_last_os_error = 0; + * not read each other's value. + * + * The definition moved up to the read-error block above, which also writes it; + * MinGW rejects a tentative __thread definition, so there can only be one. */ int fs_last_os_error(void) { return s_last_os_error; } From 9c6410550f1c912aee1e8582d2f20130ab00ad7f Mon Sep 17 00:00:00 2001 From: Paul Hammant Date: Sun, 9 Aug 2026 12:15:24 +0100 Subject: [PATCH 4/4] test(fs): budget the known #1461 empty-string leak for the macOS leaks gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The macOS leaks(1) gate failed the new test: [FAIL] test_fs_read_error_detail: 3 leaks (allowed ≤ 0) The test exercises fs.read / fs.read_binary FAILURE paths, which return ("", ). The bare "" literal in the value slot is heap-allocated and never freed — 1 byte per failing call, and NOT reachable from the caller: string.free() and string_release() on the returned value both fail to reclaim it, and the leak is attributed to the caller's frame rather than to std.fs. That is #1461, not this change. Confirmed pre-existing on main: an equivalent 4-line program using only json.parse — no fs at all — leaks identically, while a plain string.to_double() tuple does not, so it is specific to this return shape rather than to tuples in general. tests/leaks_known.txt already documents the same family for test_rsa_pkcs1 (the #1311 quirk on cross-module (ptr, string) returns, whose fix heap-classified nested tuple-destructure ERROR slots); this is the mirror case — the "" literal sharing a slot with heap strings. Budgeted rather than papered over. The cap is 4, the exact number of failing calls the test makes (2x fs.read, 2x fs.read_binary), so the entry cannot silently absorb a NEW leak. Measured 2 under Linux valgrind and 3 under macOS leaks(1) — the tools disagree on how many they attribute — so the call count is the honest ceiling. The entry says to delete it once #1461 lands. I should have checked the leaks gate rather than only checking that the leak pre-existed; the gate is exactly the mechanism that turns "known" into "budgeted". Co-Authored-By: Claude Opus 4.8 --- tests/leaks_known.txt | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/leaks_known.txt b/tests/leaks_known.txt index 8c54e26c..fdbb5bea 100644 --- a/tests/leaks_known.txt +++ b/tests/leaks_known.txt @@ -25,3 +25,26 @@ test_cryptography_hmac_md4_md5 122 # codegen (nested tuple-destructure error slots are heap-classified), so the # remaining count is the OpenSSL atexit artifact only. Cap kept as a ceiling. test_rsa_pkcs1 130 + +# ── KNOWN CODEGEN BUG: the empty-string ("") tuple slot (#1461) ────── +# test_fs_read_error_detail exercises fs.read / fs.read_binary FAILURE +# paths, which return ("", ). The bare "" literal in the value +# slot is heap-allocated but never freed — 1 byte per failing call. It +# is NOT reachable from the caller: freeing the returned value with +# string.free() or string_release() does not reclaim it, and the leak +# is attributed to the CALLER's frame, not to std.fs. +# +# Same family as the #1311 quirk described for test_rsa_pkcs1 above: +# that fix heap-classified nested tuple-destructure ERROR slots; this +# is the mirror case, the "" literal sharing a slot with heap strings. +# Confirmed pre-existing on main — an equivalent 4-line program using +# only json.parse (no fs at all) leaks identically, and a plain +# string.to_double() tuple does not, so it is specific to this shape +# rather than to tuples in general. +# +# Cap is 4: the test makes exactly 4 failing calls (2x fs.read, +# 2x fs.read_binary). Measured 2 on Linux/valgrind and 3 on macOS +# leaks(1) — the tools differ on whether they see all of them. Set to +# the call count so the entry cannot silently absorb a NEW leak, and +# drop this entry entirely once #1461 lands. +test_fs_read_error_detail 4