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.
bukka
left a comment
bukka
left a comment
There was a problem hiding this comment.
The CodeRabbit comments seem correct, please fix them.
First, I don't think so. Which comment do you mean in particular? Second, your code obviously was never tested on Windows. It wasn't even compiled on Windows (otherwise the compiler would have complained). So you may want to delete it entirely... Without a meaningful test or any actual user, you are only carrying dead weight. |
|
The implementation in SecureAllocator.h comes from 1b80d83 and it wasn't changed significantly since then. Note that the text in the commit reads "tentative port to WIN32", where the tentative seemingly should be taken seriously. |
|
Ah, it actually just suggests that there are two additional problems which seems correct but the whole thing is actually dead code because SENSITIVE_NON_PAGED is always defined. We build SoftHSMv2 on Windows on CI as you can see but this is just dead code... |
|
Your change is correct per se but need to think if it's actually even worth keeping. |
|
Me personally, I would review the code needed (or not needed) - but that would be something I am leaving to you as I am not too familiar with the code base. If YOU put this on your to do list (or into a GH issue), then I would close this PR. |
bukka
left a comment
bukka
left a comment
There was a problem hiding this comment.
As this fixes part of the problem, I think it's fine to merge it. I need to make a note to see if this whole code is something we want to keep.
2 participants
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.