Skip to content

Commit 2585b3d

Browse files
committed
fix(runtime): the Discussion-#12 misread class dies tree-wide, gated
Smell-audited: 1: the four fixes remove the representation coupling (forEachEntry) rather than adding .hash_map arms — same shape as the contributor's analyzer fix; the 19 remaining by-design decode sites carry per-line repr-decode-ok waivers instead of a file allowlist so each exception stays visible in review. Discussion #12's segfault (valueSetToForm) was one face of a class: code outside src/runtime/collection/ decoding a collection's backing representation unconditionally, wrong past the 8-entry ArrayMap→HashMap promotion. A tree-wide audit found three more LIVE instances and one silent drop, all fixed here via map.forEachEntry: - multimethod.zig: the isa?/derive candidate walk was array_map-only, so hierarchy dispatch silently vanished at the 9th defmethod while exact-match kept working. clj-oracle-verified corpus line added (test/diff/clj_corpus/multimethod.txt). - cljw/http/client.zig: a valid 9-entry :headers map was rejected as "not a map of string→string". - cljw/http/server.zig: >8 response headers were dropped wholesale (the 16-header cap + SE-5 CRLF check survive inside the callback). - app/error_render.zig: a >8-entry *error-context* map was omitted from the EDN event — the site's own comment claimed the opposite. scripts/check_repr_decode.sh (new; smoke core + full gate, BASELINE=0) forbids the class: any decodePtr of a collection representation struct outside src/runtime/collection/ fails unless the line carries a `repr-decode-ok:` waiver. Waivers landed at the 19 audited-safe sites (printers' dual-path arms, sorted comparator field reads, the representation-dependent serializer, walk.zig's dispatch-guaranteed arm). A warn-only secondary pattern flags lone `.array_map` tag tests. Endgame (stop exporting the repr structs; compiler-enforced) = D-576. e2e: http server/client grow 9-header cases (/h9, /many-headers); error_format grows the 9-entry context case. All boundary-locked at 8→9.
1 parent 8f8bd43 commit 2585b3d

17 files changed

Lines changed: 243 additions & 77 deletions

