fix(store): clamp ADR render cursor to prevent stack overflow - #1175
Conversation
Signed-off-by: SEPURI-SAI-KRISHNA <saik20533@gmail.com>
|
Thanks for the reproduce-first hardening fix. Triage is complete: this is a high-priority |
|
Thanks, and sorry for the wait. Queued for review. MERGEABLE, all 20 checks green, +71/-3 across two files — nothing blocking on your side. Clamping a render cursor to prevent stack overflow is exactly the kind of bounded, provable fix that is easy to review and easy to trust. |
|
Thank you — merged. I verified the overflow rather than trusting the writeup: in One thing I want to record accurately, because it affects how urgently anyone should backport this: Appreciated. |
What does this PR do?
Fixes a stack out-of-bounds write (CWE-787) in
adr_render_section(
src/store/store.c), the helpercbm_adr_renderuses to concatenate ADRsections into a fixed 16 KB stack buffer (
buf[ST_MAX_DEGREE * ST_GROWTH]).The cursor was advanced by the return value of
snprintf:snprintfreturns the number of bytes it would have written, so once theaccumulated sections exceed
buf_sz,posmoves past the end. On the nextsection
buf + pospoints past the buffer andbuf_sz - pos(anintsubtraction) goes negative, converting to a huge
size_t— sosnprintfwrites out of bounds.
The fix clamps
posinto[0, buf_sz - 1]after every write, so eachsnprintfreceives a positive size and the returned cursor never escapes thebuffer. This mirrors the clamping already done by
http_appendfinsrc/ui/http_server.cand the sibling accumulators instore.c(
search_build_exclude_labels,bfs_build_types_clause).Reproduced first (ASan, before the fix):
After the fix the full
store_archsuite (53 tests) passes with no sanitizererrors.
Adds a regression test
adr_render_oversized_sections_no_overflowthat sizesthree sections so the cursor lands exactly
buf_sz + 8after the secondsection — the third section then performs the first out-of-bounds write right
at the buffer tail (ASan redzone), making the failure deterministic.
Note on scope
cbm_adr_render/cbm_store_adr_update_sectionsare exported instore.hand exercised by the test suite, but currently have no production caller — the
manage_adrMCP tool and the UI/api/adrendpoint both persist raw contentvia
cbm_store_adr_store. So this is a latent defect in exported API ratherthan a live remote overflow; the fix hardens the renderer before anything
wires up the section-merge path.
Checklist
git commit -s) — required, CI rejectsunsigned commits (DCO, see CONTRIBUTING.md)
make -f Makefile.cbm test)make -f Makefile.cbm lint-ci)