Conversation
…llback handle_trace_call_path first looks the function up by bare name (cbm_store_find_nodes_by_name). find_nodes_generic allocates its initial 16-slot array before stepping the statement and hands it back even when zero rows match. When the bare name misses and the qualified_name fallback then resolves the node, the handler overwrote `nodes` with a fresh one-element array, leaking the empty 1 KB array on every trace_path call made with a qualified name - for the whole lifetime of the daemon. The malloc-failure branch of the fallback lost the same array. Release the zero-row array before the fallback replaces the pointer. The other find_nodes_* callers (get_code_snippet outline, search_code grep classification, detect_changes seeding, Cypher label scans) already free the container on zero rows; the not-found path in this handler did too. Proof (macOS leak lane, Homebrew clang 22.1.8, `make -f Makefile.cbm test-lsan LSAN_SUITES=mcp`): - with the new test, without the fix: 322 passed, then "ERROR: LeakSanitizer: detected memory leaks - Direct leak of 1024 byte(s) in 1 object(s) allocated from find_nodes_generic store.c:2771 <- handle_trace_call_path mcp.c:9176 <- test_tool_trace_call_path_qn_fallback_frees_name_miss", exit 1 (the only leak in the suite). - with the fix: 322 passed, 4 skipped, no leak report, exit 0. The new test pins its own precondition (the bare-name lookup of the qualified name returns zero rows) and that the trace resolves through the fallback, so it keeps exercising this path. LSan (default-on under Linux ASan, test-lsan on macOS) turns any regression red. Found while attributing CI on #2305. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
trace_path first looks the function up by bare name.
find_nodes_generic(store.c) allocates its starting 16-slot array before reading any rows, and returns it even when nothing matches. When the bare name misses and the qualified_name fallback resolves the node,handle_trace_call_pathoverwrote that pointer with a new one-element array. So every trace_path call made with a qualified name leaked about 1 KB for the daemon's lifetime, and the malloc-failure branch lost the same array. The fix frees the empty result before the fallback replaces it (3 lines insrc/mcp/mcp.c). Every other caller of thefind_nodes_*lookups already frees the empty array.Regression test:
tool_trace_call_path_qn_fallback_frees_name_miss(mcp suite). It checks that the bare-name lookup really returns zero rows and that the trace resolves through the fallback. LSan catches the leak itself: it is on by default under Linux ASan, andmake -f Makefile.cbm test-lsanruns it on macOS.Proof, with
make -f Makefile.cbm test-lsan LSAN_SUITES=mcp:find_nodes_generic(store.c:2771) viahandle_trace_call_path. It is the only leak in the suite, and the run exits non-zero.Plain
build/c/test-runner mcpandmake -f Makefile.cbm lint-ciare green.Found while attributing CI on #2305. #2305 needs a branch update once this lands, so its CI picks up the fix.