schfs: records at arbitrary offsets are packed, and the compiler is told

A leaf's value area holds variable-length records end to end, so a record begins
wherever the one before it finished -- a multiple of 8 only by luck. The code
reads them by casting a pointer into the block, which tells the compiler they
are aligned when they are not.

That works on x86, which is why it has never shown as a bug, and it is undefined
in C. A compiler entitled to assume 8-byte alignment may emit an instruction
that requires it, and the failure would arrive at a different optimisation level
or on a different architecture looking like filesystem corruption rather than
like codegen. UndefinedBehaviorSanitizer reported 31 of them across obj.c,
objw.c, attr.c and space.c.

Seven types now carry SCHFS_PACKED: dirent_rec, extent, extref, index_rec,
attr_rec, snap and inode. Two of those -- attr_rec and inode -- were never
reported, because schfs_objtest does not reach an unaligned instance of either.
They were latent rather than absent, and casting into a leaf is the same lie
whether or not a test has caught it yet.

NO ON-DISK LAYOUT CHANGED, and that was measured before the annotation was
applied rather than assumed: every one of these already carries explicit `pad`
fields and therefore has no implicit padding, so sizes and member offsets are
identical with and without it. Seven _Static_asserts now pin the sizes, because
a record that silently grows misreads every existing volume from that point on
and looks like corruption rather than like a struct that changed.

schfs_node_hdr, schfs_item, schfs_kptr and schfs_uberblock are deliberately left
alone. The node header is exactly 32 bytes, so the items and key pointers after
it are already 8-aligned, and annotating them would cost byte-wise access on the
tree's hottest paths to fix a problem they do not have.

Verified by reading an image written before the change: schfs_fsck --check
passes on structure, checksums, key ordering, data blocks and derived free
space, and the kernel mounts its own root unchanged.

Also here: mkschfs passed a null pointer to qsort for an empty directory. Every
implementation ignores it when the count is zero; the standard does not say they
have to.

make schfs-check now runs with no sanitizer output at all.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQ7Qx8E1q1M1bbCq8QLHS2

docs/architecture/schfs.md

@@ -244,6 +244,27 @@ current code refuses and says why.
 
 ---
 
+## Records are packed, and the compiler is told
+
+A leaf's value area holds variable-length records end to end, so a record starts
+wherever the previous one finished — 8-aligned only by luck. The seven types
+that live there carry `SCHFS_PACKED`, because casting a pointer into a block and
+calling it a `struct schfs_dirent_rec *` is otherwise a lie to the compiler: the
+type promises alignment the address does not have.
+
+**The annotation changes no on-disk layout.** Every one of those structs already
+carries explicit `pad` fields and so has no implicit padding; sizes and offsets
+are identical with and without it, and seven `_Static_assert`s hold them there.
+What changes is the generated code — byte-wise access where alignment cannot be
+proven, which is the truth of the situation.
+
+The block-level structures are deliberately excluded. `schfs_node_hdr` is
+exactly 32 bytes, so the items and key pointers after it are already aligned,
+and packing them would cost byte-wise access on the tree's hottest paths for
+nothing.
+
+---
+
 ## One source, two compilers
 
 Everything except `kmount.c` is compiled **both** into the kernel and into the

docs/guides/testing.md

@@ -121,20 +121,27 @@ linked into the kernel binary for a `ramroot` build.
 
 ---
 
-## One known failure left
+## The suite is green
 
-`make schfs-check` passes. UndefinedBehaviorSanitizer still reports **31
+`make schfs-check` passes with no sanitizer output at all. It was not, until
-misaligned reads** from casting pointers into tree leaves where records are not
+recently, and both failures are worth knowing about because neither was in the
-8-aligned — itemised in [../plan/1.0.md](../plan/1.0.md), and printed on stderr
+filesystem's logic:
-without failing the run.
 
-The leak half of this is closed, and how it closed is the useful part: the test
+**A leak with no API to fix it.** The test leaked 518 pages because the
-leaked 518 pages because the allocation map had no release path *at all*, which
+allocation map had no release path *at all* — which meant the kernel leaked the
-meant the kernel leaked the same map on every unmount. **A suite that leaks
+same map on every unmount. **A suite that leaks cannot detect a leak**: the
-cannot detect a leak** — the filesystem's own allocations would have arrived as
+filesystem's own allocations would have arrived as noise inside the harness's.
-noise inside the harness's. `schfs_objtest` now asserts that every page it
+`schfs_objtest` now asserts that every page it handed out came back.
-handed out came back, so the next one fails the suite rather than joining the
+
-background.
+**31 misaligned reads.** Records are read by casting a pointer into a tree leaf,
+where they sit at arbitrary offsets. That works on x86 and is undefined in C, so
+it had never shown as a bug and would have arrived as apparent filesystem
+corruption at some future optimisation level.
+
+The shared lesson is the one this directory keeps relearning: **a sanitizer that
+is reliably noisy is a sanitizer nobody reads.** Both were dismissible as "known
+and harmless" for as long as they were there, and one of them was a real kernel
+leak.
 
 ## What is not tested
 

