Use CreateFileMapping\MapViewOfFile and UnmapViewOfFile\CloseHandle instead of VitualAllocEx and VirtualFreeEx` on Windows
#135
Conversation
This file contains 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
There was a problem hiding this comment.
Copilot reviewed 7 out of 10 changed files in this pull request and generated no comments.
Files not reviewed (3)
- docs/assets/hyperlight_arch.excalidraw: Language not supported
- src/hyperlight_host/src/hypervisor/hyperv_windows.rs: Evaluated as low risk
- src/hyperlight_host/src/hypervisor/hypervisor_handler.rs: Evaluated as low risk
Comments suppressed due to low confidence (3)
src/hyperlight_host/src/mem/shared_mem.rs:111
- The new behavior introduced in the Drop implementation of HostMapping should be covered by tests.
if let Err(e) = unsafe { UnmapViewOfFile(mem_mapped_address) } {
src/hyperlight_host/src/hypervisor/surrogate_process.rs:85
- There is a spelling mistake in the error message: 'drropping' should be 'dropping'.
tracing::error!("Failed to get surrogate process manager when drropping SurrogateProcess: {:?}", e);
src/hyperlight_host/src/hypervisor/surrogate_process.rs:60
- [nitpick] The variable 'mem_mapped_address' should follow Rust's naming convention and be renamed to 'memory_mapped_view_address'.
let mem_mapped_address = MEMORY_MAPPED_VIEW_ADDRESS {
simongdavies
force-pushed
the
use-mmap-files
branch
from
fdc5feb to
7e6ade8
Compare
Reflect the use of CreateFileMapping/MapViewOfFile/UnmapViewOfFile/CloseHandle instead of VirtualAlloc/VirtualFree Signed-off-by: Simon Davies <[email protected]>
…owsDriver Signed-off-by: Simon Davies <[email protected]>
When the driver is created a handle to the FileMapping for the memory for this sandbox. Uses that handle as an argument to the get_surrogate_process function on the SurrogateProcessManager Removes the code that copies the Sandbox memory to and from the surrogate process memory each time the VM is run. Signed-off-by: Simon Davies <[email protected]>
simongdavies
force-pushed
the
use-mmap-files
branch
from
7e6ade8 to
d28e54c
Compare
Replace use of VirtualAlloc/VirtualFree with CreateFileMapping/MapViewOFile/UnmapViewOfFile/CloseHandle to allow memry to be mapped into surrogate processes without any copying Signed-off-by: Simon Davies <[email protected]>
Signed-off-by: Simon Davies <[email protected]>
simongdavies
force-pushed
the
use-mmap-files
branch
from
d28e54c to
0c9a093
Compare
There was a problem hiding this comment.
Copilot reviewed 7 out of 10 changed files in this pull request and generated no comments.
Files not reviewed (3)
- docs/assets/hyperlight_arch.excalidraw: Language not supported
- src/hyperlight_host/src/hypervisor/hyperv_windows.rs: Evaluated as low risk
- src/hyperlight_host/src/mem/mgr.rs: Evaluated as low risk
Comments suppressed due to low confidence (4)
src/hyperlight_host/src/sandbox/uninitialized.rs:1162
- The error message should be clear and consistent. Consider rephrasing to: 'GuestBinary not found: 'some/path/that/does/not/exist': The system cannot find the path specified (os error)'.
matches!(sbox, Err(e) if e.to_string().contains("GuestBinary not found: 'some/path/that/does/not/exist': The system cannot find the path specified. (os error"))
src/hyperlight_host/src/hypervisor/surrogate_process.rs:66
- The error message should be updated to reflect the new function name
UnmapViewOfFile2.
Failed to free surrogate process resources (UnmapViewOfFile2 failed): {:?}
src/hyperlight_host/src/hypervisor/surrogate_process.rs:85
- The error message should be updated to reflect the new function name
return_surrogate_process.
Failed to get surrogate process manager when dropping SurrogateProcess: {:?}
src/hyperlight_host/src/hypervisor/surrogate_process.rs:64
- Ensure that the new behavior introduced by
UnmapViewOfFile2is adequately covered by tests.
if let Err(e) = unsafe { UnmapViewOfFile2(process_handle, memory_mapped_view_address, flags) } {
0e2c611 to
7f56eec
Compare
simongdavies
changed the title
CreateFileMapping\MapViewOFile and UnmapViewOfFile\CloseHandle instead of VitualAllocEx and VirtualFreeEx` on WindowsCreateFileMapping\MapViewOfFile and UnmapViewOfFile\CloseHandle instead of VitualAllocEx and VirtualFreeEx` on Windows
… feature flag Signed-off-by: Simon Davies <[email protected]>
Signed-off-by: Simon Davies <[email protected]>
…tualFreeEx Signed-off-by: Simon Davies <[email protected]>
…ualAllocEx Signed-off-by: Simon Davies <[email protected]>
simongdavies
force-pushed
the
use-mmap-files
branch
from
7f56eec to
c96bb27
Compare
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.
This PR replaces the use of
VirtualAllocExandVirtualFreeExwithCreateFileMapping,MapViewOfFile,UnmapViewOfFileandCloseHandleon Windows.This enables the mapping of memory into the surrogate processes without zero copying.
There are 9 commits as part of this PR each with its own description/explanation.