Skip to content

fixed wrong usage of VirtualFree (win32) #891

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bukka merged 2 commits into from
Jul 3, 2026
Merged

fixed wrong usage of VirtualFree (win32) #891

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a73f4b9
fixed wrong usage of VirtualFree (win32)
frankmorgner Jun 27, 2026
951b77a
fixed more usages of VirtualFree()
frankmorgner Jun 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/lib/data_mgr/SecureAllocator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ template<class T> class SecureAllocator
#ifndef _WIN32
free(r);
#else
VirtualFree((const void*) r, MEM_RELEASE);
VirtualFree((const void*) r, 0, MEM_RELEASE);

@coderabbitai coderabbitai Bot Jun 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

sed -n '110,190p' src/lib/data_mgr/SecureAllocator.h

Repository: 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.

#endif

return NULL;
Expand Down Expand Up @@ -173,7 +173,7 @@ template<class T> class SecureAllocator
#ifndef _WIN32
free(p);
#else
VirtualFree((const void*) r, MEM_RELEASE);
VirtualFree((const void*) p, 0, MEM_RELEASE);
#endif
#else
// Release the memory
Expand Down
4 changes: 2 additions & 2 deletions src/lib/data_mgr/salloc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ void* salloc(size_t len)
#ifndef _WIN32
free(ptr);
#else
VirtualFree((const void*) pre, MEM_RELEASE);
VirtualFree((const void*) pre, 0, MEM_RELEASE);

@coderabbitai coderabbitai Bot Jun 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

#endif

return NULL;
Expand Down Expand Up @@ -116,7 +116,7 @@ void sfree(void* ptr)
#ifndef _WIN32
munlock((const void*) ptr, len);
#else
VirtualFree((const void*) pre, MEM_RELEASE);
VirtualFree((const void*) pre, 0, MEM_RELEASE);

@coderabbitai coderabbitai Bot Jun 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.h

Repository: 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.h

Repository: 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.

#endif

#endif // SENSITIVE_NON_PAGED
Expand Down
Loading