fix(small_buf_map): use *const Self for read methods to avoid use-after-stack-free#2
fix(small_buf_map): use *const Self for read methods to avoid use-after-stack-free#2jeffoodchain wants to merge 1 commit into
Conversation
…er-stack-free
## Problem
`SmallBufMap`'s read accessors (`get`, `getKey`, `getValue`, `getKeyValue`,
`getKeyIndex`, `count`, `dataCount`, `hasEntry`, `getPutOp`) took `self: Self`
— i.e. by value. Each call copied the entire `[buffer_size]u8` onto the
callee's stack. Methods returning a slice (`get`, `getKey`, `getValue`,
`getKeyValue`) returned a slice into that **stack-local copy**, which becomes
a dangling pointer the moment the function returns.
This is a use-after-stack-free: the bytes survive until the next call
overwrites that stack region, so whether the caller reads correct data or
zeros depends on surrounding code, optimization level, and compiler version
— undefined behavior, not a deterministic crash.
## Symptom
`zig build test` (debug, zig 0.14.1) failed on `enr.test.ENR test vector`
and `enr_bench.test.bench`:
```
error: 'enr.test.ENR test vector' failed: src/enr.zig:29:13: in init
return Error.BadID;
```
`ENR.decodeInto` calls `kvs.put(...)` for each KV, then `kvs.get("id")`.
The bytes at `kvs.buffer[2..4]` were correct (`0x76 0x34` = `"v4"`), but
`get` returned a slice into the freed stack frame, which had been clobbered
to zero by intervening calls — so `IDScheme.init` rejected it with
`Error.BadID`.
`EncodedENR.verify` also calls `kvs.get` and has the same latent bug; it
just isn't exercised by the current test set.
## Fix
Change every read method's receiver from `self: Self` to `self: *const Self`.
The slice returned by `get` now points into the caller's live `kvs.buffer`,
not a copy. Mutating methods (`put`, `append`, `insert`, `remove`, ...) were
already `*Self` and are unchanged. No call sites need updating — Zig
auto-coerces `kvs.get(...)` to pass `&kvs` regardless of receiver type.
Also avoids a `[buffer_size]u8` memcpy (229 bytes for `ENR.KVs`) on every read.
## Verification
Stress-tested by inserting `std.debug.print` / loops between `kvs.get("id")`
and the consumer in `ENR.decodeInto` — previously the slice's bytes were
clobbered by the prints, now they survive:
```
buffer head: 69 64 76 34 69 70 7f 00
kvs.get("id") = 7634 (2b) ← "v4", correct
```
`zig build test` is green.
There was a problem hiding this comment.
Code Review
This pull request refactors the SmallBufMap implementation by updating several method signatures to take a constant pointer to Self instead of passing the struct by value. The review feedback identifies a potential out-of-bounds access vulnerability in loops within the count, getKeyIndex, and getPutOp functions, suggesting that the loop bounds should be constrained by the buffer size. Additionally, it was recommended to simplify the getKeyIndex method by leveraging the existing logic in getPutOp to reduce code redundancy.
| } | ||
|
|
||
| pub fn count(self: Self) u8 { | ||
| pub fn count(self: *const Self) u8 { |
There was a problem hiding this comment.
The loop condition i < 256 in count (and similarly in getKeyIndex and getPutOp) can lead to an out-of-bounds access and runtime panic if the map is full.
Since each entry requires at least 2 bytes of metadata at the end of the buffer, the maximum number of entries is bounded by buffer_size / 2. When the map is full, hasEntry(i) will eventually attempt to access an index below zero (e.g., buffer_size - (2 * i) - 1).
Consider bounding the loop by buffer_size / 2 to ensure safety when the map reaches capacity.
| } | ||
|
|
||
| inline fn getKeyIndex(self: Self, key: []const u8) ?u8 { | ||
| inline fn getKeyIndex(self: *const Self, key: []const u8) ?u8 { |
There was a problem hiding this comment.
The implementation of getKeyIndex is redundant with getPutOp. Since the map is maintained in sorted order, getPutOp already performs an optimized search that exits early when it encounters a key greater than the target.
You could simplify this method by calling getPutOp and checking if the operation is .replace:
inline fn getKeyIndex(self: *const Self, key: []const u8) ?u8 {
const res = self.getPutOp(key);
return if (res.op == .replace) res.i else null;
}This would improve maintainability and slightly improve performance for missing keys.
Problem
SmallBufMap's read accessors (get,getKey,getValue,getKeyValue,getKeyIndex,count,dataCount,hasEntry,getPutOp) tookself: Self— i.e. by value. Each call copied the entire[buffer_size]u8onto the callee's stack. Methods returning a slice (get,getKey,getValue,getKeyValue) returned a slice into that stack-local copy, which becomes a dangling pointer the moment the function returns.This is a use-after-stack-free: the bytes survive until the next call
overwrites that stack region, so whether the caller reads correct data or zeros depends on surrounding code, optimization level, and compiler version.
Issue
zig build test(debug, zig 0.14.1) failed onenr.test.ENR test vectorand
enr_bench.test.bench:ENR.decodeIntocallskvs.put(...)for each KV, thenkvs.get("id").The bytes at
kvs.buffer[2..4]were correct (0x76 0x34="v4"), butgetreturned a slice into the freed stack frame, which had been clobbered to0x00by intervening calls — soIDScheme.initrejected it withError.BadID.EncodedENR.verifyalso callskvs.getand has the same latent bug; it just isn't exercised by the current test set.Fix
Change every read method's receiver from
self: Selftoself: *const Self.The slice returned by
getnow points into the caller's livekvs.buffer,not a copy. Mutating methods (
put,append,insert,remove, ...) werealready
*Selfand are unchanged. No call sites need updating — Zigauto-coerces
kvs.get(...)to pass&kvsregardless of receiver type.Also avoids a
[buffer_size]u8memcpy (229 bytes forENR.KVs) on every read.Verification
Stress-tested by inserting
std.debug.print/ loops betweenkvs.get("id")and the consumer in
ENR.decodeInto— previously the slice's bytes wereclobbered by the prints, now they survive: