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..24ee1bdb 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,219 @@ 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]; + +/* #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, + * 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; + } + + /* 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); + 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; +} + +/* 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 +1582,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 +1667,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); @@ -1468,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; } 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/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 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)") + } +}