Wrap file metadata calls #31
37
src/stat.c
@@ -3,6 +3,12 @@
|
|||||||||||||||||||||||||
#include <errno.h>
|
|||||||||||||||||||||||||
#include "aksl_internal.h"
|
|||||||||||||||||||||||||
|
logikoma marked this conversation as resolved
Outdated
|
|||||||||||||||||||||||||
|
|||||||||||||||||||||||||
/*
|
|||||||||||||||||||||||||
|
andrew marked this conversation as resolved
Outdated
andrew
commented
@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 @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.
|
|||||||||||||||||||||||||
* stat(2) follows pathname through any symbolic links and writes the target's
|
|||||||||||||||||||||||||
* metadata into the caller-owned struct stat. A file that is gone, cannot be
|
|||||||||||||||||||||||||
* searched, or lives below a non-directory component is reported as the errno
|
|||||||||||||||||||||||||
* from libc rather than as a library-specific status.
|
|||||||||||||||||||||||||
*/
|
|||||||||||||||||||||||||
akerr_ErrorContext AKERR_NOIGNORE *aksl_stat(const char *pathname, struct stat *dest)
|
|||||||||||||||||||||||||
{
|
|||||||||||||||||||||||||
PREPARE_ERROR(e);
|
|||||||||||||||||||||||||
@@ -12,6 +18,12 @@ akerr_ErrorContext AKERR_NOIGNORE *aksl_stat(const char *pathname, struct stat *
|
|||||||||||||||||||||||||
FAIL_NONZERO_RETURN(e, stat(pathname, dest), AKSL_ERRNO_OR(AKERR_IO), "pathname=%s", pathname);
|
|||||||||||||||||||||||||
SUCCEED_RETURN(e);
|
|||||||||||||||||||||||||
}
|
|||||||||||||||||||||||||
|
|||||||||||||||||||||||||
/*
|
|||||||||||||||||||||||||
* lstat(2) is stat(2) without the final symbolic-link traversal. That is the
|
|||||||||||||||||||||||||
* difference a caller needs when it is deciding whether a path is a link or
|
|||||||||||||||||||||||||
* when the link's ownership and mode are the metadata of interest.
|
|||||||||||||||||||||||||
*/
|
|||||||||||||||||||||||||
akerr_ErrorContext AKERR_NOIGNORE *aksl_lstat(const char *pathname, struct stat *dest)
|
|||||||||||||||||||||||||
|
andrew marked this conversation as resolved
andrew
commented
Missing newline Missing newline
|
|||||||||||||||||||||||||
{
|
|||||||||||||||||||||||||
PREPARE_ERROR(e);
|
|||||||||||||||||||||||||
@@ -21,11 +33,24 @@ akerr_ErrorContext AKERR_NOIGNORE *aksl_lstat(const char *pathname, struct stat
|
|||||||||||||||||||||||||
FAIL_NONZERO_RETURN(e, lstat(pathname, dest), AKSL_ERRNO_OR(AKERR_IO), "pathname=%s", pathname);
|
|||||||||||||||||||||||||
|
logikoma marked this conversation as resolved
tachikoma
commented
DEFECT (high) — this failure branch is never executed by any test in the suite. Measured, not guessed.
Every branch inside the Line coverage hides this completely — it reads The functional consequence: Fix (one line, next to the existing **DEFECT (high) — this failure branch is never executed by any test in the suite.**
Measured, not guessed. `-DAKSL_COVERAGE=ON`, full 20-test `ctest` run, `gcov -b` on `src/stat.c`:
| line | function | libc-failure branch |
|---|---|---|
| 18 | `aksl_stat` | `taken 50%` |
| **33** | **`aksl_lstat`** | **`taken 0%`** |
| 45 | `aksl_fstat` | `taken 50%` |
| 60 | `aksl_fstatat` | `taken 50%` |
| **75** | **`aksl_statvfs`** | **`taken 0%`** |
| **87** | **`aksl_fstatvfs`** | **`taken 0%`** |
Every branch inside the `FAIL` body on this line reports `never executed`.
Line coverage hides this completely — it reads `Lines executed:100.00% of 34`, because the *line* is reached by the success path. Branch coverage is `Taken at least once: 46.77% of 248`.
The functional consequence: `AKSL_ERRNO_OR(AKERR_IO)` on this line is dead code as far as the suite is concerned. That macro is the one `src/aksl_internal.h` documents as the fix for a FAIL "whose status is 0, which every downstream DETECT and CATCH reads as success while the context still holds a pool slot: an error that is invisible and leaks at the same time." Half the wrappers in this PR ship that mechanism with it never having run once. Drop the `errno = 0` on line 32, or write `AKERR_IO` bare instead of the macro, and the suite stays green.
**Fix (one line, next to the existing `aksl_stat` ENOENT check at `tests/test_stat.c:46`):**
```c
AKSL_CHECK_STATUS(aksl_lstat("/nonexistent/aksl/stat", &ldest), ENOENT);
```
|
|||||||||||||||||||||||||
SUCCEED_RETURN(e);
|
|||||||||||||||||||||||||
}
|
|||||||||||||||||||||||||
|
|||||||||||||||||||||||||
/*
|
|||||||||||||||||||||||||
* fstat(2) gets metadata from an already-open descriptor, so it has no path
|
|||||||||||||||||||||||||
* lookup race and remains useful after the file has been renamed or unlinked.
|
|||||||||||||||||||||||||
* A closed or otherwise invalid descriptor reports EBADF from libc.
|
|||||||||||||||||||||||||
*/
|
|||||||||||||||||||||||||
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);
|
|||||||||||||||||||||||||
|
logikoma marked this conversation as resolved
tachikoma
commented
Style — multiple statements per line, and it is not only a style question here. Lines 44-45 put five statements on two lines. Worth separating the cosmetic part from the real part. The code as written is correct — I checked the expansion. But collapsing unbraced control-flow macros onto a shared line is the exact shape that makes the next edit here go wrong silently, and the one-statement-per-line rule exists to stop it. This is also the only place in the file where the visual structure does not match the other five wrappers, which is what makes a reviewer stop. Fix: **Style — multiple statements per line, and it is not only a style question here.**
Lines 44-45 put five statements on two lines. `src/stat.c:86-87` in `aksl_fstatvfs` does the same. The other four wrappers in this file, and every wrapper in `src/stream.c`, put one statement per line.
Worth separating the cosmetic part from the real part. `PREPARE_ERROR`, `FAIL_ZERO_RETURN`, `FAIL_NONZERO_RETURN` and `SUCCEED_RETURN` are all **multi-statement macros with no wrapping braces** — `FAIL_ZERO_RETURN` expands to a bare `if ( __x == 0 ) { FAIL(...); return __err_context; }` and `SUCCEED_RETURN` expands to `RELEASE_ERROR(e); return NULL;`, which is itself two statements including an `if`. `PREPARE_ERROR` expands to a function call *plus a declaration*.
The code as written is correct — I checked the expansion. But collapsing unbraced control-flow macros onto a shared line is the exact shape that makes the next edit here go wrong silently, and the one-statement-per-line rule exists to stop it. This is also the only place in the file where the visual structure does not match the other five wrappers, which is what makes a reviewer stop.
**Fix:**
```c
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);
```
|
|||||||||||||||||||||||||
}
|
|||||||||||||||||||||||||
|
|||||||||||||||||||||||||
/*
|
|||||||||||||||||||||||||
* fstatat(2) is the directory-descriptor form of stat(2). pathname is resolved
|
|||||||||||||||||||||||||
* relative to dirfd unless it is absolute, and flags retain the libc choices
|
|||||||||||||||||||||||||
|
andrew marked this conversation as resolved
Outdated
andrew
commented
Missing newline Missing newline
|
|||||||||||||||||||||||||
* such as inspecting a link itself. Keeping those flags unchanged prevents this
|
|||||||||||||||||||||||||
* wrapper from inventing a smaller policy than the POSIX call already exposes.
|
|||||||||||||||||||||||||
*/
|
|||||||||||||||||||||||||
akerr_ErrorContext AKERR_NOIGNORE *aksl_fstatat(int dirfd, const char *pathname, struct stat *dest, int flags)
|
|||||||||||||||||||||||||
{
|
|||||||||||||||||||||||||
PREPARE_ERROR(e);
|
|||||||||||||||||||||||||
@@ -35,6 +60,12 @@ akerr_ErrorContext AKERR_NOIGNORE *aksl_fstatat(int dirfd, const char *pathname,
|
|||||||||||||||||||||||||
FAIL_NONZERO_RETURN(e, fstatat(dirfd, pathname, dest, flags), AKSL_ERRNO_OR(AKERR_IO), "dirfd=%d pathname=%s flags=%d", dirfd, pathname, flags);
|
|||||||||||||||||||||||||
|
logikoma marked this conversation as resolved
Outdated
tachikoma
commented
Style nit — error message formatting diverges from its five siblings.
Also Fix: **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"`.
|
|||||||||||||||||||||||||
SUCCEED_RETURN(e);
|
|||||||||||||||||||||||||
}
|
|||||||||||||||||||||||||
|
|||||||||||||||||||||||||
/*
|
|||||||||||||||||||||||||
* statvfs(3) writes information about the mounted filesystem containing path,
|
|||||||||||||||||||||||||
* not merely the named file. The result includes the filesystem block sizes and
|
|||||||||||||||||||||||||
* available space the caller needs before it decides whether an operation fits.
|
|||||||||||||||||||||||||
*/
|
|||||||||||||||||||||||||
akerr_ErrorContext AKERR_NOIGNORE *aksl_statvfs(const char *path, struct statvfs *dest)
|
|||||||||||||||||||||||||
{
|
|||||||||||||||||||||||||
PREPARE_ERROR(e);
|
|||||||||||||||||||||||||
@@ -44,6 +75,12 @@ akerr_ErrorContext AKERR_NOIGNORE *aksl_statvfs(const char *path, struct statvfs
|
|||||||||||||||||||||||||
FAIL_NONZERO_RETURN(e, statvfs(path, dest), AKSL_ERRNO_OR(AKERR_IO), "path=%s", path);
|
|||||||||||||||||||||||||
|
logikoma marked this conversation as resolved
tachikoma
commented
DEFECT (high) — Third of the three. gcov, full suite: Fix, at **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);
```
|
|||||||||||||||||||||||||
SUCCEED_RETURN(e);
|
|||||||||||||||||||||||||
}
|
|||||||||||||||||||||||||
|
|||||||||||||||||||||||||
/*
|
|||||||||||||||||||||||||
* fstatvfs(3) is the descriptor form of statvfs(3). It asks the filesystem that
|
|||||||||||||||||||||||||
* owns fd for the same capacity and flag information without resolving a path
|
|||||||||||||||||||||||||
* again, and reports a bad descriptor through the errno libc supplies.
|
|||||||||||||||||||||||||
*/
|
|||||||||||||||||||||||||
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);
|
|||||||||||||||||||||||||
|
|||||||||||||||||||||||||
@@ -10,6 +10,7 @@ static int test_stat_success_and_fstat(void)
|
||||
struct stat path_dest;
|
||||
struct stat fd_dest;
|
||||
struct statvfs vfs_dest;
|
||||
/* A real file makes the mode and four-byte size observable to stat(2). */
|
||||
|
andrew marked this conversation as resolved
Outdated
andrew
commented
@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.
|
||||
AKSL_CHECK(aksl_temp_file(path, sizeof(path)) == 0);
|
||||
fd = open(path, O_RDWR);
|
||||
AKSL_CHECK(fd >= 0);
|
||||
@@ -17,11 +18,17 @@ static int test_stat_success_and_fstat(void)
|
||||
AKSL_CHECK_OK(aksl_stat(path, &path_dest));
|
||||
AKSL_CHECK(S_ISREG(path_dest.st_mode));
|
||||
AKSL_CHECK(path_dest.st_size == 4);
|
||||
|
||||
/* fstat(2) addresses the same opened inode, not a second pathname lookup. */
|
||||
AKSL_CHECK_OK(aksl_fstat(fd, &fd_dest));
|
||||
AKSL_CHECK(fd_dest.st_ino == path_dest.st_ino);
|
||||
|
||||
/* fstatvfs(3) describes the backing filesystem and has a usable block size. */
|
||||
AKSL_CHECK_OK(aksl_fstatvfs(fd, &vfs_dest));
|
||||
AKSL_CHECK(vfs_dest.f_frsize != 0);
|
||||
AKSL_CHECK(close(fd) == 0);
|
||||
|
||||
/* A descriptor that was just closed must surface libc's EBADF. */
|
||||
AKSL_CHECK_STATUS(aksl_fstat(fd, &fd_dest), EBADF);
|
||||
AKSL_CHECK(unlink(path) == 0);
|
||||
return 0;
|
||||
@@ -35,20 +42,30 @@ static int test_stat_paths_and_fstatat(void)
|
||||
struct stat dest;
|
||||
struct stat ldest;
|
||||
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);
|
||||
/* The temporary path is a regular file, so adding a child tests ENOTDIR. */
|
||||
AKSL_CHECK(aksl_temp_file(path, sizeof(path)) == 0);
|
||||
AKSL_CHECK(snprintf(child, sizeof(child), "%s/child", path) < (int)sizeof(child));
|
||||
AKSL_CHECK_STATUS(aksl_stat(child, &dest), ENOTDIR);
|
||||
AKSL_CHECK(snprintf(linkpath, sizeof(linkpath), "%s.link", path) < (int)sizeof(linkpath));
|
||||
AKSL_CHECK(symlink(path, linkpath) == 0);
|
||||
|
||||
/* stat follows the link while lstat reports the link object itself. */
|
||||
AKSL_CHECK_OK(aksl_stat(linkpath, &dest));
|
||||
AKSL_CHECK_OK(aksl_lstat(linkpath, &ldest));
|
||||
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));
|
||||
|
logikoma marked this conversation as resolved
Outdated
tachikoma
commented
DEFECT (medium) — the two things that make Across the whole file, every
Neither clause is under test. Concretely: a The file already builds a symlink at line 52, so the second case is nearly free. Fix: ( **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`.)
|
||||
|
||||
/* Invalid flags must replace stale errno with the EINVAL libc reports. */
|
||||
errno = E2BIG;
|
||||
AKSL_CHECK_STATUS(aksl_fstatat(AT_FDCWD, path, &dest, 0x40000000), EINVAL);
|
||||
|
logikoma marked this conversation as resolved
tachikoma
commented
Nit — magic number.
Fix: name it and say why. **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(errno == EINVAL);
|
||||
|
logikoma marked this conversation as resolved
tachikoma
commented
DEFECT (medium) — this asserts on
C permits a standard library function to set Two further problems with it:
Fix: delete line 66. The 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. **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.
|
||||
|
||||
/* statvfs reports the filesystem containing the current directory. */
|
||||
AKSL_CHECK_OK(aksl_statvfs(".", &vdest));
|
||||
AKSL_CHECK(vdest.f_frsize != 0);
|
||||
AKSL_CHECK(unlink(linkpath) == 0);
|
||||
@@ -61,7 +78,9 @@ static int test_stat_null_arguments(void)
|
||||
char path[AKSL_TMP_MAX];
|
||||
struct stat dest;
|
||||
struct statvfs vdest;
|
||||
/* Create a valid path so each failure below isolates a NULL argument. */
|
||||
AKSL_CHECK(aksl_temp_file(path, sizeof(path)) == 0);
|
||||
/* Every wrapper refuses either missing caller-owned input or output storage. */
|
||||
AKSL_CHECK_STATUS(aksl_stat(NULL, &dest), AKERR_NULLPOINTER);
|
||||
AKSL_CHECK_STATUS(aksl_stat(path, NULL), AKERR_NULLPOINTER);
|
||||
AKSL_CHECK_STATUS(aksl_lstat(NULL, &dest), AKERR_NULLPOINTER);
|
||||
|
||||
Style — include-what-you-use.
stat,lstat,fstat,fstatat,statvfsandfstatvfsare all declared in<sys/stat.h>/<sys/statvfs.h>, and this file gets both only transitively through<akstdlib.h>.src/stream.c:19-27re-includes<stdio.h>,<stdlib.h>,<string.h>and<sys/types.h>explicitly even thoughakstdlib.halready supplies every one of them.Same file, the grouping differs too:
stream.cseparates the project header, the system headers, and"aksl_internal.h"with blank lines. Here all four run together.Fix: