Skip to content

Commit d8006d4

Browse files
committed
fix(api): Stash Notecard "err" message into allocator-backed storage
The Api singleton thunk path captures Notecard error strings into a caller-stack `detail::NcErrorCapture` whose `view()` returned a pointer into its own `buf[64]`. Generated `execute()` then constructed `ApiResult<...>(ErrorInfo{..., nc_err_.view()})` whose `ErrorMessage` held that pointer — undefined behavior the moment execute() returned and the stack frame was reused. Reading `result.error().message` after a Notecard error returned silently truncated bytes or random heap contents. The non-singleton path doesn't have the bug because it routes through `streaming_attempt`'s `pool.intern(err)` (allocator-backed, heap-leaked, durable). Adds `Notecard::stash_nc_err(string_view)` and the matching `StaticNotecard::stash_nc_err(string_view)` that copy the bytes into the configured allocator (one allocation per Notecard error, never freed — same lifetime model as `StringPool::intern`). Adds an optional `durable_ptr` field to `NcErrorCapture` whose `view()` prefers the durable pointer when set. The api.hpp.j2 thunks call `stash_nc_err` after `execute_void`/`execute_generic_with_body` returns and route the durable pointer back through `set_durable`, so the generated code needs no change — `nc_err_.view()` just starts returning durable bytes automatically. Avoids fixed-size buffers entirely: arena allocators pay only for the messages they actually receive, and zero-allocator builds get `nullptr` and fall back to the original (still-dangling) buf-pointer path — no worse than before, no extra RAM consumed. NOTE_MINIMAL gates the entire stash mechanism out — the durable_ptr field, set_durable(), the view() branch, stash_nc_err() itself, and the thunk's call to it all collapse to the original code. AVR-class targets (which set NOTE_MINIMAL) typically branch on `result.error().code` and ignore the textual message; the +250 B of flash for the durable-copy logic isn't worth it there. AVR sizes verified at baseline post-change (avr-notecpp / direct / raw / multistack all unchanged). Test impact: the streaming-error tests that previously failed under NOTE_SINGLETON=1 (e.g. `streaming: void endpoint with Notecard error`, `streaming: Notecard error response propagates`) now pass because `rsp.error().message == "<text>"` reads stable bytes. Failure count in the singleton-only build drops from 73 to 65; the remaining 65 are unrelated pre-existing limitations of singleton mode (GenericResponseSink can't dispatch array fields; thunk path doesn't inject request IDs). Also documents the rationale for the existing `binary_ctrl_buf_[256]` fixed-size buffer in Notecard, since it's the same pattern this fix deliberately avoided.
1 parent 4d5a6e9 commit d8006d4

5 files changed

Lines changed: 148 additions & 2 deletions

File tree

src/note/api.hpp

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,13 +140,43 @@ class Api {
140140
#if NOTE_SINGLETON
141141
// Static thunks — one copy each, shared by all endpoints via create_().
142142
// Eliminates per-type lambda instantiation in the factory methods.
143+
//
144+
// After execute(), if a Notecard "err" was captured, route the bytes
145+
// through `stash_nc_err()` so they live in allocator-backed storage
146+
// beyond the caller's stack. Generated `execute()` later builds an
147+
// `ApiResult<...>(ErrorInfo{..., err.view()})` whose pointer would
148+
// otherwise dangle the moment execute() returns. `stash_nc_err()`
149+
// returns nullptr when no allocator is configured; the fallback to
150+
// `NcErrorCapture::buf` is still dangling in that case, but no
151+
// worse than before — and no fixed-size buffer is added for the
152+
// common case.
153+
//
154+
// The stash call is skipped under NOTE_MINIMAL: AVR-class targets
155+
// typically don't surface the textual error message (they branch on
156+
// `result.error().code` and ignore the string), so the +250 bytes
157+
// of flash for the durable-copy logic isn't worth it. The dangling
158+
// `view()` quirk persists there, gated to that minimal build only.
143159
static Result<void> void_thunk_(void* p, string_view req, BuildFn fn, void* ctx, detail::NcErrorCapture& err) {
160+
#if NOTE_MINIMAL
144161
return static_cast<NcT*>(p)->execute_void(req, fn, ctx, err);
162+
#else
163+
auto* nc = static_cast<NcT*>(p);
164+
auto rv = nc->execute_void(req, fn, ctx, err);
165+
err.set_durable(nc->stash_nc_err(err.view()));
166+
return rv;
167+
#endif
145168
}
146169
static Result<void> generic_thunk_(void* p, string_view req, BuildFn fn, void* ctx,
147170
void* rsp, const FieldDesc* f, uint8_t n, detail::NcErrorCapture& err, bool& ex,
148171
void* body_ptr, BodyHandlerFactory body_factory) {
172+
#if NOTE_MINIMAL
149173
return static_cast<NcT*>(p)->execute_generic_with_body(req, fn, ctx, rsp, f, n, err, ex, body_ptr, body_factory);
174+
#else
175+
auto* nc = static_cast<NcT*>(p);
176+
auto rv = nc->execute_generic_with_body(req, fn, ctx, rsp, f, n, err, ex, body_ptr, body_factory);
177+
err.set_durable(nc->stash_nc_err(err.view()));
178+
return rv;
179+
#endif
150180
}
151181
static Result<void> send_thunk_(void* p, BuildFn fn, void* ctx) {
152182
return static_cast<NcT*>(p)->send_command(fn, ctx);

src/note/notecard.hpp

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -690,6 +690,29 @@ class Notecard {
690690

691691
JsonBackend& backend() { return *backend_; }
692692

693+
/// Allocator-backed durable copy of a Notecard error message — used
694+
/// by the Api singleton thunk path to give `NcErrorCapture::view()`
695+
/// a pointer that outlives the caller-stack-allocated NcErrorCapture
696+
/// frame. Without this, generated `execute()` returns an
697+
/// `ApiResult<...>` whose `ErrorMessage` points into dead stack
698+
/// memory by the time `result.error().message` is read.
699+
///
700+
/// Allocates `sv.size()` bytes from the configured `alloc_` (one
701+
/// allocation per Notecard error; never freed — same heap-leaked
702+
/// pattern as `streaming_attempt`'s `pool.intern(err)` on the
703+
/// non-singleton path). Returns `nullptr` if no allocator is
704+
/// configured or the allocation fails — caller falls back to
705+
/// `NcErrorCapture::buf` (still dangling past execute(), but at
706+
/// least no fixed buffer is consumed for the rare no-allocator
707+
/// build).
708+
const char* stash_nc_err(string_view sv) {
709+
if (sv.empty() || !alloc_.has_value()) return nullptr;
710+
auto* p = static_cast<char*>(alloc_->allocate(sv.size()));
711+
if (!p) return nullptr;
712+
for (size_t i = 0; i < sv.size(); ++i) p[i] = sv[i];
713+
return p;
714+
}
715+
693716
private:
694717
template<typename RequestT>
695718
ApiResult<typename RequestT::Response> do_binary_send(RequestT& req) {
@@ -1081,7 +1104,22 @@ class Notecard {
10811104
std::optional<Allocator> alloc_;
10821105
DebugListener debug_{};
10831106
byte_span cobs_buf_{}; // optional external COBS working buffer
1084-
char binary_ctrl_buf_[256]{}; // buffer for binary control command responses
1107+
/// Synchronous-response buffer for binary-transfer control commands
1108+
/// (`{"req":"card.binary"}` status / `delete:true` reset). Fixed
1109+
/// 256 bytes because:
1110+
/// - `do_binary_send`/`do_binary_receive` issue these queries
1111+
/// synchronously and need somewhere to land the response before
1112+
/// extracting `max`, `length`, `status` (MD5).
1113+
/// - Sized to fit the largest realistic card.binary status reply
1114+
/// (offset/length/cobs/max integers + a 32-byte MD5 + scaffolding).
1115+
/// Smaller responses leave headroom; the builder doesn't grow.
1116+
/// - Lives on the Notecard (not the request) so the binary-transfer
1117+
/// orchestration helpers can stay non-template and avoid pulling
1118+
/// in the full request type just for a status query.
1119+
/// Trade-off: 256 bytes per Notecard even for builds that never use
1120+
/// binary transfer. Acceptable on hosts/MCUs (Notecards are rarely
1121+
/// instantiated more than once); not used on AVR-class targets.
1122+
char binary_ctrl_buf_[256]{};
10851123
#ifndef NOTE_RSP_BUF_SIZE
10861124
#define NOTE_RSP_BUF_SIZE 1024
10871125
#endif

src/note/protocol.hpp

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,14 +89,41 @@ struct NcErrorCapture {
8989
static constexpr size_t kMaxLen = 64;
9090
char buf[kMaxLen]{};
9191
size_t len = 0;
92+
#if !NOTE_MINIMAL
93+
/// Optional pointer to a durable copy of the captured bytes — set by
94+
/// the singleton thunk path via `Notecard::stash_nc_err(...)` so that
95+
/// `view()` returns a string_view that outlives the local
96+
/// NcErrorCapture frame. When nullptr, view() falls back to `buf`
97+
/// (caller-stack lifetime, used by the StaticNotecard transact path
98+
/// which captures bytes directly into `buf` during SAX dispatch).
99+
/// Gated out under NOTE_MINIMAL: AVR-class targets accept the
100+
/// dangling-view quirk in exchange for ~90 B less flash.
101+
const char* durable_ptr = nullptr;
102+
#endif
92103

93104
void capture(string_view v) {
94105
len = v.size() < kMaxLen ? v.size() : kMaxLen;
95106
for (size_t i = 0; i < len; ++i) buf[i] = v[i];
107+
#if !NOTE_MINIMAL
108+
durable_ptr = nullptr;
109+
#endif
96110
}
97111

112+
#if !NOTE_MINIMAL
113+
/// Override view()'s pointer with allocator-backed bytes that outlive
114+
/// this NcErrorCapture. `len` stays as the captured length. Caller is
115+
/// responsible for keeping `data` alive — typically done by routing
116+
/// through the Notecard's allocator via `stash_nc_err(...)`.
117+
void set_durable(const char* data) { durable_ptr = data; }
118+
#endif
119+
98120
bool empty() const { return len == 0; }
99-
string_view view() const { return {buf, len}; }
121+
string_view view() const {
122+
#if !NOTE_MINIMAL
123+
if (durable_ptr) return {durable_ptr, len};
124+
#endif
125+
return {buf, len};
126+
}
100127
};
101128

102129
/// ReceiveContext — wraps a SaxDispatch to intercept "err" and "crc" fields.

src/note/static_notecard.hpp

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,27 @@ class StaticNotecard {
236236
/// Access the transport stack (e.g. for binary I/O).
237237
Stack& stack() { return stack_; }
238238

239+
#if !NOTE_MINIMAL
240+
/// Mirror of `Notecard::stash_nc_err` for the Api singleton-thunk
241+
/// path. Allocates the bytes from the Notecard's configured
242+
/// allocator (heap-leaked — same pattern as StringPool::intern; the
243+
/// allocator's lifetime carries the bytes). No fixed buffer: arena
244+
/// builds pay only for the messages they actually receive, and
245+
/// zero-allocator builds get nullptr (caller falls back to the
246+
/// caller-stack `NcErrorCapture::buf`, dangling past execute() —
247+
/// the same lifetime quirk that existed before this fix).
248+
/// Gated out under NOTE_MINIMAL: the singleton thunk on AVR-class
249+
/// targets doesn't call this, so the function would otherwise sit
250+
/// unused; gating ensures the linker doesn't accidentally retain it.
251+
const char* stash_nc_err(string_view sv) {
252+
if (sv.empty()) return nullptr;
253+
auto* p = static_cast<char*>(alloc_.allocate(sv.size()));
254+
if (!p) return nullptr;
255+
for (size_t i = 0; i < sv.size(); ++i) p[i] = sv[i];
256+
return p;
257+
}
258+
#endif
259+
239260
/// Non-template transact with retry. Wraps transact_dispatch in retry_loop.
240261
Result<void> transact_retried(BuildFn build_fn, void* build_ctx,
241262
SaxDispatch dispatch,

tools/codegen/templates/api.hpp.j2

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,13 +80,43 @@ public:
8080
#if NOTE_SINGLETON
8181
// Static thunks — one copy each, shared by all endpoints via create_().
8282
// Eliminates per-type lambda instantiation in the factory methods.
83+
//
84+
// After execute(), if a Notecard "err" was captured, route the bytes
85+
// through `stash_nc_err()` so they live in allocator-backed storage
86+
// beyond the caller's stack. Generated `execute()` later builds an
87+
// `ApiResult<...>(ErrorInfo{..., err.view()})` whose pointer would
88+
// otherwise dangle the moment execute() returns. `stash_nc_err()`
89+
// returns nullptr when no allocator is configured; the fallback to
90+
// `NcErrorCapture::buf` is still dangling in that case, but no
91+
// worse than before — and no fixed-size buffer is added for the
92+
// common case.
93+
//
94+
// The stash call is skipped under NOTE_MINIMAL: AVR-class targets
95+
// typically don't surface the textual error message (they branch on
96+
// `result.error().code` and ignore the string), so the +250 bytes
97+
// of flash for the durable-copy logic isn't worth it. The dangling
98+
// `view()` quirk persists there, gated to that minimal build only.
8399
static Result<void> void_thunk_(void* p, string_view req, BuildFn fn, void* ctx, detail::NcErrorCapture& err) {
100+
#if NOTE_MINIMAL
84101
return static_cast<NcT*>(p)->execute_void(req, fn, ctx, err);
102+
#else
103+
auto* nc = static_cast<NcT*>(p);
104+
auto rv = nc->execute_void(req, fn, ctx, err);
105+
err.set_durable(nc->stash_nc_err(err.view()));
106+
return rv;
107+
#endif
85108
}
86109
static Result<void> generic_thunk_(void* p, string_view req, BuildFn fn, void* ctx,
87110
void* rsp, const FieldDesc* f, uint8_t n, detail::NcErrorCapture& err, bool& ex,
88111
void* body_ptr, BodyHandlerFactory body_factory) {
112+
#if NOTE_MINIMAL
89113
return static_cast<NcT*>(p)->execute_generic_with_body(req, fn, ctx, rsp, f, n, err, ex, body_ptr, body_factory);
114+
#else
115+
auto* nc = static_cast<NcT*>(p);
116+
auto rv = nc->execute_generic_with_body(req, fn, ctx, rsp, f, n, err, ex, body_ptr, body_factory);
117+
err.set_durable(nc->stash_nc_err(err.view()));
118+
return rv;
119+
#endif
90120
}
91121
static Result<void> send_thunk_(void* p, BuildFn fn, void* ctx) {
92122
return static_cast<NcT*>(p)->send_command(fn, ctx);

0 commit comments

Comments
 (0)