fixed wrong usage of VirtualFree (win32)#891
Conversation
SecureAlloc should have never even compiled on win32. First, the API usage is wrong - VirtualFree requires a size parameter (0 in case of MEM_RELEASE). Second, the deallocator used a non existing variable name as reference. This change fixes the usage, but the change itself is untested.
📝 WalkthroughWalkthroughFour Windows VirtualFree signature fix
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/data_mgr/SecureAllocator.h`:
- Line 125: The two VirtualFree calls in SecureAllocator should stop casting the
pointer to const void because that still does not match the Win32 LPVOID
signature and causes Windows build issues. Update the release path in the
SecureAllocator logic so VirtualFree is called with a mutable void*/LPVOID
pointer instead, keeping the existing allocation/free flow intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a784059a-02a0-4e3c-bcc4-8207c09aef37
📒 Files selected for processing (1)
src/lib/data_mgr/SecureAllocator.h
| free(r); | ||
| #else | ||
| VirtualFree((const void*) r, MEM_RELEASE); | ||
| VirtualFree((const void*) r, 0, MEM_RELEASE); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '110,190p' src/lib/data_mgr/SecureAllocator.hRepository: softhsm/SoftHSMv2
Length of output: 1853
Remove the const cast from both VirtualFree calls. VirtualFree takes LPVOID, so VirtualFree((const void*) ...) still mismatches the Win32 signature and breaks Windows builds; pass void*/LPVOID instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/data_mgr/SecureAllocator.h` at line 125, The two VirtualFree calls in
SecureAllocator should stop casting the pointer to const void because that still
does not match the Win32 LPVOID signature and causes Windows build issues.
Update the release path in the SecureAllocator logic so VirtualFree is called
with a mutable void*/LPVOID pointer instead, keeping the existing
allocation/free flow intact.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/data_mgr/salloc.cpp`:
- Line 74: The Windows release path in salloc/sfree is still using an undefined
pointer name (`pre`) and mixed pointer variables, so make the pointer handling
consistent across the Windows branch of salloc and the cleanup path in sfree.
Update the allocation/lock/cleanup/return flow to use the same existing variable
(`ptr` or equivalent) everywhere, including the VirtualFree call and any
registry or unlock logic, so both _WIN32 paths compile cleanly.
- Line 119: The secure-path cleanup in `salloc.cpp` is mixing allocation APIs:
the `VirtualAlloc`/`VirtualFree` branch should not fall through to `free(ptr)`.
Update the cleanup logic around the `pre`/`ptr` handling so the `VirtualFree`
path only releases memory allocated by `VirtualAlloc`, and keep `free()`
exclusively for the malloc/valloc path; use the existing `salloc` deallocation
flow to separate the two cases cleanly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7932b398-1dde-436e-b840-8410c2ad3533
📒 Files selected for processing (1)
src/lib/data_mgr/salloc.cpp
| free(ptr); | ||
| #else | ||
| VirtualFree((const void*) pre, MEM_RELEASE); | ||
| VirtualFree((const void*) pre, 0, MEM_RELEASE); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
pre is still undefined on both Windows release paths.
These call sites still cannot compile on _WIN32: sfree only has ptr, and salloc's Windows branch currently mixes r, ptr, and now pre. The size fix is correct, but the pointer variable needs to be made consistent across allocation, locking, cleanup, registry, and return first.
Suggested fix
`#ifndef` _WIN32
void* ptr = (void*) valloc(len);
`#else`
- pointer r = (pointer) VirtualAlloc(NULL, n * sizeof(T), MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE);
+ void* ptr = VirtualAlloc(NULL, len, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE);
`#endif`
@@
`#ifndef` _WIN32
if (mlock((const void*) ptr, len) != 0)
`#else`
- if (VirtualLock((const void*) r, n * sizeof(T)) == 0)
+ if (VirtualLock((const void*) ptr, len) == 0)
`#endif`
@@
`#ifndef` _WIN32
free(ptr);
`#else`
- VirtualFree((const void*) pre, 0, MEM_RELEASE);
+ VirtualFree((const void*) ptr, 0, MEM_RELEASE);
`#endif`
@@
`#else`
- VirtualFree((const void*) pre, 0, MEM_RELEASE);
+ VirtualFree((const void*) ptr, 0, MEM_RELEASE);
`#endif`Also applies to: 119-119
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/data_mgr/salloc.cpp` at line 74, The Windows release path in
salloc/sfree is still using an undefined pointer name (`pre`) and mixed pointer
variables, so make the pointer handling consistent across the Windows branch of
salloc and the cleanup path in sfree. Update the allocation/lock/cleanup/return
flow to use the same existing variable (`ptr` or equivalent) everywhere,
including the VirtualFree call and any registry or unlock logic, so both _WIN32
paths compile cleanly.
| munlock((const void*) ptr, len); | ||
| #else | ||
| VirtualFree((const void*) pre, MEM_RELEASE); | ||
| VirtualFree((const void*) pre, 0, MEM_RELEASE); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '44,125p' src/lib/data_mgr/salloc.cpp
echo '---'
sed -n '160,180p' src/lib/data_mgr/SecureAllocator.hRepository: softhsm/SoftHSMv2
Length of output: 2211
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- src/lib/data_mgr/salloc.cpp ---\n'
cat -n src/lib/data_mgr/salloc.cpp | sed -n '1,220p'
printf '\n--- src/lib/data_mgr/SecureAllocator.h ---\n'
cat -n src/lib/data_mgr/SecureAllocator.h | sed -n '1,260p'Repository: softhsm/SoftHSMv2
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/lib/data_mgr/salloc.cpp ---'
cat -n src/lib/data_mgr/salloc.cpp | sed -n '1,220p'
echo
echo '--- src/lib/data_mgr/SecureAllocator.h ---'
cat -n src/lib/data_mgr/SecureAllocator.h | sed -n '1,260p'Repository: softhsm/SoftHSMv2
Length of output: 11328
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n '\bpre\b|\bVirtualFree\b|\bfree\(ptr\)' src/lib/data_mgr/salloc.cpp src/lib/data_mgr/SecureAllocator.hRepository: softhsm/SoftHSMv2
Length of output: 580
Fix the Windows secure-path cleanup in src/lib/data_mgr/salloc.cpp:115-124. The VirtualAlloc branch still calls VirtualFree(...) and then falls through to free(ptr), which mixes deallocation APIs for the same buffer; keep free() on the malloc/valloc path only.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/data_mgr/salloc.cpp` at line 119, The secure-path cleanup in
`salloc.cpp` is mixing allocation APIs: the `VirtualAlloc`/`VirtualFree` branch
should not fall through to `free(ptr)`. Update the cleanup logic around the
`pre`/`ptr` handling so the `VirtualFree` path only releases memory allocated by
`VirtualAlloc`, and keep `free()` exclusively for the malloc/valloc path; use
the existing `salloc` deallocation flow to separate the two cases cleanly.
1 participant
SecureAlloc should have never even compiled on win32. First, the API usage is wrong - VirtualFree requires a size parameter (0 in case of MEM_RELEASE). Second, the deallocator used a non existing variable name as reference.
This change fixes the usage, but the change itself is untested.
Summary by CodeRabbit
VirtualFreecall pattern, reducing the risk of cleanup issues.