Skip to content

OpenBSD fix long socket addresses #118349

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.

Sign up for GitHub

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

Closed
notgull wants to merge 5 commits into from
Closed

OpenBSD fix long socket addresses #118349

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fe1d97b
OpenBSD fix long socket addresses
notgull Nov 27, 2023
da6b4f6
bug -> feature
notgull Dec 2, 2023
d1090cf
chore: Review comments
notgull Feb 18, 2024
731b456
chore: Fix tidy
notgull Feb 18, 2024
b7eebfa
chore: Fix binary operation
notgull Feb 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion library/std/src/os/unix/net/addr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,17 @@ impl SocketAddr {
// When there is a datagram from unnamed unix socket
// linux returns zero bytes of address
len = sun_path_offset(&addr) as libc::socklen_t; // i.e., zero-length address
} else if addr.sun_family != libc::AF_UNIX as libc::sa_family_t {
} else if cfg!(target_os = "openbsd") {
// OpenBSD implements getsockname in a way where the
// socket name's length is more than what the buffer
// actually contains. Figure out the length for ourselves.
// https://marc.info/?l=openbsd-bugs&m=170105481926736&w=2
let sun_path: &[u8] = unsafe { crate::mem::transmute::<&[i8], &[u8]>(&addr.sun_path) };
len = crate::sys::memchr::memchr(0, sun_path)
.map_or(len, |new_len| (new_len + sun_path_offset(&addr)) as libc::socklen_t);
}

if addr.sun_family != libc::AF_UNIX as libc::sa_family_t {
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum Feb 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is now run even when len == 0. That might be the right behavior, but it seems like a change in behavior that should go to a separate PR.

Copy link
Contributor Author

@notgull notgull Feb 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added back the original behavior.

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum Feb 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the current PR actually adds back the original behavior, but thinking more on this it's not clear to me what the original intent behind placing the family check in an else was. I'm not able to find when this code was originally introduced quickly (maybe in #51553 but that seems surprising to me...), but I think my preference would be to check the family before the length (i.e., not guard it with any length comparisons). That seems like the most intuitive behavior for this function and I'd expect that it should work, if we see tests fail etc. we can revert and add some comments describing why.

(The current PR is adding a len == 0 check before returning an error is looking at the updated length after it may have been overwritten, though only in the OpenBSD case could it get overwritten to a zero I think).

return Err(io::const_io_error!(
io::ErrorKind::InvalidInput,
"file descriptor did not correspond to a Unix socket",
Expand Down