Skip to content

fix(store,lint): clean up clang-tidy warnings and uninitialized store memory - #1298

Open
aoright wants to merge 1 commit into
DeusData:mainfrom
aoright:fix/lint-and-store-memory-uninit
Open

fix(store,lint): clean up clang-tidy warnings and uninitialized store memory#1298
aoright wants to merge 1 commit into
DeusData:mainfrom
aoright:fix/lint-and-store-memory-uninit

Conversation

@aoright

@aoright aoright commented Jul 27, 2026

Copy link
Copy Markdown

Summary

  1. Store Defect Fix: Replaced malloc with calloc in src/store/store.c (bfs_multi) for visited array allocation to prevent reading uninitialized node hop memory during traversal.
  2. Linter & Cognitive Complexity Fixes:
    • Extracted helper functions (cbm_pipeline_lsp_target_tail_match, git_dirty_signature_submodules, git_parse_porcelain_stream) to reduce cognitive complexity of functions below threshold (25).
    • Synchronized header declaration parameter names with implementation in pass_lsp_cross.h.
    • Replaced array magic numbers with CBM_SZ_* named constants from constants.h across headers and watcher.
    • Maintained exact 12-byte semver array definition via CBM_DAEMON_SEMVER_SIZE 12U in runtime.h.

Verification

  • scripts/build.sh succeeded.
  • scripts/test.sh passed 100% (6,748+ tests in 121 suites).

@aoright
aoright requested a review from DeusData as a code owner July 27, 2026 09:45
@aoright
aoright force-pushed the fix/lint-and-store-memory-uninit branch from 795b4e4 to a84975c Compare July 27, 2026 09:59
… memory

- Fix uninitialized memory allocation defect in store.c bfs_multi (malloc -> calloc)
- Synchronize header parameter names with implementation in pass_lsp_cross.h
- Refactor complex functions (cbm_pipeline_find_lsp_resolution, git_dirty_signature) to reduce cognitive complexity below threshold
- Replace magic number array bounds with named constants from constants.h across headers and watcher

Signed-off-by: aoright <102943475+aoright@users.noreply.github.com>
@aoright
aoright force-pushed the fix/lint-and-store-memory-uninit branch from a84975c to 45324c2 Compare July 28, 2026 01:10
@DeusData DeusData added bug Something isn't working stability/performance Server crashes, OOM, hangs, high CPU/memory priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Jul 28, 2026
@DeusData DeusData added this to the 0.9.1-rc milestone Jul 28, 2026
@DeusData

DeusData commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Thank you for the contribution and for identifying the uninitialized traversal state while documenting the broader lint cleanup. This is now triaged as a high-priority stability bug for 0.9.1-rc. Our community PR queue is currently quite full, so it may take a little time before we can complete the review and, if approved, merge it. We are doing our best to support community contributions and will return with code-grounded feedback as capacity opens.

@DeusData

Copy link
Copy Markdown
Owner

Thank you for this — the lint cleanup itself is legitimate and well done, and I want to be specific about what I verified before raising the one thing that needs changing.

What checks out. I compared every constant substitution against constants.h — all of them are value-identical with no signedness change. The watcher.c split into git_parse_porcelain_stream / git_dirty_signature_submodules, and the poll_project split, are control-flow-preserving moves; I walked them branch by branch including the COMMAND_FAILED fall-through, the fclose-once path and the "reserve 0 for clean" signature convention. The lsp_resolve.h helper extractions are byte-identical, the pass_lsp_cross.h parameter renames match their implementations, and the git submodule foreach string is unchanged — only relocated. That is careful work.

The one thing I have to push back on: the "Store Defect Fix" is not a defect fix.

The malloccalloc change in cbm_store_bfs_multi is described as fixing uninitialized memory, but there is no uninitialized read to fix. scan_node initialises all nine node fields, .hop is assigned immediately afterwards, and only [0, n) is ever read — visited_count is set to n, and the sentinel row at max_results is written before the loop breaks.

And if such a read did exist, calloc would not close it: the safe_realloc growth path beyond the initial capacity is never zeroed, so the bug would simply move to the grown region.

Nothing here is careless, and the change is harmless — it costs a memset and nothing else. But a PR that claims to fix a memory defect sets an expectation for anyone reading the history later, and this project's convention is reproduce-first: a claimed defect comes with a test that fails without the fix. That test cannot be written here, because the defect is not there.

What I would like: drop that hunk, or keep it and reclassify it honestly as defensive hardening rather than a fix. Either is fine. With that changed, this merges.

One optional piece of maintainer taste, entirely your call: a few of the substitutions make the code harder to read rather than easier — CBM_SZ_2 used as a realloc growth factor and as porcelain column offsets, CBM_ALLOC_ONE as an fread element size, SKIP_ONE as the argv NULL-terminator slot. Constants named for sizes, used as non-sizes, read worse than the literals did. Worth a second look, but I would not hold the PR for it.

One practical note: this touches watcher.c heavily (96 added / 74 removed), so it will create rebase work for anything else in that file. Worth landing reasonably promptly once updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. stability/performance Server crashes, OOM, hangs, high CPU/memory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants