-
Notifications
You must be signed in to change notification settings - Fork 3
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.
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
mmap/sbrk/brk implementation and integration into glibc/wasmtime #74
Conversation
There is a minor issue that I just noticed. Since mmap is now find memory space from highest address, so the first mmap will usually return an address that is close to 4294967296. In wasmtime, previously I was using Besides, the test added by #67 (chain_thread.c), which was not able to run previously because it uses mmap somewhere, is verified to be able to run successfully after mmap has been integrated. Though I did some temporary tweak to make mmap is returning address from 2^31 so that it will not overflow i32. But this should at least tell us that things are pretty much there and we only have a minor issue (replace i32 to u32) to fix |
@@ -179,7 +179,7 @@ impl Cage { | |||
sigset: newsigset, | |||
main_threadid: interface::RustAtomicU64::new(0), | |||
interval_timer: interface::IntervalTimer::new(child_cageid), | |||
vmmap: interface::RustLock::new(Vmmap::new()), // Initialize empty virtual memory map for new process |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we sure that this deep copies on clone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually cannot be 100% sure about this. clone
method should be designed to deep-copying itself by convention from what I've read, but if the developer (in our case, developer of nodit library or its sub-dependent) is breaking the convention and doing some hacky things here, it would probably be hard for us to know. The documentation for the NoditMap only says it returns a copy of the value without mentioning shallow or deep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something worth just adding a test for @ChinmayShringi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good. I don't have any specific requests but would like to see a few more tests.
- more complicated situations with mmap/munmap and fork
- Tests that use malloc instead of sbrk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! This is a solid implementation! Overall logic makes sense to me.
update:
There is already a simple malloc test under |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reviewed the implementations, and everything now looks good to me.
This PR finalized the implementation of mmap/munmap/sbrk/brk.
On rawposix side, the changes are mainly
On wasmtime side, the changes are mainly about bypassing part of wasmtime's internal memory management logic, including:
On glibc side, the changes are mainly about
lind_syscall.c
to special handle mmap result. Since mmap is returning a 32-bit address, which might overflowint
type and become negative, we need to distinguish the valid memory address from errno as errno are also returned as negative number from rawposix. This is achieved by checking the range of the negative value since mmap result is always multiple of pages (4096), while errno has a maximum value that is far less than 4096.