Skip to content

Commit 91e6357

Browse files
committed
build: fail when a component does not compile
The component recipe exits 0 whatever the compiler said, deliberately: one of 118 independent leaf targets that will not build must not stop the other 117. It removes the target instead, so a broken component becomes absent rather than stale and is never exercised as the last binary that happened to compile. That left the failure for something else to notice, and nothing did -- the orchestrator finds components by scanning the directory, so an absent one is a smaller set reported as success. Breaking a format string in one component leaves make, make test and make cross all green with the binary gone. check-components-built reads the three states the recipe already writes to disk: a non-empty file compiled, an empty file is the architecture-gated skip path writing a deliberate empty target, and an absent file is a compiler failure. 109 built, 9 gated, 118 sources. No expected count is used and none may be, a fixed number being the same rot one level up. -Werror=format promotes the mismatch itself, now that it can be seen. The cross build carries -Wformat already and reports none across seventeen arches, so the promotion costs nothing and closes the case where a wrong specifier prints a plausible address rather than failing. kasld_logf and progress_note gain format attributes, which carry the check to 503 call sites that a va_list had put beyond it. Every one was already correct; the attribute enforces what the code had. The remaining clang diagnostics were three kinds, and only one was a defect. bcm_msg_head_struct and mincore hold deliberate constructs -- a containing struct matching the wire format bcm_send_to_user() builds, and a byte-stepped scan whose unaligned reads are the technique -- and needed clang's spelling of suppressions already written for gcc. databounce's xabort_wrapper had lost its only caller and is removed. The 31 in api.h came from compiling the header as its own translation unit, where every static inline is unused by construction, so check-headers alone suppresses that.
1 parent 85d166f commit 91e6357

8 files changed

Lines changed: 140 additions & 18 deletions

File tree

Makefile

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ KASLD_WARN_FLAGS_WANTED := \
7171
-Werror=incompatible-pointer-types \
7272
-Werror=return-type \
7373
-Werror=format-security \
74+
-Werror=format \
7475
-Werror=frame-larger-than=2097152
7576
KASLD_HARDEN_FLAGS_WANTED := -fstack-protector-strong -D_FORTIFY_SOURCE=2
7677

@@ -311,10 +312,14 @@ $(OBJ_DIR):
311312
$(TEST_OBJ_DIR):
312313
@mkdir -p "$(TEST_OBJ_DIR)"
313314

314-
# Validate headers before building components
315+
# Validate headers before building components. -Wno-unused-function because the
316+
# point of this check is to compile the header as a translation unit of its own,
317+
# where every static inline it defines is unused by construction; the check stays
318+
# live for every real compile.
315319
.PHONY: check-headers
316320
check-headers: | $(COMP_DIR)
317-
$(Q)$(CC) $(ALL_CFLAGS) -xc -fsyntax-only $(SRC_DIR)/include/kasld/api.h
321+
$(Q)$(CC) $(ALL_CFLAGS) -Wno-unused-function -xc -fsyntax-only \
322+
$(SRC_DIR)/include/kasld/api.h
318323

319324
$(COMP_DIR)/%: $(COMP_SRC_DIR)/%.c $(HDRS) | $(COMP_DIR)
320325
$(call cc-component, $(CC) $(ALL_CFLAGS) $(ALL_LDFLAGS) -I$(SRC_DIR) $< -o $@)
@@ -779,6 +784,7 @@ lint :
779784
$(TEST_DIR)/check-component-output \
780785
$(TEST_DIR)/check-component-meta \
781786
$(TEST_DIR)/check-component-cap \
787+
$(TEST_DIR)/check-components-built \
782788
$(TEST_DIR)/check-log-prefixes \
783789
$(TEST_DIR)/check-live-probes \
784790
$(TEST_DIR)/check-hash-parity \

docs/testing.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,7 @@ stays plain, and setting `KASLD_COLOR` non-empty or empty forces either.
178178
| `check-absence-vs-denial` | no component reports a denied source as an absent one — a failed probe's reason is in `errno`, and UNAVAILABLE claims the target's build while NOPERM reports its hardening |
179179
| `check-component-output` | components write only wire lines to stdout (stdout is the machine channel; diagnostics go to stderr) |
180180
| `check-component-meta` | every component declares `KASLD_META` with a `method:` key |
181+
| `check-components-built` | every component source produced a build artefact. The component recipe exits 0 whatever the compiler said, deliberately: with 118 independent leaf targets, one that will not compile must not stop the other 117 being built and tested. It removes the target instead, so a broken component becomes absent rather than stale and is never silently exercised as the last binary that happened to compile. That left the failure for something else to notice, and nothing did — the orchestrator finds components by scanning the directory, so an absent one is simply a smaller set reported as success, and no guard compared the build against the source. A component could stop compiling with `make`, `make test` and `make cross` all staying green. The recipe already writes the distinction to disk, so this reads it rather than tracking its own: a non-empty file compiled, an empty file is the architecture-gated skip path writing a deliberate empty target, and an absent file is a compiler failure. No expected count is used, and none may be — a fixed number rots the moment a component is added, which is this same failure one level up |
181182
| `check-component-cap` | `MAX_COMPONENTS` keeps a margin above the in-tree component count — a component directory that overruns it silently drops the excess |
182183
| `check-log-prefixes` | no diagnostic message begins with a `[.]`/`[-]`/`[+]` marker (the `kasld_info`/`kasld_err`/`kasld_found` helper already prepends one — an embedded marker doubles it) |
183184
| `check-live-probes` | every live probe (reads live kernel/CPU state) is tagged `live:1` and self-guards with `kasld_skip_live_probe()`, so it never runs offline against the analysis host |

src/components/bcm_msg_head_struct.c

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,10 +68,19 @@ KASLD_META("method:heuristic\n"
6868
* member, so wrapping it in a containing struct (the kernel's own pattern
6969
* for sending bcm_msg_head + a fixed number of frames in one buffer) is
7070
* flagged under -Wpedantic. The layout matches the kernel's expected wire
71-
* format — relied on deliberately. Suppress the pedantic warning at
72-
* the two declaration sites rather than hide the kernel-side shape. */
71+
* format — relied on deliberately: bcm_send_to_user() allocates
72+
* sizeof(*head) + datalen and appends the frames straight after the head, so a
73+
* containing struct is what the socket actually reads. Suppress at the two
74+
* declaration sites rather than hide the kernel-side shape.
75+
*
76+
* Two spellings of one complaint: gcc reports it as -Wpedantic, clang as
77+
* -Wgnu-variable-sized-type-not-at-end. Naming both keeps the suppression as
78+
* narrow as the construct it covers, on either compiler. */
7379
#pragma GCC diagnostic push
7480
#pragma GCC diagnostic ignored "-Wpedantic"
81+
#if defined(__clang__)
82+
#pragma GCC diagnostic ignored "-Wgnu-variable-sized-type-not-at-end"
83+
#endif
7584

7685
static void rxsetup_sock(int sock) {
7786
struct sockaddr_can sa;

src/components/databounce.c

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -112,15 +112,6 @@ KASLD_META("method:timing\n"
112112
"discloses:virtual\n"
113113
"hardware:TSX required (mitigated by tsx=off)\n");
114114

115-
/* =========================================================================
116-
* TSX xabort
117-
* =========================================================================
118-
*/
119-
120-
static inline __attribute__((always_inline)) void xabort_wrapper(void) {
121-
__asm__ volatile(".byte 0xc6,0xf8,0x00" ::: "memory");
122-
}
123-
124115
/* =========================================================================
125116
* Data Bounce single sweep
126117
* =========================================================================

src/components/mincore.c

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,20 @@ static unsigned long get_kernel_addr_mincore(void) {
126126

127127
unsigned long n;
128128
/* Slide an unsigned-long-wide window over buf; stop before the read would
129-
* run past the page-sized allocation (buf[n .. n+sizeof(long)-1]). */
129+
* run past the page-sized allocation (buf[n .. n+sizeof(long)-1]).
130+
*
131+
* The window advances one BYTE at a time, so most reads are unaligned by
132+
* construction: the leaked pointer sits at an offset this side does not
133+
* know, and stepping by sizeof(long) would skip three candidate positions
134+
* in four. The cast therefore does increase the required alignment, which
135+
* is safe only because this file refuses to compile off x86-64 (see the
136+
* gate at the top), where an unaligned load is permitted. On a
137+
* strict-alignment target the same scan would have to be assembled
138+
* byte-wise instead. */
139+
#if defined(__clang__)
140+
#pragma GCC diagnostic push
141+
#pragma GCC diagnostic ignored "-Wcast-align"
142+
#endif
130143
for (n = 0; n + sizeof(unsigned long) <= page; n++) {
131144
addr = *(unsigned long *)(&buf[n]);
132145
/* Kernel address space */
@@ -137,6 +150,9 @@ static unsigned long get_kernel_addr_mincore(void) {
137150
return addr;
138151
}
139152
}
153+
#if defined(__clang__)
154+
#pragma GCC diagnostic pop
155+
#endif
140156
}
141157

142158
if (munmap((void *)0x66000000, len))

src/include/kasld/cli.h

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,14 @@ static inline int kasld_is_verbose(void) {
4444
}
4545

4646
/* Emit `[<level>] <msg>\n` to stderr. `gated` lines print only when verbose.
47-
* Prefer the wrappers below; the level alphabet is closed at '.', '-', '+'. */
48-
static inline void kasld_logf(char level, int gated, const char *fmt, ...) {
47+
* Prefer the wrappers below; the level alphabet is closed at '.', '-', '+'.
48+
*
49+
* The format attribute is what makes every CALLER checkable: the format reaches
50+
* vfprintf through a va_list, where a compiler can no longer relate it to the
51+
* arguments, so without this the check is lost at the one place a mismatch
52+
* would print a plausible wrong address rather than fail. */
53+
__attribute__((format(printf, 3, 4))) static inline void
54+
kasld_logf(char level, int gated, const char *fmt, ...) {
4955
if (gated && !kasld_is_verbose())
5056
return;
5157
va_list ap;

src/orchestrator.c

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -258,8 +258,11 @@ static void progress_paint(int done, int total, int inflight) {
258258
}
259259

260260
/* Emit a diagnostic on its own line without corrupting the bar. Safe from any
261-
* thread, and a plain stderr write when no bar is drawn. */
262-
static void progress_note(const char *fmt, ...) {
261+
* thread, and a plain stderr write when no bar is drawn. The format attribute
262+
* carries the check to the callers, which is the only place it can happen once
263+
* the format has entered a va_list. */
264+
__attribute__((format(printf, 1, 2))) static void progress_note(const char *fmt,
265+
...) {
263266
va_list ap;
264267
RESULT_LOCK();
265268
int inflight = progress_inflight;

tests/check-components-built

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
#!/bin/sh
2+
# This file is part of KASLD - https://github.com/bcoles/kasld
3+
#
4+
# check-components-built — every component source produced a build artefact.
5+
#
6+
# The component recipe exits 0 whatever the compiler said, on purpose: with 118
7+
# independent leaf targets, one that will not compile must not stop the other
8+
# 117 from being built and tested. What it does instead is REMOVE the target, so
9+
# a broken component becomes absent rather than stale and is not silently
10+
# exercised as the last binary that happened to compile.
11+
#
12+
# That leaves the failure to be noticed by something. Nothing noticed it. The
13+
# orchestrator finds components by scanning the directory, so an absent one is
14+
# simply a smaller set -- 117 components, reported as success -- and no guard
15+
# compared what was built against what exists in source. A component could stop
16+
# compiling and `make`, `make test` and `make cross` would all stay green.
17+
#
18+
# The recipe already writes the distinction to disk, so this reads it rather
19+
# than tracking anything of its own:
20+
#
21+
# non-empty file compiled
22+
# empty file arch-gated, skipped by the `#error "Architecture is not
23+
# supported"` path, which writes an empty target deliberately
24+
# absent the compiler failed
25+
#
26+
# A count is not used and must not be: a fixed expected number rots the moment a
27+
# component is added, which is the failure mode this file exists to prevent, one
28+
# level up. The source directory is the inventory and the build answers to it.
29+
#
30+
# Usage: tests/check-components-built (also run by `make test`)
31+
# ---
32+
# <bcoles@gmail.com>
33+
34+
set -u
35+
ROOT=$(CDPATH='' cd -- "$(dirname -- "$0")/.." && pwd)
36+
# shellcheck source=tests/lib/guard-scope.sh
37+
. "$ROOT/tests/lib/guard-scope.sh"
38+
39+
if [ -t 2 ] || [ -n "${KASLD_COLOR:-}" ]; then
40+
RED=$(printf '\033[31m'); GREEN=$(printf '\033[32m')
41+
YELLOW=$(printf '\033[33m'); RESET=$(printf '\033[0m')
42+
else
43+
RED=; GREEN=; YELLOW=; RESET=
44+
fi
45+
46+
# The native triple, resolved the same way the build resolves it. Probing
47+
# build/*/ and taking the first that exists would pick a cross directory --
48+
# i686 sorts before x86_64 -- and check a set this host never built.
49+
COMP=
50+
for t in "$(cc -dumpmachine 2>/dev/null)" "$(${CC:-cc} -dumpmachine 2>/dev/null)"; do
51+
[ -n "$t" ] || continue
52+
if [ -d "$ROOT/build/$t/components" ]; then
53+
COMP=$ROOT/build/$t/components
54+
break
55+
fi
56+
done
57+
[ -n "$COMP" ] || {
58+
printf '%scheck-components-built: SKIP%s (no native component build; run make first)\n' \
59+
"$YELLOW" "$RESET"
60+
exit 0
61+
}
62+
63+
missing=; nsrc=0; nbuilt=0; nskipped=0
64+
for src in "$ROOT"/src/components/*.c; do
65+
[ -f "$src" ] || continue
66+
nsrc=$((nsrc + 1))
67+
name=$(basename "$src" .c)
68+
if [ ! -e "$COMP/$name" ]; then
69+
missing="$missing $name"
70+
elif [ -s "$COMP/$name" ]; then
71+
nbuilt=$((nbuilt + 1))
72+
else
73+
nskipped=$((nskipped + 1))
74+
fi
75+
done
76+
77+
if [ -n "$missing" ]; then
78+
printf '%scheck-components-built: FAIL%s (component source with no build artefact)\n' \
79+
"$RED" "$RESET" >&2
80+
for m in $missing; do printf ' %s\n' "$m" >&2; done
81+
printf 'The compile failed and the recipe removed the target. Re-run make and\n' >&2
82+
printf 'read the CC line for that component; the diagnostic is printed there even\n' >&2
83+
printf 'though the build reports success.\n' >&2
84+
exit 1
85+
fi
86+
87+
guard_scope "check-components-built" "$nsrc" 50
88+
89+
printf '%scheck-components-built: OK%s (%s built, %s architecture-gated, %s sources)\n' \
90+
"$GREEN" "$RESET" "$nbuilt" "$nskipped" "$nsrc"

0 commit comments

Comments
 (0)