Conversation
Add nmSysGetSize(). Add a warning when using nmStats() with the necessary stat tracking disabled. Update errors to print to stderr instead of stdout. Abstract registered block name printing into a public helper function.
Greptile SummaryThis PR refactors the NewMalloc memory management subsystem for improved readability, adding extensive comments, consistent formatting, and modernized C idioms ( Key issues found:
Confidence Score: 1/5
|
| Filename | Overview |
|---|---|
| centrallix-lib/src/newmalloc.c | Major refactoring of the memory management module. Contains several issues: broken BUFFER_OVERFLOW_CHECKING macros due to MemStruct size change, undeclared variable in nmSysRealloc, memory leak in nmSysMalloc, and format specifier mismatches. |
| centrallix-lib/include/newmalloc.h | Header updated with int-to-size_t type changes for function signatures, new nmSysGetSize() and nmPrintNames() declarations, and MAX_SIZE changed to unsigned long literal. Changes are largely correct. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph "nmMalloc(size)"
A[nmMalloc called] --> B{MIN_SIZE ≤ size ≤ MAX_SIZE?}
B -- Yes --> C{Cache hit?}
C -- Yes --> D[Pop from lists cache]
C -- No --> E[nmDebugMalloc]
B -- No --> F[nmDebugMalloc]
D --> G[Set OVERLAY Magic]
E --> G
F --> G
end
subgraph "nmFree(ptr, size)"
H[nmFree called] --> I{Cacheable size?}
I -- Yes, NO_BLK_CACHE off --> J[Add to lists cache, set ptr=NULL]
I -- No --> K{ptr != NULL?}
J --> K
K -- Yes --> L[nmDebugFree]
K -- No --> M[Done]
L --> M
end
subgraph "nmSysMalloc(size) — NM_USE_SYSMALLOC"
N[nmSysMalloc called] --> O["nmDebugMalloc(sizeof(uint) + size)"]
O --> P{size > UINT_MAX?}
P -- Yes --> Q["⚠️ return NULL (LEAK: ptr not freed)"]
P -- No --> R[Store size as uint prefix]
R --> S[Return ptr + sizeof uint]
end
subgraph "nmSysRealloc — NM_USE_SYSMALLOC"
T[nmSysRealloc called] --> U{ptr == NULL?}
U -- Yes --> V[nmSysMalloc]
U -- No --> W{Size unchanged?}
W -- Yes --> X[Return ptr]
W -- No --> Y["nmDebugRealloc(buffer, new_size + uint)"]
Y --> Z[Update size prefix]
Z --> AA["⚠️ SIZED_BLK_COUNTING uses undeclared 'size'"]
end
Last reviewed commit: c40fc8d
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Fix buffer overflow in nmDebugRealloc(). Fix buffer overflow in nmRegister(). Fix incorrect format specifiers for size types. Fix pointer arithmetic on void*. Reduce padding in RegisteredBlockType struct. Add doc comment for RegisteredBlockType struct. Fix typos in comments. Improve detail and clarity of some comments.
|
@greptileai This new commit should resolve your comments. |
Greptile SummaryThis PR refactors the NewMalloc memory management subsystem for improved readability, adding extensive comments, consistent formatting, and cleaner control flow. It also introduces two new functions:
Confidence Score: 2/5
|
| Filename | Overview |
|---|---|
| centrallix-lib/include/newmalloc.h | Header updated with formatting improvements, new nmPrintNames() and nmSysGetSize() declarations, and minor comment/spacing fixes. Changes are straightforward with no logical issues. |
| centrallix-lib/src/newmalloc.c | Major refactor with improved comments, code structure, new nmPrintNames() and nmSysGetSize() functions, and strtcpy usage. Has a build-breaking missing include (strtcpy.h), a type mismatch in nmSysGetSize, and a potential signed overflow in nmDeltas. |
Flowchart
flowchart TD
A[nmSysMalloc] -->|"allocates size+sizeof(int)"| B[nmDebugMalloc]
C[nmSysFree] -->|"frees ptr-sizeof(int)"| D[nmDebugFree]
E[nmSysRealloc] -->|"same size? early return"| F{size == new_size?}
F -->|Yes| G[return ptr unchanged]
F -->|No| H[nmDebugRealloc]
I["nmSysGetSize (NEW)"] -->|"reads *(ptr - sizeof(int))"| J[return stored size]
K[nmSysStrdup] -->|"strlen + nmSysMalloc + memcpy"| A
B --> L{BUFFER_OVERFLOW_CHECKING?}
L -->|Yes| M["malloc with magic guards"]
L -->|No| N["plain malloc"]
D --> O{BUFFER_OVERFLOW_CHECKING?}
O -->|Yes| P["check magic + unlink + free"]
O -->|No| Q["plain free"]
Last reviewed commit: 6940889
Update allocation function to use size_t (instead of int), matching C stdlib functions. Update statistics variables to use unsigned long long instead of int. Add ignored errors when tracked statistics indicate something has gone wrong. Improve formatting for printing of statistics. Improve formatting of printed errors to start with ERROR (making them more obvious). Fix build warning due to a missing include statement. Clean up some comments.
|
@greptileai Sorry for that mess! This type overhaul should resolve all of your previous concerns. |
Additional Comments (1)
The On a 64-bit system:
This means These macros should be updated to use |
|
No, there's no code outside the newmalloc module that depends on the int at the start of the nmSys block or on it being four bytes long. All access to nmSys blocks goes through the public API functions ( You're free to change the size or type of that field as long as you update all internal references within |
Update nmSys blocks to use size_t to store size data. Update MemStruct to match new types. Update RegisteredBlockType to match new types. Update and add comments. Fix incorrect format specifiers.
|
@greptileai This new commit should resolve your previous concerns. Do you have any other comments? |
|
The code looks good! All my previous concerns have been addressed:
The refactoring improves code readability significantly while maintaining correctness. Nice work! |
2 participants
Improves the developer experience when using the NewMalloc system by using a cleaner code structure with comments to enable faster speed-reading. This new code intends to make it easy for readers to quickly skim code without getting lost in irrelevant logic for features that might not even be enabled.
This branch also introduces some improvements in the final commit, the largest of which is adding
nmSysGetSize(), which I anticipate will be very useful in some scenarios. (I already have at least one place I want to use it.)I recommend that Greg take ownership of the first commit so that we can avoid having me get git blamed for reformatting most of the file with very minimal changes to any of the logic.