Skip to content
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

Parse unix address from sockaddr without length #2564

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pronebird
Copy link
Contributor

What does this PR do

I noticed that feeding *const sockaddr_un into SockaddrStorage without length returns None. I had impression that it should actually work.

However extracting sa_len manually and feeding it into SockaddrStrorage works as expected and I am able to cast it to UnixAddr later on by calling SockaddrStorage::as_unix_addr().

 SockaddrStorage::from_raw(sa_ptr, Some((*sa_ptr).sa_len.into()))

Checklist:

  • I have read CONTRIBUTING.md
  • I have written necessary tests and rustdoc comments
  • A change log has been added if this PR modifies nix's API

@asomers
Copy link
Member

asomers commented Dec 17, 2024

What new behavior is enabled, exactly? A test would be helpful to understand your aim. Also, you need a changelog entry.

@pronebird
Copy link
Contributor Author

pronebird commented Dec 17, 2024

What new behavior is enabled, exactly? A test would be helpful to understand your aim. Also, you need a changelog entry.

That SockaddrStorage can actually parse a pointer to sockaddr_un when the length is not provided. Currently this doesn’t work:

SockaddrStorage::from_raw(sockaddr_un_ptr, None)

I think that should work since the length is actually held inside of the pointer and can be obtained automatically.

I can add a test and change entry.

@asomers
Copy link
Member

asomers commented Dec 17, 2024

There's a reason that it doesn't work. It's deliberate. The purpose of the length argument is to tell Nix how much of the struct is actually initialized. In C, pointers to sockaddrs are often not fully initialized, or even allocated. But reading uninitialized memory in Rust is UB. So that argument is important.

@pronebird
Copy link
Contributor Author

pronebird commented Dec 18, 2024

The length argument is Option<socklen_t> so it's ok to pass None if I don't know the length when a syscall returns a non-null *sockaddr that has the AF_UNIX family set and a length inside of it.

The else branch is happily converting the whole range of types, i.e:
https://github.com/nix-rust/nix/blob/master/src/sys/socket/addr.rs#L1184-L1222

UnixAddr::from_raw() also happily accepts None as len and works on macOS (aka BSD):
https://github.com/nix-rust/nix/blob/master/src/sys/socket/addr.rs#L535-L569

So all this functionality is already there and it feels that there is just an omission in SockaddrStorage.

@pronebird
Copy link
Contributor Author

So it seems that the following is not true for all platforms:

assert!(
  UnixAddr::size() <= mem::size_of::<libc::sockaddr_storage>() as libc::socklen_t
);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants