Skip to content

Commit

Permalink
Consider zeroed bytes in sockaddr_in for HorizonOS
Browse files Browse the repository at this point in the history
  • Loading branch information
@Meziu
Meziu committed Aug 21, 2024
1 parent 4d7c095 commit 96b0f4c
Showing 1 changed file with 6 additions and 0 deletions.
6 changes: 6 additions & 0 deletions library/std/src/sys_common/net.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,13 @@ where
pub fn sockaddr_to_addr(storage: &c::sockaddr_storage, len: usize) -> io::Result<SocketAddr> {
match storage.ss_family as c_int {
c::AF_INET => {
#[cfg(not(target_os = "horizon"))]
assert!(len >= mem::size_of::<c::sockaddr_in>());

// Under HorizonOS, sockaddr_in has 8 zeroed trailing bytes that do not get written.
#[cfg(target_os = "horizon")]
assert!(len >= (mem::size_of::<c::sockaddr_in>() - mem::size_of::<[c::c_char; 8]>()));

This comment has been minimized.

Sign in to view

Copy link
@ian-h-chamberlain

ian-h-chamberlain Aug 21, 2024

@Meziu it seems like other targets also define sin_zero in libc e.g. https://github.com/rust-lang/libc/blob/72cb7aaf50bf70b5d360b1a64b0d52d3ed3fbf15/src/unix/linux_like/mod.rs#L45

I wonder why they don't need to offset for padding the same way as this... is it possible that our definition of sockaddr_storage is actually the problematic one?

This comment has been minimized.

Sign in to view

Copy link
@Meziu

Meziu Aug 21, 2024

Author Owner

You mean devkitARM's definition is wrong, because the one in libc is 1:1 what they defined. Though yes, it's very likely some weird things are going on with that sockaddr_storage, I remember having issues with it all the way back when I started working on this project.

This comment has been minimized.

Sign in to view

Copy link
@ian-h-chamberlain

ian-h-chamberlain Aug 21, 2024 •
edited
Loading

Looking at some of the implementations like https://github.com/devkitPro/libctru/blob/master/libctru/source/services/soc/soc_getpeername.c#L54

I think the issue might be that the return value of addrlen is not being set large enough in every case... manpages describe this:

   The addrlen argument is a value-result argument: the caller must
   initialize it to contain the size (in bytes) of the structure
   pointed to by addr; on return it will contain the actual size of
   the peer address.

   The returned address is truncated if the buffer provided is too
   small; in this case, addrlen will return a value greater than was
   supplied to the call.

It looks like most of the soc implementations just return the actual length of the result, instead of including padding there (even if the padding is actually zeroed out as expected). Looking at the socket example it's probably easy to check if we just print the returned length e.g. https://github.com/devkitPro/3ds-examples/blob/master/network/sockets/source/sockets.c#L120

I probably don't have time to dig in further right now, but assuming libctru is the issue, we could either try to upstream implementation changes to zero out and return the full struct length, or otherwise maybe try a more one-size-fits-all approach here, maybe like

const SIZE_OF_ADDR = size_of::<c::sockaddr_in>() -
	size_of::<c::sockaddr_in::sin_zero>();

assert!(len >= SIZE_OF_ADDR);

I guess that might be problematic if any targets leave sin_zero undefined, but maybe those could be fixed case-by-case, idk.

This comment has been minimized.

Sign in to view

Copy link
@Meziu

Meziu Aug 21, 2024

Author Owner

I probably don't have time to dig in further right now, but assuming libctru is the issue, we could either try to upstream implementation changes to zero out and return the full struct length

Might be our best shot, but I'll have to find examples of Posix-complying targets that behave that way to support the case (or else there must be something amiss).

or otherwise maybe try a more one-size-fits-all approach here, maybe like

const SIZE_OF_ADDR = size_of::<c::sockaddr_in>() -
	size_of::<c::sockaddr_in::sin_zero>();

assert!(len >= SIZE_OF_ADDR);

I guess that might be problematic if any targets leave sin_zero undefined, but maybe those could be fixed case-by-case, idk.

I don't think any target other than ours have this problem, so I believe we should probably write our own edge-cases around the std common implementations, rather than trying to reinvent the wheel.

This comment has been minimized.

Sign in to view

Copy link
@Meziu

Meziu Sep 1, 2024

Author Owner

@ian-h-chamberlain Did your testing suggestion, and you were right. Under linux (which has this sockaddr_in definition) there are 8 bytes of "data" and 8 bytes of zeroed padding. However, after testing with some simple localhost binding, the resulting address length returned by accept always accounted for the total 16 bytes (likely because that's the size of the sockaddr struct, which is the "alias" for all of these *_in variants).

Under libctru, the result was different, which returns having written only 8 of the 16 "total" bytes (which is the cause of our issue).

The cause seems to be here, where the function returns the data given by the IPC call.

It is interesting, however, how these soc calls seem to be carefully written to be POSIX compliant. I wonder whether writing 8 isn't actually the more correct protocol...

Ok(SocketAddr::V4(FromInner::from_inner(unsafe {
*(storage as *const _ as *const c::sockaddr_in)
})))
Expand Down

0 comments on commit 96b0f4c

Please sign in to comment.