‎scripts/check_repr_decode.sh‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
#!/usr/bin/env bash
2+
# scripts/check_repr_decode.sh — collection representation-encapsulation guard.
3+
#
4+
# Born from Discussion #12: `valueSetToForm` decoded a set's backing map as an
5+
# ArrayMap unconditionally, so a 9+-element set literal (promoted to a HashMap
6+
# backing) segfaulted macroexpansion. The same misread was then found live in
7+
# multimethod hierarchy dispatch (silently gone at the 9th defmethod), the
8+
# http client (9-entry :headers rejected) and the http server (>8 response
9+
# headers dropped). One bug, five sites, three symptom faces.
10+
#
11+
# The class: code OUTSIDE src/runtime/collection/ decoding a collection's
12+
# backing-representation struct directly. The representation (array vs HAMT,
13+
# promotion boundaries) is the collection module's private business; everyone
14+
# else iterates via the generic accessors (`map.forEachEntry`,
15+
# `set.forEachElem`, `count`, `get`, …), which handle every backing.
16+
#
17+
# What it does:
18+
# 1. HARD: flag any `decodePtr(*const <mod>.<ReprStruct>)` outside
19+
# src/runtime/collection/ whose line (or the line above) does not carry a
20+
# `repr-decode-ok:` waiver. Violations beyond BASELINE fail --gate.
21+
# 2. WARN (informational, never fails): a lone one-representation tag test
22+
# (`== .array_map` with no `hash_map` nearby) — the guard-shape that is
23+
# memory-safe but drops semantics past the promotion boundary.
24+
#
25+
# Waiver form (line above or same line; keep it greppable):
26+
# // repr-decode-ok: <why this site may know the representation> [refs: …]
27+
#
28+
# Modes:
29+
# bash scripts/check_repr_decode.sh informational; always exits 0
30+
# bash scripts/check_repr_decode.sh --strict exit 1 on any violation
31+
# bash scripts/check_repr_decode.sh --gate exit 1 if violations > BASELINE
32+
33+
set -euo pipefail
34+
35+
BASELINE=0
36+
MODE="${1:-info}"
37+
38+
cd "$(dirname "$0")/.."
39+
40+
# Representation structs private to src/runtime/collection/. Matching on the
41+
# TYPE name (not the import alias) is deliberate: files alias the module as
42+
# map_collection / map_mod / set_mod / …, but the struct names are unique in
43+
# the tree today.
44+
REPR_TYPES='ArrayMap|PersistentHashMap|HamtMapNode|HashCollisionMapNode|PersistentHashSet|SortedMap|SortedSet|RbNode|Vector|TailNode|HamtNode'
45+
46+
violations=0
47+
while IFS= read -r hit; do
48+
file="${hit%%:*}"
49+
rest="${hit#*:}"
50+
line="${rest%%:*}"
51+
# Waiver on the flagged line or the line above.
52+
if sed -n "$((line > 1 ? line - 1 : 1)),${line}p" "$file" | grep -q 'repr-decode-ok:'; then
53+
continue
54+
fi
55+
echo "REPR-DECODE: $hit"
56+
violations=$((violations + 1))
57+
done < <(grep -rnE "decodePtr\(\*(const )?[A-Za-z_]+\.(${REPR_TYPES})\)" src \
58+
--include='*.zig' | grep -v '^src/runtime/collection/' || true)
59+
60+
if [ "$violations" -gt 0 ]; then
61+
echo "check_repr_decode: $violations un-waived representation decode(s) outside src/runtime/collection/."
62+
echo " Iterate via the generic accessors (map.forEachEntry / set.forEachElem / count / get)"
63+
echo " — they handle every backing, incl. past the ArrayMap→HashMap promotion at 9 entries."
64+
echo " A site that legitimately must know the representation takes an inline waiver:"
65+
echo " // repr-decode-ok: <why> [refs: …]"
66+
else
67+
echo "check_repr_decode: no un-waived representation decodes outside src/runtime/collection/"
68+
fi
69+
70+
# Secondary, WARN-only: single-representation tag tests with no dual-backing
71+
# awareness on the same line. Catches the guard-shape (semantics silently
72+
# dropped past promotion) once the decode itself is gone. Deliberately not a
73+
# gate: printers/serializers legitimately scope by tag.
74+
warns=$(grep -rnE '[=!]= *\.array_map' src --include='*.zig' \
75+
| grep -v '^src/runtime/collection/' | grep -v 'hash_map' | grep -v 'repr-decode-ok:' || true)
76+
if [ -n "$warns" ]; then
77+
echo
78+
echo "WARN (informational) — lone .array_map tag tests (no hash_map on the line):"
79+
printf '%s\n' "$warns" | sed 's/^/ /'
80+
fi
81+
82+
case "$MODE" in
83+
--strict) [ "$violations" -eq 0 ] || exit 1 ;;
84+
--gate) [ "$violations" -le "$BASELINE" ] || exit 1 ;;
85+
*) ;;
86+
esac
87+
exit 0

