andrew/libakstdlib
1
0
Fork 0
Code Issues 33 Pull Requests 3 Actions 1 Packages Projects Releases Wiki Activity

Wrap file metadata calls #31

Merged
andrew merged 3 commits from 9 into main 2026-08-03 11:05:45 -04:00
Conversation 10 Commits 3 Files Changed 4 +91 -10
3 changed files with 91 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 15e9104d9e - Show all commits

32
include/akstdlib.h
View File

@@ -76,6 +76,7 @@
#include <stddef.h>
#include <stdint.h>
#include <stdio.h>
#include <fcntl.h>
#include <sys/stat.h>
logikoma marked this conversation as resolved
tachikoma commented 2026-08-03 09:59:20 -04:00
Review
Copy Link

DEFECT (high) — the header is not self-contained for aksl_fstatat.

Line 79-80 add <sys/stat.h> and <sys/statvfs.h>, but not <fcntl.h>. POSIX declares AT_FDCWD, AT_SYMLINK_NOFOLLOW, AT_EMPTY_PATH and AT_NO_AUTOMOUNT in <fcntl.h>, not in <sys/stat.h>. The aksl_fstatat doc block on line 838-844 tells the caller to pass AT_FDCWD and "libc fstatat flags", and then does not give them a way to spell either one.

Verified. A translation unit whose only include is <akstdlib.h>:

#include <akstdlib.h>
int main(void)
{
    struct stat st;
    akerr_ErrorContext *e = aksl_fstatat(AT_FDCWD, ".", &st, AT_SYMLINK_NOFOLLOW);
    (void)e;
    return 0;
}

error: 'AT_FDCWD' undeclared (first use in this function)
error: 'AT_SYMLINK_NOFOLLOW' undeclared (first use in this function)

Adding #include <fcntl.h> to the test file compiles it clean. The suite never caught this because tests/test_stat.c:4 includes <fcntl.h> itself, so the test is not testing the header the way a consumer sees it.

Fix: add #include <fcntl.h> here, beside the other two.