docs/plan/1.0.md

@@ -164,25 +164,34 @@ known reasons stops being read, and then it is red for unknown reasons too.
       asserts the balance — 520 pages taken, 520 given back — so the next leak
       fails the suite instead of being absorbed by it.
 
-- [ ] **31 UndefinedBehaviorSanitizer errors**, all one root cause: records are
+- [x] ~~**31 UndefinedBehaviorSanitizer errors.**~~ Fixed, plus two the test
-      read straight out of a tree leaf by casting a pointer into the block, and
+      never reached. One root cause: a leaf's value area holds variable-length
-      a record's offset within a leaf is not a multiple of 8. Sites:
+      records packed end to end, so a record begins wherever the last one ended
-      `obj.c` (9), `objw.c` (11), `attr.c` (7), `space.c` (5), across
+      — a multiple of 8 only by luck — and the code read them by casting a
-      `schfs_dirent_rec`, `schfs_extent`, `schfs_extref`, `schfs_index_rec` and
+      pointer into the block, which tells the compiler they are aligned when
-      `schfs_snap`.
+      they are not.
 
-      **This is a real hazard, not sanitizer pedantry.** Unaligned access
+      The seven record types that live at arbitrary offsets now carry
-      happens to work on x86, which is why it has never shown as a bug — but it
+      `SCHFS_PACKED`. **No on-disk layout changed**, and that was measured
-      is undefined in C, and a compiler that knows a pointer is 8-aligned is
+      before it was applied rather than assumed: every one already had explicit
-      entitled to emit an instruction that requires it. The failure would appear
+      `pad` fields, so sizes and member offsets are identical with and without
-      under a different optimisation level or on a different architecture, and
+      the annotation. Seven `_Static_assert`s now pin those sizes, because a
-      would look like filesystem corruption rather than a codegen problem.
+      record that silently grows misreads every existing volume from that point
+      on and looks like corruption rather than like a struct that changed.
 
-      The fix that keeps the format unchanged is to stop casting: copy the bytes
+      `schfs_node_hdr`, `schfs_item`, `schfs_kptr` and `schfs_uberblock` are
-      into an aligned local with `memmove`, or declare the records packed so the
+      deliberately left alone — the node header is exactly 32 bytes, so
-      compiler stops assuming. Padding the records to 8 bytes would also work
+      everything after it is already 8-aligned, and annotating them would cost
-      and costs on-disk space — and is now a **format change**, which is what
+      byte-wise access on the tree's hottest paths to fix a problem they do not
-      the version policy above is for.
+      have.
+
+      Verified by reading an image written *before* the change: full
+      `schfs_fsck --check` passes on structure, checksums, key ordering, data
+      blocks and derived free space.
+
+      Also caught on the way: `mkschfs` passed a null pointer to `qsort` for an
+      empty directory. Every implementation ignores it when the count is zero;
+      the standard does not say they have to.
 
 **The identity item:**
 

include/schfs.h

@@ -13,6 +13,36 @@
 
 #include <stdint.h>
 
+// ---------------------------------------------------------------------------
+// Records that live at an arbitrary offset inside a block.
+//
+// A leaf's value area holds variable-length records packed end to end, so a
+// record begins wherever the one before it ended -- which is a multiple of 8
+// only by luck. The code reads them by casting a pointer into the block, and
+// that is a lie to the compiler unless it is told: the type says "8-byte
+// aligned" and the pointer is not.
+//
+// It works on x86, which is why it never showed as a bug, and it is undefined
+// in C. A compiler entitled to assume alignment may emit an instruction that
+// requires it, and the failure would arrive at a different optimisation level
+// or on a different architecture looking like filesystem corruption rather than
+// like codegen. UndefinedBehaviorSanitizer reported 31 of these across obj.c,
+// objw.c, attr.c and space.c.
+//
+// This annotation changes NO on-disk layout. Every struct it is applied to
+// already carries explicit `pad` fields and therefore has no implicit padding;
+// sizes and member offsets are identical with and without it, which was
+// measured rather than assumed before it was added. What it changes is the
+// code generated to read them: byte-wise where the compiler cannot prove
+// alignment, which is exactly the truth here.
+//
+// It is deliberately NOT applied to schfs_node_hdr, schfs_item, schfs_kptr or
+// schfs_uberblock. Those sit at fixed offsets in a block -- the header is 32
+// bytes, so everything after it is 8-aligned -- and annotating them would cost
+// byte-wise access on the hottest paths in the tree to fix a problem they do
+// not have.
+#define SCHFS_PACKED __attribute__((packed))
+
 #define SCHFS_MAGIC      0x53636846u   // "SchF"
 #define SCHFS_NODE_MAGIC 0x536e6f64u   // "Snod"
 