‎src/app/error_render.zig‎

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -226,20 +226,23 @@ fn formatErrorEdn(info: error_mod.Info, ctx: error_print.SourceContext, w: *Writ
226226
}
227227
}
228228
// Merge the snapshotted `cljw.error/*error-context*` entries as
229-
// top-level event fields (ADR-0055 D3). array_map only — context
230-
// maps are small (the >8-entry hash_map path mirrors printMap's
231-
// limitation; a large context is rare and never silently truncated
232-
// because count ≤ 8 stays array_map).
229+
// top-level event fields (ADR-0055 D3). Iterate via the generic
230+
// `forEachEntry` so a >8-entry context (promoted to hash_map) lands in
231+
// the event whole — the old array_map-only decode skipped it entirely
232+
// (Discussion #12 bug class).
233233
if (info.context) |ctx_map| {
234-
if (ctx_map.tag() == .array_map) {
235-
const am = ctx_map.decodePtr(*const map_collection.ArrayMap);
236-
var i: u32 = 0;
237-
while (i < am.count) : (i += 1) {
238-
try w.writeByte(' ');
239-
try print_value(w, am.entries[2 * i]);
240-
try w.writeByte(' ');
241-
try print_value(w, am.entries[2 * i + 1]);
242-
}
234+
if (ctx_map.tag() == .array_map or ctx_map.tag() == .hash_map) {
235+
const CtxEmit = struct {
236+
w: *Writer,
237+
fn put(c: *@This(), k: Value, v: Value) anyerror!void {
238+
try c.w.writeByte(' ');
239+
try print_value(c.w, k);
240+
try c.w.writeByte(' ');
241+
try print_value(c.w, v);
242+
}
243+
};
244+
var cctx: CtxEmit = .{ .w = w };
245+
try map_collection.forEachEntry(ctx_map, &cctx, CtxEmit.put);
243246
}
244247
}
245248
try w.writeAll("}\n");

‎src/eval/backend/clojure_lang_method.zig‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -215,8 +215,8 @@ pub fn tryClojureLangMethod(
215215
// `(comparator [_] (.comparator backing-sorted-map))` chains through here.
216216
if (std.mem.eql(u8, name, "comparator") and args.len == 0) {
217217
const comp = switch (receiver.tag()) {
218-
.sorted_map => receiver.decodePtr(*const sorted.SortedMap).comparator,
219-
.sorted_set => receiver.decodePtr(*const sorted.SortedSet).map.decodePtr(*const sorted.SortedMap).comparator,
218+
.sorted_map => receiver.decodePtr(*const sorted.SortedMap).comparator, // repr-decode-ok: comparator field read; sorted_map has a single backing
219+
.sorted_set => receiver.decodePtr(*const sorted.SortedSet).map.decodePtr(*const sorted.SortedMap).comparator, // repr-decode-ok: comparator field read; sorted_set has a single backing
220220
else => return null,
221221
};
222222
if (!comp.isNil()) return comp;

‎src/eval/bytecode/serialize.zig‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -385,7 +385,7 @@ fn writeValueRaw(ctx: *WriteCtx, w: *std.Io.Writer, v: Value) SerializeError!voi
385385
},
386386
.array_map => {
387387
try writeU8(w, @intFromEnum(ValueTag.array_map));
388-
const am = v.decodePtr(*const map_mod.ArrayMap);
388+
const am = v.decodePtr(*const map_mod.ArrayMap); // repr-decode-ok: serialized format is representation-dependent by design; a hash_map raises HashMapNotSerializable
389389
try writeU32(w, am.count);
390390
var i: u32 = 0;
391391
while (i < am.count) : (i += 1) {
@@ -402,9 +402,9 @@ fn writeValueRaw(ctx: *WriteCtx, w: *std.Io.Writer, v: Value) SerializeError!voi
402402
},
403403
.hash_set => {
404404
try writeU8(w, @intFromEnum(ValueTag.hash_set));
405-
const s = v.decodePtr(*const set_mod.PersistentHashSet);
405+
const s = v.decodePtr(*const set_mod.PersistentHashSet); // repr-decode-ok: set backing read for the serializable (array_map) check below
406406
if (s.map.tag() != .array_map) return SerializeError.HashMapNotSerializable;
407-
const am = s.map.decodePtr(*const map_mod.ArrayMap);
407+
const am = s.map.decodePtr(*const map_mod.ArrayMap); // repr-decode-ok: serialized format is representation-dependent by design; guarded by the check above
408408
try writeU32(w, am.count);
409409
var i: u32 = 0;
410410
while (i < am.count) : (i += 1) try writeValue(ctx, w, am.entries[2 * i]);

‎src/lang/primitive/sequence.zig‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -592,8 +592,8 @@ pub fn emptyFn(rt: *Runtime, env: *Env, args: []const Value, loc: SourceLocation
592592
// Sorted variants keep their comparator (clj PersistentTreeMap.empty()
593593
// returns an empty tree with the same comp) — data.priority-map's
594594
// `(empty priority->set-of-items)` relies on it.
595-
.sorted_map => try sorted.emptyMapBy(rt, coll.decodePtr(*const sorted.SortedMap).comparator),
596-
.sorted_set => try sorted.emptySetBy(rt, coll.decodePtr(*const sorted.SortedSet).map.decodePtr(*const sorted.SortedMap).comparator),
595+
.sorted_map => try sorted.emptyMapBy(rt, coll.decodePtr(*const sorted.SortedMap).comparator), // repr-decode-ok: comparator field read; sorted_map has a single backing
596+
.sorted_set => try sorted.emptySetBy(rt, coll.decodePtr(*const sorted.SortedSet).map.decodePtr(*const sorted.SortedMap).comparator), // repr-decode-ok: comparator field read; sorted_set has a single backing
597597
.persistent_queue => try persistent_queue.emptyQueue(rt), // (empty q) → EMPTY
598598
.list, .cons => try list.emptyList(rt), // (empty '(1 2)) → () (D-164)
599599
// JVM Clojure: (empty "hi") → nil (String is not a Clojure

‎src/lang/primitive/walk.zig‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ fn rebuildArrayMap(
165165
ctx: anytype,
166166
) anyerror!Value {
167167
var result = map_collection.empty();
168-
const am = form.decodePtr(*const map_collection.ArrayMap);
168+
const am = form.decodePtr(*const map_collection.ArrayMap); // repr-decode-ok: dispatch-guaranteed .array_map; the .hash_map arm routes to rebuildHashMap
169169
// GC-ROOT: C6 — root the source map + accumulator + the in-flight [k v] entry
170170
// across transform_child (D-252) [ref: .dev/gc_rooting.md §C].
171171
var gc_roots: [3]Value = .{ form, result, .nil_val };

‎src/runtime/cljw/http/client.zig‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -58,18 +58,24 @@ fn doRequest(rt: *Runtime, method: std.http.Method, args: []const Value, loc: So
5858

5959
const hdr_v = map_mod.get(m, try keyword_mod.intern(rt, null, "headers")) catch Value.nil_val;
6060
if (!hdr_v.isNil()) {
61-
if (hdr_v.tag() != .array_map)
61+
if (hdr_v.tag() != .array_map and hdr_v.tag() != .hash_map)
6262
return error_catalog.raise(.http_opts_invalid, loc, .{ .detail = "the :headers option must be a map of string→string" });
63-
const am = hdr_v.decodePtr(*const map_mod.ArrayMap);
63+
// Iterate via the generic `forEachEntry`: a 9+-entry :headers map
64+
// promotes to hash_map and the old array_map-only decode rejected
65+
// it as "not a map" (Discussion #12 bug class).
6466
var list: std.ArrayList(std.http.Header) = .empty;
65-
var i: u32 = 0;
66-
while (i < am.count) : (i += 1) {
67-
const k = am.entries[2 * i];
68-
const v = am.entries[2 * i + 1];
69-
if (!k.isString() or !v.isString())
70-
return error_catalog.raise(.http_opts_invalid, loc, .{ .detail = "each :headers entry must be string→string" });
71-
try list.append(scratch, .{ .name = string_mod.asString(k), .value = string_mod.asString(v) });
72-
}
67+
const HdrCollect = struct {
68+
scratch: std.mem.Allocator,
69+
list: *std.ArrayList(std.http.Header),
70+
loc: SourceLocation,
71+
fn put(c: *@This(), k: Value, v: Value) anyerror!void {
72+
if (!k.isString() or !v.isString())
73+
return error_catalog.raise(.http_opts_invalid, c.loc, .{ .detail = "each :headers entry must be string→string" });
74+
try c.list.append(c.scratch, .{ .name = string_mod.asString(k), .value = string_mod.asString(v) });
75+
}
76+
};
77+
var hctx: HdrCollect = .{ .scratch = scratch, .list = &list, .loc = loc };
78+
try map_mod.forEachEntry(hdr_v, &hctx, HdrCollect.put);
7379
extra_headers = list.items;
7480
}
7581
}

‎src/runtime/cljw/http/server.zig‎

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -188,23 +188,34 @@ pub fn runServer(
188188
const b = map_mod.get(resp, kw_body) catch Value.nil_val;
189189
if (b.tag() == .string) body = string_mod.asString(b);
190190
const h = map_mod.get(resp, kw_headers) catch Value.nil_val;
191-
if (h.tag() == .array_map) {
192-
const am = h.decodePtr(*const map_mod.ArrayMap);
193-
var i: u32 = 0;
194-
while (i < am.count and n_headers < header_buf.len) : (i += 1) {
195-
const hk = am.entries[2 * i];
196-
const hv = am.entries[2 * i + 1];
197-
if (hk.tag() == .string and hv.tag() == .string) {
191+
if (h.tag() == .array_map or h.tag() == .hash_map) {
192+
// Iterate via the generic `forEachEntry`: a 9+-entry :headers
193+
// map promotes to hash_map and the old array_map-only decode
194+
// dropped ALL custom headers past that boundary (Discussion
195+
// #12 bug class). The 16-header cap + SE-5 CRLF check are
196+
// preserved inside the callback.
197+
const HdrEmit = struct {
198+
header_buf: *[16]std.http.Header,
199+
n_headers: *usize,
200+
headers_clean: *bool,
201+
fn put(c: *@This(), hk: Value, hv: Value) anyerror!void {
202+
if (!c.headers_clean.* or c.n_headers.* >= c.header_buf.len) return;
203+
if (hk.tag() != .string or hv.tag() != .string) return;
198204
const nm = string_mod.asString(hk);
199205
const vl = string_mod.asString(hv);
200206
if (!headerFieldClean(nm) or !headerFieldClean(vl)) {
201-
headers_clean = false;
202-
break;
207+
c.headers_clean.* = false;
208+
return;
203209
}
204-
header_buf[n_headers] = .{ .name = nm, .value = vl };
205-
n_headers += 1;
210+
c.header_buf[c.n_headers.*] = .{ .name = nm, .value = vl };
211+
c.n_headers.* += 1;
206212
}
207-
}
213+
};
214+
var hctx: HdrEmit = .{ .header_buf = &header_buf, .n_headers = &n_headers, .headers_clean = &headers_clean };
215+
// The callback never errors and the tag is guarded above, so a
216+
// failure here is unreachable; swallow to keep the serve loop
217+
// crash-free on handler data (the surrounding contract).
218+
map_mod.forEachEntry(h, &hctx, HdrEmit.put) catch {};
208219
}
209220
}
210221
// SE-5: a control byte in any response header would split the response (or

‎src/runtime/multimethod.zig‎

Lines changed: 30 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -287,32 +287,40 @@ pub fn getMethod(
287287
// Cycle 2-3 hierarchy resolution: collect isa? candidates;
288288
// if ≥ 2 use `dominates` (prefer-method ∪ isa?) to pick the
289289
// best; if no dominator survives the walk → raise ambiguity.
290-
var best_key: ?Value = null;
291-
var best_method: ?Value = null;
292-
var ambiguous = false;
293-
if (mf.method_table.tag() == .array_map) {
294-
const am = mf.method_table.decodePtr(*const map_mod.ArrayMap);
295-
var i: u32 = 0;
296-
while (i < am.count) : (i += 1) {
297-
const key = am.entries[2 * i];
298-
if (@intFromEnum(key) == @intFromEnum(dispatch_val)) continue;
299-
if (!(try isaCheck(ancestors, dispatch_val, key))) continue;
300-
301-
if (best_key) |bk| {
302-
if (try dominates(mf.prefer_table, ancestors, key, bk)) {
303-
best_key = key;
304-
best_method = am.entries[2 * i + 1];
305-
ambiguous = false;
306-
} else if (!(try dominates(mf.prefer_table, ancestors, bk, key))) {
307-
ambiguous = true;
290+
// Iterate via the generic `forEachEntry` so a method_table that has
291+
// promoted past the ArrayMap ceiling (9+ defmethods) is walked too —
292+
// the old array_map-only decode silently skipped the whole isa? walk
293+
// past that boundary, so hierarchy dispatch vanished at the 9th method
294+
// while exact-match kept working (Discussion #12 bug class).
295+
const IsaWalk = struct {
296+
prefer_table: Value,
297+
ancestors: Value,
298+
dispatch_val: Value,
299+
best_key: ?Value = null,
300+
best_method: ?Value = null,
301+
ambiguous: bool = false,
302+
fn visit(c: *@This(), key: Value, method: Value) anyerror!void {
303+
if (@intFromEnum(key) == @intFromEnum(c.dispatch_val)) return;
304+
if (!(try isaCheck(c.ancestors, c.dispatch_val, key))) return;
305+
306+
if (c.best_key) |bk| {
307+
if (try dominates(c.prefer_table, c.ancestors, key, bk)) {
308+
c.best_key = key;
309+
c.best_method = method;
310+
c.ambiguous = false;
311+
} else if (!(try dominates(c.prefer_table, c.ancestors, bk, key))) {
312+
c.ambiguous = true;
308313
}
309314
} else {
310-
best_key = key;
311-
best_method = am.entries[2 * i + 1];
315+
c.best_key = key;
316+
c.best_method = method;
312317
}
313318
}
314-
}
315-
if (ambiguous) {
319+
};
320+
var walk: IsaWalk = .{ .prefer_table = mf.prefer_table, .ancestors = ancestors, .dispatch_val = dispatch_val };
321+
try map_mod.forEachEntry(mf.method_table, &walk, IsaWalk.visit);
322+
const best_method = walk.best_method;
323+
if (walk.ambiguous) {
316324
const name_sym = symbol.asSymbol(mf.name);
317325
return error_catalog.raise(.multimethod_ambiguous_dispatch, loc, .{
318326
.name = name_sym.name,

0 commit comments

Comments
 (0)