-
Notifications
You must be signed in to change notification settings - Fork 57
refactor: remove UserPtr and UserConstPtr usages #107
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
base: main
Are you sure you want to change the base?
Conversation
|
Good job 👍 |
AsakuraMizu
left a comment
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.
Sorry, there were too many files, and I haven't had time to look at a few of them myself.
| let mut len = addrlen.vm_read()?; | ||
| remote_addr.write_to_user(addr, &mut len)?; | ||
| addrlen.vm_write(len)?; |
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.
Don't do this. I already suggested you to change the signature of SocketAddrExt::write_to_user.
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.
Pull request overview
This pull request refactors the codebase to remove custom UserPtr and UserConstPtr types in favor of a unified memory access interface from starry_vm. The changes introduce VmPtr/VmMutPtr primitives and helper functions like vm_read_slice, vm_write_slice, and vm_load_until_nul for safer user-space memory operations.
Key Changes:
- Replaced all
UserPtr/UserConstPtrusages with raw pointers andstarry_vmprimitives - Updated system call signatures to accept raw pointers instead of custom wrapper types
- Refactored
CMsgBuilderto work with raw pointers and returnResultfrom constructor - Simplified syscall dispatcher by replacing
.into()calls withas _casts
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| api/src/mm.rs | Removed UserPtr/UserConstPtr implementations; kept VmBytes/VmBytesMut wrappers and vm_load_string helper |
| api/src/vfs/dev/event.rs | Converted event device ioctls to use vm_write_slice and allocate temporary buffers |
| api/src/syscall/net/socket.rs | Updated socket syscalls to use raw pointers with VmPtr/VmMutPtr traits |
| api/src/syscall/net/opt.rs | Refactored getsockopt/setsockopt to use vm_read/vm_write with temporary variables |
| api/src/syscall/net/name.rs | Simplified getsockname/getpeername to use raw pointers |
| api/src/syscall/net/io.rs | Updated sendmsg/recvmsg to use vm_read_uninit and addr_of_mut for field access |
| api/src/syscall/net/cmsg.rs | Refactored CMsgBuilder to use raw pointers and made constructor fallible |
| api/src/syscall/mod.rs | Replaced .into() conversions with as _ casts throughout syscall dispatcher |
| api/src/syscall/ipc/shm.rs | Updated shmctl to use vm_read/vm_write with nullable pointer handling |
| api/src/syscall/io_mpx/select.rs | Converted select/pselect6 to read/write local copies and use nullable pattern |
| api/src/syscall/io_mpx/poll.rs | Updated poll/ppoll to use local Vec buffers with vm_read/vm_write |
| api/src/syscall/io_mpx/epoll.rs | Converted epoll functions to use temporary buffers and vm_write |
| api/src/syscall/fs/memfd.rs | Changed memfd_create signature to use raw pointer |
| api/src/syscall/fs/io.rs | Updated truncate to use vm_load_string |
| api/src/syscall/fs/fd_ops.rs | Modified fcntl to use vm_read_uninit/vm_write for flock64 |
| api/src/socket.rs | Updated SocketAddrExt trait to use raw pointers; refactored read/write methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| addr.cast::<u8>() | ||
| .get_as_mut_slice(len)? | ||
| .copy_from_slice(&data[..len]); | ||
| let _ = vm_write_slice(addr.cast::<u8>(), &data[..len]); |
Copilot
AI
Jan 8, 2026
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.
The function vm_write_slice can fail and return an error, but the result is being ignored here with let _. This could silently fail to write the socket address data to user space, which would be a serious bug. The error should be propagated or handled appropriately.
| let _ = vm_write_slice(addr.cast::<u8>(), &data[..len]); | |
| vm_write_slice(addr.cast::<u8>(), &data[..len])?; |
| for (i, it) in local.iter().enumerate() { | ||
| unsafe { fds.add(i).vm_write(*it)? }; | ||
| } | ||
| // do_poll(fds, timeout, nullable!(sigmask.get_as_ref())?.copied()) |
Copilot
AI
Jan 8, 2026
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.
The commented-out line at the end suggests there may be incomplete refactoring. Either this comment should be removed if the refactoring is complete, or the old code should be restored if the new implementation is not yet validated.
| // do_poll(fds, timeout, nullable!(sigmask.get_as_ref())?.copied()) |
#39
Description
UserPtr/UserConstPtrand their supporting memory validation functions to implement a unified user-space memory access interface.VmPtr/VmMutPtr,vm_read_slice,vm_write_slice, andvm_load_until_nulfromstarry_vm, covering scenarios such as read/write operations and string loading.axioframework'sRead/WriteandIoBuf/IoBufMutmodels, avoiding inconsistencies caused by custom pointer types.