BUG:sockaddr cannot transmit parameters normally #56
Conversation
…an len. If the condition is incorrectly written, the parameters cannot be transmitted normally.
|
Good job! It's a typo but we didn't find it when adding it to this repository since it was not used by any function at that time. By the way another issue is that this interface is actually hard to use. For example in our code we still have to add a wrapper function like this: fn to_socketaddr(addr: UserConstPtr<u8>, addrlen: socklen_t) -> LinuxResult<SocketAddr> {
let addr = addr.get_as_slice(addrlen as usize)?;
let addr = unsafe { SockAddr::read(addr.as_ptr().cast(), addrlen)? };
SocketAddr::try_from(addr)
}
fn to_sockaddr(addr: SocketAddr, ptr: UserPtr<u8>, addrlen: UserPtr<socklen_t>) -> LinuxResult {
let addr = SockAddr::from(addr);
*addrlen.get_as_mut()? = addr.addr_len();
let bytes = addr.bytes();
ptr.get_as_mut_slice(bytes.len())?.copy_from_slice(bytes);
Ok(())
}I believe this is a design flaw since I just copy-and-pasted it from rustix without considering the difference between user code and kernel code. Given your clear understanding of this, would you be interested in helping us redesign this component? I'm thinking of using similar strategy like d092bfe, that is, removing trait SocketAddrExt: Sized {
fn read_sockaddr(addr: UserConstPtr<u8>, addrlen: socklen_t) -> LinuxResult<Self>;
fn write_sockaddr(&self, addr: UserPtr<u8>) -> LinuxResult<socklen_t>;
}This is just my initial thinking - don’t feel constrained by this proposal! Either way, excited to hear your thoughts! |
…nverted to SocketAddr. Use tcp_client to call sys_connect test and pass normally
|
I've created a trait to convert sockaddr to SocketAddr (extending SocketAddrExt), and confirmed its functionality with tcp_client and sys_connect. Please let me know if any further improvements or refinements are desired. |
|
Please pull the main branch and fix the CICD. |
* 'main' of https://github.com/oscomp/starry-next: [init] Add commit hash for arceos backbone Upgrade toolchain to nightly-2025-05-20 [ci] use setup-musl & setup-qemu action from Marketplace
…to write to dst_addr, and test successfully in sys_getsockname
|
I have modified the comments, removed socketaddr.rs, modified write_to_user to write to dst_addr, and tested success in sys_getsockname. |
|
LGTM. But please allow me to nitpick a bit. |
api/src/socket.rs
Outdated
| return Err(LinuxError::EINVAL); | ||
| } | ||
|
|
||
| let storage = copy_sockaddr_from_user(addr, addrlen)?; |
There was a problem hiding this comment.
copy_sockaddr_from_user will be called again later in <SocketAddrV4 as SocketAddrExt>::read_from_user/<SocketAddrV6 as SocketAddrExt>::read_from_user, so there will be a duplicate.
Question: Do we actually need a MaybeUninit<sockaddr> storage?
There was a problem hiding this comment.
When converting SocketAddr, UserConstPtr is directly read to judge the family. For SocketAddrV4 and SocketAddrV6, I think it seems safer to copy sockaddr to kernel space, but the functions after executing read_from_user will not use this space (created by copy_sockaddr_from_user). I don't know if this copy_sockaddr_from_user can be deleted and directly read from UserConstPtr. There seems to be no problem in my single test case, but I haven't tested more complex test cases.
before:
In impl SocketAddrExt for SocketAddrV4 {
fn read_from_user ······
······
let storage = copy_sockaddr_from_user(addr, addrlen)?;
let ptr = storage.as_ptr() as *const sockaddr_in;after:
In impl SocketAddrExt for SocketAddrV4 {
fn read_from_user ······
······
let src_addr: &'static sockaddr = addr.get_as_ref()?;
let src_ptr: *const sockaddr_in = src_addr as *const sockaddr as *const sockaddr_in;
api/src/socket.rs
Outdated
| SocketAddr::V4(_) => size_of::<sockaddr_in>() as socklen_t, | ||
| SocketAddr::V6(_) => size_of::<sockaddr_in6>() as socklen_t, |
…y, and implement forwarding for family and addr_len methods
api/src/socket.rs
Outdated
| if addr.is_null() { | ||
| return Err(LinuxError::EINVAL); | ||
| } | ||
| let dst_addr: &'static mut sockaddr = addr.get_as_mut()?; |
There was a problem hiding this comment.
Can we remove this type comment?
let dst_addr = addr.get_as_mut()?;
|
Please fix the CICD |
Key error:
size_of::<__kernel_sa_family_t>()should be greater thanlen. If the condition is incorrectly written, the parameters cannot be transmitted normally.