**DEFECT (high) — the header is not self-contained for `aksl_fstatat`.** Line 79-80 add `<sys/stat.h>` and `<sys/statvfs.h>`, but not `<fcntl.h>`. POSIX declares `AT_FDCWD`, `AT_SYMLINK_NOFOLLOW`, `AT_EMPTY_PATH` and `AT_NO_AUTOMOUNT` in `<fcntl.h>`, not in `<sys/stat.h>`. The `aksl_fstatat` doc block on line 838-844 tells the caller to pass `AT_FDCWD` and "libc fstatat flags", and then does not give them a way to spell either one. Verified. A translation unit whose only include is `<akstdlib.h>`: ```c #include <akstdlib.h> int main(void) { struct stat st; akerr_ErrorContext *e = aksl_fstatat(AT_FDCWD, ".", &st, AT_SYMLINK_NOFOLLOW); (void)e; return 0; } ``` ``` error: 'AT_FDCWD' undeclared (first use in this function) error: 'AT_SYMLINK_NOFOLLOW' undeclared (first use in this function) ``` Adding `#include <fcntl.h>` to the test file compiles it clean. The suite never caught this because `tests/test_stat.c:4` includes `<fcntl.h>` itself, so the test is not testing the header the way a consumer sees it. **Fix:** add `#include <fcntl.h>` here, beside the other two.
#include <sys/statvfs.h>
/* off_t, for the aksl_fseeko/aksl_ftello pair. POSIX, like aksl_realpath. */
@@ -810,10 +811,25 @@ akerr_ErrorContext AKERR_NOIGNORE *aksl_strhash_djb2_str(const char *str, uint32
/** @} */
/* ====================================================================== */
/** @name File and filesystem metadata
*
* The stat family reports into caller-owned POSIX structs rather than a smaller
* library-defined copy: the platform owns the fields, and a wrapper should not
* discard a field merely because this library does not currently use it. libc
logikoma marked this conversation as resolved Outdated
tachikoma commented 2026-08-03 09:59:20 -04:00
Outdated
Review
Copy Link

DEFECT (medium) — no @throws on any of the six.

The header carries 293 @throws tags across 146 aksl_ declarations. The declaration immediately above this one, aksl_strhash_djb2_str on line 810, documents @throws AKERR_NULLPOINTER If either pointer is NULL. These six document none.

That leaves the only non-obvious half of the contract undocumented. A consumer reading the header cannot learn from it that a NULL argument yields AKERR_NULLPOINTER, or — much more importantly — that a libc failure surfaces the raw errno value as the status with AKERR_IO only as a fallback when errno is 0. That second one is the whole design decision the PR body is announcing, and the header is where a consumer would look for it.

Fix: on each of the six, add the statuses. For example:

 * @throws AKERR_NULLPOINTER If pathname or dest is NULL.
 * @throws AKERR_IO If stat(2) failed and left errno at 0.
 * @throws (errno) The errno stat(2) set, reported directly as the status.
**DEFECT (medium) — no `@throws` on any of the six.** The header carries 293 `@throws` tags across 146 `aksl_` declarations. The declaration immediately above this one, `aksl_strhash_djb2_str` on line 810, documents `@throws AKERR_NULLPOINTER If either pointer is NULL.` These six document none. That leaves the only non-obvious half of the contract undocumented. A consumer reading the header cannot learn from it that a NULL argument yields `AKERR_NULLPOINTER`, or — much more importantly — that a libc failure surfaces the **raw errno value** as the status with `AKERR_IO` only as a fallback when errno is 0. That second one is the whole design decision the PR body is announcing, and the header is where a consumer would look for it. **Fix:** on each of the six, add the statuses. For example: ``` * @throws AKERR_NULLPOINTER If pathname or dest is NULL. * @throws AKERR_IO If stat(2) failed and left errno at 0. * @throws (errno) The errno stat(2) set, reported directly as the status. ```
* failures retain their errno value as the status, so callers can distinguish
* absent paths from inaccessible ones without parsing an error message.
* @{
*/
/* ====================================================================== */
/**
* @brief stat(2).
* @param[in] pathname Path to inspect. Required.
* @param[out] dest File metadata. Required.
* @throws AKERR_NULLPOINTER If pathname or dest is NULL.
* @throws AKERR_IO If stat(2) failed and left errno at 0.
* @throws (errno) The errno stat(2) set, reported directly as the status.
* @return NULL on success, an error context otherwise.
*/
akerr_ErrorContext AKERR_NOIGNORE *aksl_stat(const char *pathname, struct stat *dest);
@@ -822,6 +838,9 @@ akerr_ErrorContext AKERR_NOIGNORE *aksl_stat(const char *pathname, struct stat *
* @brief lstat(2), inspecting a symbolic link itself.
* @param[in] pathname Path to inspect. Required.
* @param[out] dest File metadata. Required.
* @throws AKERR_NULLPOINTER If pathname or dest is NULL.
* @throws AKERR_IO If lstat(2) failed and left errno at 0.
* @throws (errno) The errno lstat(2) set, reported directly as the status.
* @return NULL on success, an error context otherwise.
*/
akerr_ErrorContext AKERR_NOIGNORE *aksl_lstat(const char *pathname, struct stat *dest);
@@ -830,6 +849,9 @@ akerr_ErrorContext AKERR_NOIGNORE *aksl_lstat(const char *pathname, struct stat
* @brief fstat(2).
* @param[in] fd Open file descriptor.
* @param[out] dest File metadata. Required.
* @throws AKERR_NULLPOINTER If dest is NULL.
* @throws AKERR_IO If fstat(2) failed and left errno at 0.
* @throws (errno) The errno fstat(2) set, reported directly as the status.
* @return NULL on success, an error context otherwise.
*/
akerr_ErrorContext AKERR_NOIGNORE *aksl_fstat(int fd, struct stat *dest);
@@ -840,6 +862,9 @@ akerr_ErrorContext AKERR_NOIGNORE *aksl_fstat(int fd, struct stat *dest);
* @param[in] pathname Path to inspect. Required.
* @param[out] dest File metadata. Required.
* @param[in] flags libc fstatat flags, passed unchanged.
* @throws AKERR_NULLPOINTER If pathname or dest is NULL.
* @throws AKERR_IO If fstatat(2) failed and left errno at 0.
* @throws (errno) The errno fstatat(2) set, reported directly as the status.
* @return NULL on success, an error context otherwise.
*/
akerr_ErrorContext AKERR_NOIGNORE *aksl_fstatat(int dirfd, const char *pathname, struct stat *dest, int flags);
@@ -848,6 +873,9 @@ akerr_ErrorContext AKERR_NOIGNORE *aksl_fstatat(int dirfd, const char *pathname,
* @brief statvfs(3).
* @param[in] path Path on the filesystem. Required.
* @param[out] dest Filesystem metadata. Required.
* @throws AKERR_NULLPOINTER If path or dest is NULL.
* @throws AKERR_IO If statvfs(3) failed and left errno at 0.
* @throws (errno) The errno statvfs(3) set, reported directly as the status.
* @return NULL on success, an error context otherwise.
*/
akerr_ErrorContext AKERR_NOIGNORE *aksl_statvfs(const char *path, struct statvfs *dest);
@@ -856,11 +884,15 @@ akerr_ErrorContext AKERR_NOIGNORE *aksl_statvfs(const char *path, struct statvfs
* @brief fstatvfs(3).
* @param[in] fd Open file descriptor.
* @param[out] dest Filesystem metadata. Required.
* @throws AKERR_NULLPOINTER If dest is NULL.
* @throws AKERR_IO If fstatvfs(3) failed and left errno at 0.
* @throws (errno) The errno fstatvfs(3) set, reported directly as the status.
* @return NULL on success, an error context otherwise.
*/
akerr_ErrorContext AKERR_NOIGNORE *aksl_fstatvfs(int fd, struct statvfs *dest);
/** @} */
/* ====================================================================== */
/** @name Streams: open, read, write, close
*

32
src/stat.c
View File

@@ -1,6 +1,20 @@
/* sys/stat.h and sys/statvfs.h metadata wrappers. */
/*
logikoma marked this conversation as resolved Outdated
tachikoma commented 2026-08-03 09:59:20 -04:00
Outdated
Review
Copy Link

Style — no file-level rationale block.

One line, where the three most recently added wrapper files all open with a paragraph explaining why the wrappers are worth having and what conventions run through the file:

file file-level block
src/stream.c:1-17 17 lines: "the recurring theme, and the reason most of these are worth wrapping at all, is that stdio reports failure through a return value that is easy to mistake for data..."
src/string.c:1-12+ counts the raw call sites in akbasic that motivated it
src/collections.c:1-12+ states what it is not, then the two conventions that hold throughout
src/stat.c:1 /* sys/stat.h and sys/statvfs.h metadata wrappers. */

The per-function blocks added in 01034fc are genuinely good — aksl_fstat's "no path lookup race and remains useful after the file has been renamed or unlinked" is exactly the right kind of note. The file just has no equivalent at the top saying why a caller reaches for this file at all, and where the errno-as-status policy is stated once for all six.

**Style — no file-level rationale block.** One line, where the three most recently added wrapper files all open with a paragraph explaining why the wrappers are worth having and what conventions run through the file: | file | file-level block | |---|---| | `src/stream.c:1-17` | 17 lines: "the recurring theme, and the reason most of these are worth wrapping at all, is that stdio reports failure through a return value that is easy to mistake for data..." | | `src/string.c:1-12+` | counts the raw call sites in akbasic that motivated it | | `src/collections.c:1-12+` | states what it is *not*, then the two conventions that hold throughout | | `src/stat.c:1` | `/* sys/stat.h and sys/statvfs.h metadata wrappers. */` | The per-function blocks added in 01034fc are genuinely good — `aksl_fstat`'s "no path lookup race and remains useful after the file has been renamed or unlinked" is exactly the right kind of note. The file just has no equivalent at the top saying why a caller reaches for this file at all, and where the errno-as-status policy is stated once for all six.
* sys/stat.h and sys/statvfs.h metadata wrappers.
*
* These calls make information about a file or filesystem available only by a
logikoma marked this conversation as resolved Outdated
tachikoma commented 2026-08-03 09:59:20 -04:00
Outdated
Review
Copy Link

Style — include-what-you-use.

stat, lstat, fstat, fstatat, statvfs and fstatvfs are all declared in <sys/stat.h> / <sys/statvfs.h>, and this file gets both only transitively through <akstdlib.h>. src/stream.c:19-27 re-includes <stdio.h>, <stdlib.h>, <string.h> and <sys/types.h> explicitly even though akstdlib.h already supplies every one of them.

Same file, the grouping differs too: stream.c separates the project header, the system headers, and "aksl_internal.h" with blank lines. Here all four run together.

Fix:

#include <akstdlib.h>

#include <errno.h>
#include <sys/stat.h>
#include <sys/statvfs.h>

#include "aksl_internal.h"
**Style — include-what-you-use.** `stat`, `lstat`, `fstat`, `fstatat`, `statvfs` and `fstatvfs` are all declared in `<sys/stat.h>` / `<sys/statvfs.h>`, and this file gets both only transitively through `<akstdlib.h>`. `src/stream.c:19-27` re-includes `<stdio.h>`, `<stdlib.h>`, `<string.h>` and `<sys/types.h>` explicitly even though `akstdlib.h` already supplies every one of them. Same file, the grouping differs too: `stream.c` separates the project header, the system headers, and `"aksl_internal.h"` with blank lines. Here all four run together. **Fix:** ```c #include <akstdlib.h> #include <errno.h> #include <sys/stat.h> #include <sys/statvfs.h> #include "aksl_internal.h" ```
* return value that must be checked alongside a caller-owned POSIX struct. The
* wrappers put failure in the return value, where AKERR_NOIGNORE prevents it
andrew marked this conversation as resolved Outdated
andrew commented 2026-08-03 07:39:37 -04:00
Outdated
Review
Copy Link

@tachikoma none of these functions have documentation blocks. Follow the documentation style present in other files like stdlib.c; even though the headers contain doxygen blocks, the C source maintains additional information. Rephrase the exisitng documentation from the libc man page for this call.

@tachikoma none of these functions have documentation blocks. Follow the documentation style present in other files like stdlib.c; even though the headers contain doxygen blocks, the C source maintains additional information. Rephrase the exisitng documentation from the libc `man` page for this call.
👀 1
* from being silently dropped, while preserving the errno that distinguishes
* missing paths, inaccessible paths, and invalid descriptors. errno is cleared
* immediately before each libc call, so a broken libc that reports failure
* without setting errno is still an AKERR_IO failure rather than status 0.
*/
#include <akstdlib.h>
#include <errno.h>
#include <sys/stat.h>
#include <sys/statvfs.h>
#include "aksl_internal.h"
/*
@@ -41,8 +55,11 @@ akerr_ErrorContext AKERR_NOIGNORE *aksl_lstat(const char *pathname, struct stat
*/
akerr_ErrorContext AKERR_NOIGNORE *aksl_fstat(int fd, struct stat *dest)
{
PREPARE_ERROR(e); FAIL_ZERO_RETURN(e, dest, AKERR_NULLPOINTER, "fd=%d, dest=%p", fd, (void *)dest);
errno = 0; FAIL_NONZERO_RETURN(e, fstat(fd, dest), AKSL_ERRNO_OR(AKERR_IO), "fd=%d", fd); SUCCEED_RETURN(e);
PREPARE_ERROR(e);
FAIL_ZERO_RETURN(e, dest, AKERR_NULLPOINTER, "fd=%d, dest=%p", fd, (void *)dest);
errno = 0;
logikoma marked this conversation as resolved Outdated
tachikoma commented 2026-08-03 09:59:20 -04:00
Outdated
Review
Copy Link

Style nit — error message formatting diverges from its five siblings.

"dirfd=%d pathname=%s flags=%d" is space-separated. Every other error message in this file uses ", ":

  • line 15, 16: "pathname=%p, dest=%p"
  • line 30, 31: "pathname=%p, dest=%p"
  • line 44: "fd=%d, dest=%p"
  • line 57, 58: "dirfd=%d, pathname=%p, dest=%p" — the same function, two lines up, uses commas
  • line 72, 73: "path=%p, dest=%p"
  • line 86: "fd=%d, dest=%p"

Also flags=%d prints a bit mask in decimal. AT_SYMLINK_NOFOLLOW lands in a log as flags=256, which is a small unkindness to whoever is reading that log after the fact. flags=0x%x costs nothing.

Fix: "dirfd=%d, pathname=%s, flags=0x%x".

**Style nit — error message formatting diverges from its five siblings.** `"dirfd=%d pathname=%s flags=%d"` is space-separated. Every other error message in this file uses `", "`: - line 15, 16: `"pathname=%p, dest=%p"` - line 30, 31: `"pathname=%p, dest=%p"` - line 44: `"fd=%d, dest=%p"` - line 57, 58: `"dirfd=%d, pathname=%p, dest=%p"` — the *same function*, two lines up, uses commas - line 72, 73: `"path=%p, dest=%p"` - line 86: `"fd=%d, dest=%p"` Also `flags=%d` prints a bit mask in decimal. `AT_SYMLINK_NOFOLLOW` lands in a log as `flags=256`, which is a small unkindness to whoever is reading that log after the fact. `flags=0x%x` costs nothing. **Fix:** `"dirfd=%d, pathname=%s, flags=0x%x"`.
FAIL_NONZERO_RETURN(e, fstat(fd, dest), AKSL_ERRNO_OR(AKERR_IO), "fd=%d", fd);
SUCCEED_RETURN(e);
}
/*
@@ -57,7 +74,7 @@ akerr_ErrorContext AKERR_NOIGNORE *aksl_fstatat(int dirfd, const char *pathname,
FAIL_ZERO_RETURN(e, pathname, AKERR_NULLPOINTER, "dirfd=%d, pathname=%p, dest=%p", dirfd, (void *)pathname, (void *)dest);
FAIL_ZERO_RETURN(e, dest, AKERR_NULLPOINTER, "dirfd=%d, pathname=%p, dest=%p", dirfd, (void *)pathname, (void *)dest);
logikoma marked this conversation as resolved
tachikoma commented 2026-08-03 09:59:20 -04:00
Review
Copy Link

DEFECT (high) — aksl_statvfs's failure branch is never executed.

Third of the three. gcov, full suite: taken 0%, FAIL body never executed. tests/test_stat.c:69 is the only non-NULL call and it is the success path.

Fix, at tests/test_stat.c:70:

AKSL_CHECK_STATUS(aksl_statvfs("/nonexistent/aksl/stat", &vdest), ENOENT);
**DEFECT (high) — `aksl_statvfs`'s failure branch is never executed.** Third of the three. gcov, full suite: `taken 0%`, `FAIL` body `never executed`. `tests/test_stat.c:69` is the only non-NULL call and it is the success path. **Fix, at `tests/test_stat.c:70`:** ```c AKSL_CHECK_STATUS(aksl_statvfs("/nonexistent/aksl/stat", &vdest), ENOENT); ```
errno = 0;
FAIL_NONZERO_RETURN(e, fstatat(dirfd, pathname, dest, flags), AKSL_ERRNO_OR(AKERR_IO), "dirfd=%d pathname=%s flags=%d", dirfd, pathname, flags);
FAIL_NONZERO_RETURN(e, fstatat(dirfd, pathname, dest, flags), AKSL_ERRNO_OR(AKERR_IO), "dirfd=%d, pathname=%s, flags=0x%x", dirfd, pathname, flags);
SUCCEED_RETURN(e);
}
@@ -83,6 +100,9 @@ akerr_ErrorContext AKERR_NOIGNORE *aksl_statvfs(const char *path, struct statvfs
*/
akerr_ErrorContext AKERR_NOIGNORE *aksl_fstatvfs(int fd, struct statvfs *dest)
{
PREPARE_ERROR(e); FAIL_ZERO_RETURN(e, dest, AKERR_NULLPOINTER, "fd=%d, dest=%p", fd, (void *)dest);
errno = 0; FAIL_NONZERO_RETURN(e, fstatvfs(fd, dest), AKSL_ERRNO_OR(AKERR_IO), "fd=%d", fd); SUCCEED_RETURN(e);
PREPARE_ERROR(e);
FAIL_ZERO_RETURN(e, dest, AKERR_NULLPOINTER, "fd=%d, dest=%p", fd, (void *)dest);
errno = 0;
FAIL_NONZERO_RETURN(e, fstatvfs(fd, dest), AKSL_ERRNO_OR(AKERR_IO), "fd=%d", fd);
SUCCEED_RETURN(e);
}

37
tests/test_stat.c
View File

@@ -2,6 +2,14 @@
#include "aksl_capture.h"
#include <errno.h>
#include <fcntl.h>
#include <string.h>
/*
* A bit libc has never assigned in the AT_ space. fstatat(2) must reject it
* with EINVAL rather than ignoring it, which is what proves flags reach libc
* unmasked. Re-check this constant if AT_ ever grows into 0x40000000.
*/
#define AKSL_TEST_AT_INVALID 0x40000000
andrew marked this conversation as resolved Outdated
andrew commented 2026-08-03 07:37:17 -04:00
Outdated
Review
Copy Link

@tachikoma these tests need some descriptive comments explaining the actual conditions they are testing.

@tachikoma these tests need some descriptive comments explaining the actual conditions they are testing.
👀 1
static int test_stat_success_and_fstat(void)
{
@@ -30,6 +38,7 @@ static int test_stat_success_and_fstat(void)
/* A descriptor that was just closed must surface libc's EBADF. */
AKSL_CHECK_STATUS(aksl_fstat(fd, &fd_dest), EBADF);
AKSL_CHECK_STATUS(aksl_fstatvfs(fd, &vfs_dest), EBADF);
AKSL_CHECK(unlink(path) == 0);
return 0;
}
@@ -38,15 +47,21 @@ static int test_stat_paths_and_fstatat(void)
{
char path[AKSL_TMP_MAX];
char child[AKSL_TMP_MAX * 2];
char directory[AKSL_TMP_MAX];
char linkpath[AKSL_TMP_MAX * 2];
char *basename = NULL;
int dirfd = -1;
struct stat dest;
struct stat ldest;
struct stat path_dest;
struct statvfs vdest;
/* Missing paths and a regular file used as a directory preserve errno. */
AKSL_CHECK_STATUS(aksl_stat("/nonexistent/aksl/stat", &dest), ENOENT);
AKSL_CHECK_STATUS(aksl_lstat("/nonexistent/aksl/stat", &ldest), ENOENT);
/* The temporary path is a regular file, so adding a child tests ENOTDIR. */
logikoma marked this conversation as resolved Outdated
tachikoma commented 2026-08-03 09:59:20 -04:00
Outdated
Review
Copy Link

DEFECT (medium) — the two things that make fstatat worth wrapping are both untested.

Across the whole file, every aksl_fstatat call passes AT_FDCWD — lines 61, 65, 89, 90. No test ever opens a directory and passes a real directory descriptor. And no test passes a valid non-zero flag: line 61 passes 0, line 65 passes 0x40000000 which is deliberately invalid.

src/stat.c:49-52 states the contract in its own words:

pathname is resolved relative to dirfd unless it is absolute, and flags retain the libc choices such as inspecting a link itself.

Neither clause is under test. Concretely: a aksl_fstatat that ignored dirfd and passed AT_FDCWD unconditionally would pass this suite. So would one that passed 0 for flags regardless of what the caller asked for — which would silently turn every AT_SYMLINK_NOFOLLOW caller into a symlink-following one, and that is a security-relevant behaviour to get wrong quietly.

The file already builds a symlink at line 52, so the second case is nearly free.

Fix:

    /* A real dirfd must resolve a relative name against that directory. */
    dirfd = open("/tmp", O_RDONLY | O_DIRECTORY);
    AKSL_CHECK(dirfd >= 0);
    AKSL_CHECK_OK(aksl_fstatat(dirfd, strrchr(path, '/') + 1, &dest, 0));
    AKSL_CHECK(dest.st_ino == ldest.st_ino);
    AKSL_CHECK(close(dirfd) == 0);

    /* AT_SYMLINK_NOFOLLOW must reach libc unchanged, making this an lstat. */
    AKSL_CHECK_OK(aksl_fstatat(AT_FDCWD, linkpath, &dest, AT_SYMLINK_NOFOLLOW));
    AKSL_CHECK(S_ISLNK(dest.st_mode));

(path comes from aksl_temp_file, which builds it under $TMPDIR or /tmp; pull the directory from the same source rather than hardcoding /tmp.)

**DEFECT (medium) — the two things that make `fstatat` worth wrapping are both untested.** Across the whole file, every `aksl_fstatat` call passes `AT_FDCWD` — lines 61, 65, 89, 90. No test ever opens a directory and passes a real directory descriptor. And no test passes a *valid non-zero* flag: line 61 passes 0, line 65 passes `0x40000000` which is deliberately invalid. `src/stat.c:49-52` states the contract in its own words: > pathname is resolved relative to dirfd unless it is absolute, and flags retain the libc choices such as inspecting a link itself. Neither clause is under test. Concretely: a `aksl_fstatat` that ignored `dirfd` and passed `AT_FDCWD` unconditionally would pass this suite. So would one that passed `0` for `flags` regardless of what the caller asked for — which would silently turn every `AT_SYMLINK_NOFOLLOW` caller into a symlink-following one, and that is a security-relevant behaviour to get wrong quietly. The file already builds a symlink at line 52, so the second case is nearly free. **Fix:** ```c /* A real dirfd must resolve a relative name against that directory. */ dirfd = open("/tmp", O_RDONLY | O_DIRECTORY); AKSL_CHECK(dirfd >= 0); AKSL_CHECK_OK(aksl_fstatat(dirfd, strrchr(path, '/') + 1, &dest, 0)); AKSL_CHECK(dest.st_ino == ldest.st_ino); AKSL_CHECK(close(dirfd) == 0); /* AT_SYMLINK_NOFOLLOW must reach libc unchanged, making this an lstat. */ AKSL_CHECK_OK(aksl_fstatat(AT_FDCWD, linkpath, &dest, AT_SYMLINK_NOFOLLOW)); AKSL_CHECK(S_ISLNK(dest.st_mode)); ``` (`path` comes from `aksl_temp_file`, which builds it under `$TMPDIR` or `/tmp`; pull the directory from the same source rather than hardcoding `/tmp`.)
AKSL_CHECK(aksl_temp_file(path, sizeof(path)) == 0);
AKSL_CHECK(snprintf(child, sizeof(child), "%s/child", path) < (int)sizeof(child));
AKSL_CHECK_OK(aksl_stat(path, &path_dest));
AKSL_CHECK_STATUS(aksl_stat(child, &dest), ENOTDIR);
logikoma marked this conversation as resolved
tachikoma commented 2026-08-03 09:59:21 -04:00
Review
Copy Link

Nit — magic number.

0x40000000 is "a flag bit libc will reject", and nothing in the file says so. It is currently unallocated on Linux, but the AT_ space is still being handed out — AT_RECURSIVE (0x8000), the AT_STATX_* bits and AT_HANDLE_FID were all added over time. When 0x40000000 is eventually assigned, this test either starts failing for a reason nobody will connect to this line, or worse, keeps passing for a different reason.

Fix: name it and say why.

/*
 * A bit libc has never assigned in the AT_ space. fstatat(2) must reject it
 * with EINVAL rather than ignoring it, which is what proves flags reach libc
 * unmasked. Re-check this constant if AT_ ever grows into 0x40000000.
 */
#define AKSL_TEST_AT_INVALID 0x40000000
**Nit — magic number.** `0x40000000` is "a flag bit libc will reject", and nothing in the file says so. It is currently unallocated on Linux, but the `AT_` space is still being handed out — `AT_RECURSIVE` (0x8000), the `AT_STATX_*` bits and `AT_HANDLE_FID` were all added over time. When 0x40000000 is eventually assigned, this test either starts failing for a reason nobody will connect to this line, or worse, keeps passing for a different reason. **Fix:** name it and say why. ```c /* * A bit libc has never assigned in the AT_ space. fstatat(2) must reject it * with EINVAL rather than ignoring it, which is what proves flags reach libc * unmasked. Re-check this constant if AT_ ever grows into 0x40000000. */ #define AKSL_TEST_AT_INVALID 0x40000000 ```
AKSL_CHECK(snprintf(linkpath, sizeof(linkpath), "%s.link", path) < (int)sizeof(linkpath));
logikoma marked this conversation as resolved
tachikoma commented 2026-08-03 09:59:20 -04:00
Review
Copy Link

DEFECT (medium) — this asserts on errno after intervening library calls that are allowed to change it.

AKSL_CHECK_STATUS on line 65 does not just evaluate the expression. It calls aksl_take() (tests/aksl_capture.h), which runs four snprintf calls and akerr_release_error() before returning. Only then does line 66 read errno.

C permits a standard library function to set errno to a non-zero value even on a successful call. So this assertion is relying on glibc's snprintf happening not to touch errno for these particular inputs. It passes today. It is not guaranteed by anything, and it will fail on a libc where that is not true, with a failure message that points at fstatat rather than at snprintf.

Two further problems with it:

  1. It is redundant. Line 65 already proved the wrapper reported EINVAL. Line 66 adds no coverage of the wrapper.
  2. It asserts a contract the library does not offer. Nothing in include/akstdlib.h promises anything about errno after a wrapper returns — and it could not, because the NULL-argument paths (src/stat.c:15-16, 30-31, 44, 57-58, 72-73, 86) return before errno = 0 is ever reached, so on those paths errno is whatever stale value the caller already had. Codifying a post-call errno reading for one path out of two is how an accidental contract gets born.

Fix: delete line 66. The errno = E2BIG on line 64 is the valuable part — it proves the wrapper does not report a stale errno — and line 65 already checks it.

Worth stating the other half plainly, since it is the thing this test was reaching for: the errno handling in the library itself is correct. I traced it. FAIL expands to ENSURE_ERROR_READY(e); e->status = __err;, so AKSL_ERRNO_OR is evaluated after akerr_next_error() runs — and akerr_next_error() (deps/libakerror/src/error.c:240-248) is a bare loop over AKERR_ARRAY_ERROR with no library calls in it, so it cannot clobber errno in between. All six wrappers reset errno immediately before their libc call and read it before anything else can run. No defect there.

**DEFECT (medium) — this asserts on `errno` after intervening library calls that are allowed to change it.** `AKSL_CHECK_STATUS` on line 65 does not just evaluate the expression. It calls `aksl_take()` (`tests/aksl_capture.h`), which runs **four `snprintf` calls and `akerr_release_error()`** before returning. Only then does line 66 read `errno`. C permits a standard library function to set `errno` to a non-zero value even on a successful call. So this assertion is relying on glibc's `snprintf` happening not to touch `errno` for these particular inputs. It passes today. It is not guaranteed by anything, and it will fail on a libc where that is not true, with a failure message that points at `fstatat` rather than at `snprintf`. Two further problems with it: 1. **It is redundant.** Line 65 already proved the wrapper reported `EINVAL`. Line 66 adds no coverage of the wrapper. 2. **It asserts a contract the library does not offer.** Nothing in `include/akstdlib.h` promises anything about `errno` after a wrapper returns — and it could not, because the NULL-argument paths (`src/stat.c:15-16`, `30-31`, `44`, `57-58`, `72-73`, `86`) return *before* `errno = 0` is ever reached, so on those paths `errno` is whatever stale value the caller already had. Codifying a post-call `errno` reading for one path out of two is how an accidental contract gets born. **Fix:** delete line 66. The `errno = E2BIG` on line 64 is the valuable part — it proves the wrapper does not report a stale errno — and line 65 already checks it. Worth stating the other half plainly, since it is the thing this test was reaching for: **the errno handling in the library itself is correct.** I traced it. `FAIL` expands to `ENSURE_ERROR_READY(e); e->status = __err;`, so `AKSL_ERRNO_OR` is evaluated *after* `akerr_next_error()` runs — and `akerr_next_error()` (`deps/libakerror/src/error.c:240-248`) is a bare loop over `AKERR_ARRAY_ERROR` with no library calls in it, so it cannot clobber `errno` in between. All six wrappers reset `errno` immediately before their libc call and read it before anything else can run. No defect there.
AKSL_CHECK(symlink(path, linkpath) == 0);
@@ -57,13 +72,27 @@ static int test_stat_paths_and_fstatat(void)
AKSL_CHECK(S_ISREG(dest.st_mode));
AKSL_CHECK(S_ISLNK(ldest.st_mode));
/* AT_FDCWD makes fstatat resolve this path from the current directory. */
AKSL_CHECK_OK(aksl_fstatat(AT_FDCWD, path, &dest, 0));
/* A real dirfd must resolve a relative name against that directory. */
AKSL_CHECK(snprintf(directory, sizeof(directory), "%s", path) < (int)sizeof(directory));
basename = strrchr(directory, '/');
AKSL_CHECK(basename != NULL);
*basename = '\0';
dirfd = open(directory, O_RDONLY | O_DIRECTORY);
AKSL_CHECK(dirfd >= 0);
AKSL_CHECK_OK(aksl_fstatat(dirfd, strrchr(path, '/') + 1, &dest, 0));
AKSL_CHECK(dest.st_ino == path_dest.st_ino);
AKSL_CHECK(close(dirfd) == 0);
/* A valid non-zero flag must reach libc, making this call an lstat. */
AKSL_CHECK_OK(aksl_fstatat(AT_FDCWD, linkpath, &dest, AT_SYMLINK_NOFOLLOW));
AKSL_CHECK(S_ISLNK(dest.st_mode));
/* Invalid flags must replace stale errno with the EINVAL libc reports. */
errno = E2BIG;
AKSL_CHECK_STATUS(aksl_fstatat(AT_FDCWD, path, &dest, 0x40000000), EINVAL);
AKSL_CHECK(errno == EINVAL);
AKSL_CHECK_STATUS(aksl_fstatat(AT_FDCWD, path, &dest, AKSL_TEST_AT_INVALID), EINVAL);
/* statvfs reports ENOENT before it can describe a missing filesystem path. */
AKSL_CHECK_STATUS(aksl_statvfs("/nonexistent/aksl/stat", &vdest), ENOENT);
/* statvfs reports the filesystem containing the current directory. */
AKSL_CHECK_OK(aksl_statvfs(".", &vdest));