Open
Conversation
…row to platform.h Centralize heap memory management into four safe wrappers alongside existing safe_realloc: - safe_free(ptr): frees and NULLs any pointer to prevent double-free - safe_str_free(&str): frees const char* with NULL-out (replaces free((void*)str)) - safe_buf_free(buf, &count): frees array and zeros its count - safe_grow(arr, n, cap, factor): one-line capacity-doubling realloc Applied across cypher.c, store.c, mcp.c, and pass_githistory.c, eliminating ~60 lines of repetitive free/grow boilerplate.
Owner
Author
|
@copilot resolve the merge conflicts in this pull request |
Review Summary by QodoAdd safe memory wrapper functions to platform.h
WalkthroughsDescription• Add four safe memory wrapper functions to centralize heap management • safe_free() prevents double-free by NULLing pointers after freeing • safe_str_free() safely frees const char* without casting • safe_grow() macro simplifies dynamic array capacity doubling • Replace ~60 lines of repetitive free/realloc boilerplate across codebase Diagramflowchart LR
A["platform.h<br/>New Wrappers"] -->|safe_free| B["cypher.c"]
A -->|safe_str_free| B
A -->|safe_grow| B
A -->|safe_buf_free| C["store.c"]
A -->|safe_grow| C
A -->|safe_grow| D["mcp.c"]
A -->|safe_grow| E["pass_githistory.c"]
B -->|Eliminate<br/>boilerplate| F["Cleaner<br/>Code"]
C -->|Eliminate<br/>boilerplate| F
D -->|Eliminate<br/>boilerplate| F
E -->|Eliminate<br/>boilerplate| F
File Changes1. src/foundation/platform.h
|
Code Review by Qodo
|
…ts keeping safe_grow wrappers Co-authored-by: map588 <122550757+map588@users.noreply.github.com>
Resolved the merge conflicts in commit |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+34
to
+42
| /* Safe free: frees and NULLs a pointer to prevent double-free / use-after-free. | ||
| * Accepts void** so it works with any pointer type via the macro. */ | ||
| static inline void safe_free_impl(void **pp) { | ||
| if (pp && *pp) { | ||
| free(*pp); | ||
| *pp = NULL; | ||
| } | ||
| } | ||
| #define safe_free(ptr) safe_free_impl((void **)(void *)&(ptr)) |
There was a problem hiding this comment.
1. Safe_free void** type-pun 🐞 Bug ≡ Correctness
safe_free casts the address of an arbitrary typed pointer (e.g., cbm_store_t*) to void** and writes through it, which is undefined behavior under C strict-aliasing/type rules and can miscompile under optimization. This macro is now used broadly (e.g., store_open_internal uses safe_free(s)), so any UB here becomes a systemic reliability/correctness risk.
Agent Prompt
### Issue description
`safe_free` currently type-puns arbitrary pointer lvalues through a `void**` and writes via that pointer. This is undefined behavior under C strict aliasing and can miscompile under optimization.
### Issue Context
The macro is used widely after this refactor (e.g., `safe_free(s)` in store open failure paths), so any UB in the macro affects many components.
### Fix Focus Areas
- src/foundation/platform.h[34-42]
### Recommended fix
Replace the `safe_free_impl(void **pp)` + `#define safe_free(ptr) ...void** cast...` approach with a macro that frees and NULLs the *original typed lvalue* without reinterpreting it as `void**`.
One portable C approach (requires passing an assignable pointer lvalue) is:
```c
#define safe_free(p) do {
if ((p) != NULL) {
free((void *)(p));
(p) = NULL;
}
} while (0)
```
If you need single-evaluation semantics for expressions, consider documenting that `safe_free()` must be passed a simple lvalue (no side effects), or (if your toolchain allows) use a compiler extension-based temporary to ensure single evaluation while keeping correct typing.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Pull request overview
Refactors memory management across the C codebase by introducing “safe” free/grow helpers in platform.h and updating store/pipeline/cypher/mcp code to use them, aiming to reduce double-free/use-after-free risk and standardize dynamic array growth patterns.
Changes:
- Add
safe_free,safe_str_free,safe_buf_free, andsafe_growhelpers tosrc/foundation/platform.h. - Replace many
free(...)calls with safe free helpers to NULL-out pointers after freeing. - Replace repeated manual “grow + realloc” blocks with
safe_grow(...)in several hot paths.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/store/store.c |
Replaces manual frees/realloc growth with safe_str_free/safe_free and safe_grow across store operations and cleanup helpers. |
src/pipeline/pass_githistory.c |
Uses safe_grow for commit array growth in git history parsing. |
src/mcp/mcp.c |
Uses safe_grow when collecting grep matches. |
src/foundation/platform.h |
Introduces new safe memory helpers/macros used by the refactor. |
src/cypher/cypher.c |
Replaces many frees with safe wrappers and uses safe_grow for parser/executor dynamic arrays. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+34
to
+64
| /* Safe free: frees and NULLs a pointer to prevent double-free / use-after-free. | ||
| * Accepts void** so it works with any pointer type via the macro. */ | ||
| static inline void safe_free_impl(void **pp) { | ||
| if (pp && *pp) { | ||
| free(*pp); | ||
| *pp = NULL; | ||
| } | ||
| } | ||
| #define safe_free(ptr) safe_free_impl((void **)(void *)&(ptr)) | ||
|
|
||
| /* Safe string free: frees a const char* and NULLs it. | ||
| * Casts away const so callers don't need the (void*) dance. */ | ||
| static inline void safe_str_free(const char **sp) { | ||
| if (sp && *sp) { | ||
| free((void *)*sp); | ||
| *sp = NULL; | ||
| } | ||
| } | ||
|
|
||
| /* Safe buffer free: frees a heap array and zeros its element count. | ||
| * Use for dynamic arrays paired with a size_t count. */ | ||
| static inline void safe_buf_free_impl(void **buf, size_t *count) { | ||
| if (buf && *buf) { | ||
| free(*buf); | ||
| *buf = NULL; | ||
| } | ||
| if (count) { | ||
| *count = 0; | ||
| } | ||
| } | ||
| #define safe_buf_free(buf, countp) safe_buf_free_impl((void **)(void *)&(buf), (countp)) |
There was a problem hiding this comment.
safe_free/safe_buf_free rely on casting the address of an arbitrary pointer object to void ** and then writing through it. This is undefined behavior under C strict-aliasing rules (accessing a T* object via a void* lvalue) and can miscompile at -O2 with strict-aliasing enabled. Prefer a macro that frees the expression and assigns it to NULL directly (requiring an lvalue), or use _Generic to dispatch to type-correct helpers without void ** type punning.
Comment on lines
+66
to
+71
| /* Safe grow: doubles capacity and reallocs when count reaches cap. | ||
| * Usage: safe_grow(arr, count, cap, growth_factor) | ||
| * Evaluates to the new arr (NULL on OOM — old memory freed by safe_realloc). */ | ||
| #define safe_grow(arr, n, cap, factor) do { \ | ||
| if ((size_t)(n) >= (size_t)(cap)) { \ | ||
| (cap) *= (factor); \ |
There was a problem hiding this comment.
safe_grow breaks if cap is 0: cap *= factor keeps it at 0, and the safe_realloc(..., cap * sizeof(*arr)) path ends up allocating 1 byte (due to safe_realloc's size==0 handling), which is not enough for arr[0]. Either enforce/ASSERT that cap is nonzero before calling, or have safe_grow treat cap==0 as a minimum initial capacity (e.g., set it to 1 or a small default before multiplying).
Suggested change
| /* Safe grow: doubles capacity and reallocs when count reaches cap. | |
| * Usage: safe_grow(arr, count, cap, growth_factor) | |
| * Evaluates to the new arr (NULL on OOM — old memory freed by safe_realloc). */ | |
| #define safe_grow(arr, n, cap, factor) do { \ | |
| if ((size_t)(n) >= (size_t)(cap)) { \ | |
| (cap) *= (factor); \ | |
| /* Safe grow: increases capacity and reallocs when count reaches cap. | |
| * If cap is 0, initializes it to 1 so the allocation is large enough | |
| * for at least one element before applying normal growth on later calls. | |
| * Usage: safe_grow(arr, count, cap, growth_factor) | |
| * Evaluates to the new arr (NULL on OOM — old memory freed by safe_realloc). */ | |
| #define safe_grow(arr, n, cap, factor) do { \ | |
| if ((size_t)(n) >= (size_t)(cap)) { \ | |
| if ((size_t)(cap) == 0) { \ | |
| (cap) = 1; \ | |
| } else { \ | |
| (cap) *= (factor); \ | |
| } \ |
Comment on lines
+66
to
+68
| /* Safe grow: doubles capacity and reallocs when count reaches cap. | ||
| * Usage: safe_grow(arr, count, cap, growth_factor) | ||
| * Evaluates to the new arr (NULL on OOM — old memory freed by safe_realloc). */ |
There was a problem hiding this comment.
The safe_grow doc comment is misleading: it says it “doubles capacity” and “evaluates to the new arr”, but the macro is a statement and the growth factor is caller-provided (not necessarily 2). Updating the comment to match the actual semantics will reduce misuse by future callers.
Suggested change
| /* Safe grow: doubles capacity and reallocs when count reaches cap. | |
| * Usage: safe_grow(arr, count, cap, growth_factor) | |
| * Evaluates to the new arr (NULL on OOM — old memory freed by safe_realloc). */ | |
| /* Safe grow: grows capacity and reallocs when count reaches cap. | |
| * Usage: safe_grow(arr, count, cap, growth_factor) | |
| * Statement macro: multiplies cap by growth_factor, then updates arr in place | |
| * via safe_realloc (arr becomes NULL on OOM, and old memory is freed). */ |
Comment on lines
+34
to
+74
| /* Safe free: frees and NULLs a pointer to prevent double-free / use-after-free. | ||
| * Accepts void** so it works with any pointer type via the macro. */ | ||
| static inline void safe_free_impl(void **pp) { | ||
| if (pp && *pp) { | ||
| free(*pp); | ||
| *pp = NULL; | ||
| } | ||
| } | ||
| #define safe_free(ptr) safe_free_impl((void **)(void *)&(ptr)) | ||
|
|
||
| /* Safe string free: frees a const char* and NULLs it. | ||
| * Casts away const so callers don't need the (void*) dance. */ | ||
| static inline void safe_str_free(const char **sp) { | ||
| if (sp && *sp) { | ||
| free((void *)*sp); | ||
| *sp = NULL; | ||
| } | ||
| } | ||
|
|
||
| /* Safe buffer free: frees a heap array and zeros its element count. | ||
| * Use for dynamic arrays paired with a size_t count. */ | ||
| static inline void safe_buf_free_impl(void **buf, size_t *count) { | ||
| if (buf && *buf) { | ||
| free(*buf); | ||
| *buf = NULL; | ||
| } | ||
| if (count) { | ||
| *count = 0; | ||
| } | ||
| } | ||
| #define safe_buf_free(buf, countp) safe_buf_free_impl((void **)(void *)&(buf), (countp)) | ||
|
|
||
| /* Safe grow: doubles capacity and reallocs when count reaches cap. | ||
| * Usage: safe_grow(arr, count, cap, growth_factor) | ||
| * Evaluates to the new arr (NULL on OOM — old memory freed by safe_realloc). */ | ||
| #define safe_grow(arr, n, cap, factor) do { \ | ||
| if ((size_t)(n) >= (size_t)(cap)) { \ | ||
| (cap) *= (factor); \ | ||
| (arr) = safe_realloc((arr), (size_t)(cap) * sizeof(*(arr))); \ | ||
| } \ | ||
| } while (0) |
There was a problem hiding this comment.
platform.h is covered by tests/test_platform.c, but these newly introduced safe-memory helpers (safe_free, safe_str_free, safe_grow, safe_buf_free) don’t appear to have any direct tests. Adding a small test that verifies “free + NULL” behavior and that safe_grow preserves contents/capacity expectations would help prevent regressions.
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
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.
No description provided.