@@ -208,7 +238,7 @@ struct schfs_snap {
     uint32_t pad0;
     uint64_t created;           // generation the snapshot was taken at
     char     name[SCHFS_SNAP_NAME_MAX];
-};
+} SCHFS_PACKED;
 
 struct schfs_uberblock {
     uint32_t magic;             // SCHFS_MAGIC
@@ -283,7 +313,7 @@ struct schfs_inode {
     uint32_t flags;             // SCHFS_I_* — none defined yet
     uint32_t gen;               // transaction that last changed this inode;
                                 // what an incremental send/snapshot diff needs
-};
+} SCHFS_PACKED;
 
 // An extent maps a run of file blocks onto a run of disk blocks. refs is what
 // makes snapshots and reflink copies nearly free: sharing a range is a refcount
@@ -313,14 +343,14 @@ struct schfs_extent {
     // without consulting the tree at all.
     uint32_t refs;
     uint32_t pad;
-};
+} SCHFS_PACKED;
 
 // How many files point at a run of blocks. Present only for runs with more than
 // one reference; see SCHFS_EXTREF_OBJECTID.
 struct schfs_extref {
     uint64_t nblocks;
     uint64_t refs;
-};
+} SCHFS_PACKED;
 
 // Directory entry payload. The key's offset field holds the name hash, so a
 // lookup is one tree descent rather than a directory scan.
@@ -337,7 +367,7 @@ struct schfs_dirent_rec {
     uint16_t namelen;
     uint32_t pad;
     /* name bytes follow, namelen of them, not NUL-terminated */
-};
+} SCHFS_PACKED;
 
 #define SCHFS_NAME_MAX 255
 
@@ -364,7 +394,7 @@ struct schfs_attr_rec {
     uint16_t flags;
     uint32_t pad;
     /* name bytes, then value bytes; the pair padded to an 8-byte boundary */
-};
+} SCHFS_PACKED;
 
 // What a value *is*, so that a query can compare two of them meaningfully. A
 // string and an integer that happen to share bytes are not the same value, and
@@ -392,7 +422,7 @@ struct schfs_index_rec {
     uint32_t valuelen;
     uint32_t pad;
     /* value bytes follow, so a query can reject a hash collision */
-};
+} SCHFS_PACKED;
 
 #define SCHFS_INDEX_LEN(valuelen) \
     ((unsigned)((sizeof(struct schfs_index_rec) + (valuelen) + 7) & ~7u))
@@ -437,3 +467,14 @@ SCHFS_ASSERT_SIZE(schfs_attr_rec,    16);
 SCHFS_ASSERT_SIZE(schfs_index_rec,   16);
 
 #endif /* SCHFS_H */
+
+// The annotation above must not have moved anything. These are on-disk sizes:
+// if one changes, every existing volume is misread from that record onward, and
+// the failure looks like corruption rather than like a struct that grew.
+_Static_assert(sizeof(struct schfs_dirent_rec) == 16, "dirent record size changed");
+_Static_assert(sizeof(struct schfs_extent)     == 32, "extent record size changed");
+_Static_assert(sizeof(struct schfs_extref)     == 16, "extref record size changed");
+_Static_assert(sizeof(struct schfs_index_rec)  == 16, "index record size changed");
+_Static_assert(sizeof(struct schfs_attr_rec)   == 16, "attr record size changed");
+_Static_assert(sizeof(struct schfs_snap)       == 96, "snapshot record size changed");
+_Static_assert(sizeof(struct schfs_inode)      == 80, "inode record size changed");

tools/mkschfs.c

@@ -584,7 +584,11 @@ add_tree(uint64_t dirino, const char *hostdir)
     }
     closedir(d);
 
-    qsort(l.v, l.n, sizeof(*l.v), nl_cmp);
+    // qsort declares its first argument non-null, and an empty directory
+    // leaves l.v at NULL. Every implementation ignores the pointer when the
+    // count is zero; the standard does not say they have to.
+    if (l.n > 0)
+        qsort(l.v, l.n, sizeof(*l.v), nl_cmp);
     long subdirs = 0;
     for (int i = 0; i < l.n; i++) {
         subdirs += add_entry(dirino, hostdir, l.v[i]);