Skip to content

std::net: adding new option todevice. #129172

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

Closed
wants to merge 4 commits into from

Conversation

devnexen
Copy link
Contributor

@devnexen devnexen commented Aug 16, 2024

Allows to bind a socket to an network interface, to exclusively listen from it. For packet sockets, regular bind is more appropriate.

tracking issue: #129182

@rustbot
Copy link
Collaborator

rustbot commented Aug 16, 2024

r? @Noratrieb

rustbot has assigned @Noratrieb.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 16, 2024
@rust-log-analyzer

This comment has been minimized.

@devnexen devnexen force-pushed the linux_bindtodevice branch from 293b0a2 to e6d0253 Compare August 17, 2024 04:13
@rust-log-analyzer

This comment has been minimized.

@devnexen devnexen force-pushed the linux_bindtodevice branch 2 times, most recently from b7bb523 to 215ccf3 Compare August 17, 2024 04:44
@rust-log-analyzer

This comment has been minimized.

@devnexen devnexen force-pushed the linux_bindtodevice branch from 215ccf3 to b0210c7 Compare August 17, 2024 05:11
@devnexen devnexen marked this pull request as ready for review August 17, 2024 05:39
@Noratrieb
Copy link
Member

Thank you for the PR! This PR contains a public library API change, which should follow the process of creating an ACP first: https://std-dev-guide.rust-lang.org/development/feature-lifecycle.html
Use @rustbot ready when your ACP has been accepeted.
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 21, 2024
@devnexen
Copy link
Contributor Author

Sorry for asking but is creating ACP a different process than tracking issue ?

@Noratrieb
Copy link
Member

Noratrieb commented Aug 21, 2024

Yes, asking for an ACP, which is a relatively new process, asks the libs-api team whether this is worth adding in the first place. The tracking issue is there once it has been added, to track stabilization.

let buf: [libc::c_char; libc::IFNAMSIZ] =
getsockopt(self, libc::SOL_SOCKET, libc::SO_BINDTODEVICE)?;
let s: &[u8] = unsafe { core::slice::from_raw_parts(buf.as_ptr() as *const u8, buf.len()) };
let ifrname = CStr::from_bytes_with_nul(s).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Bytes with null will return an Interior null runtime error when device names are shorter then 16 characters.
for example eno1 would be represented as [101, 110, 111, 49, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0].

Suggested change
let ifrname = CStr::from_bytes_with_nul(s).unwrap();
let ifrname = CStr::from_bytes_until_nul(s).unwrap();

#[cfg(any(target_os = "linux", target_os = "haiku", target_os = "vxworks"))]
pub fn set_todevice(&self, ifrname: &CStr) -> io::Result<()> {
let mut buf = [0; libc::IFNAMSIZ];
for (src, dst) in ifrname.to_bytes().iter().zip(&mut buf[..libc::IFNAMSIZ - 1]) {
Copy link
Member

Choose a reason for hiding this comment

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

this should probably check if the input name is too long rather than silently truncating it because you used zip()

Copy link
Contributor

@biabbas biabbas Aug 22, 2024

Choose a reason for hiding this comment

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

That value is already truncated to size specified by optlen when getsockopt is called with optlen.

Copy link
Contributor

Choose a reason for hiding this comment

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

Plus length of interface is 16 on most unix systems, so no value is truncated.

Copy link
Member

Choose a reason for hiding this comment

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

what I mean is, assuming IFNAMSIZ == 16, if you have an interface named mylonginterface (15 bytes), and you call this function with "mylonginterfacename" then it will silently truncate that and use mylonginterface instead of erroring like it should, because setsockopt never has a chance to see the bad name and report an error.

Allows to bind a socket to an network interface, to exclusively listen
from it. For packet sockets, regular bind is more appropriate.
@devnexen devnexen force-pushed the linux_bindtodevice branch from b0210c7 to dab4104 Compare August 22, 2024 06:28
/// let socket = UnixDatagram::unbound()?;
/// socket.set_todevice(c"eth0")?;
/// let name = socket.todevice()?;
/// assert_eq!(Ok("eth0"), name.to_str());
Copy link
Contributor

Choose a reason for hiding this comment

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

Something seems odd here.

#![feature(unix_set_todevice)]
use std::os::unix::net::UnixDatagram;
    ///
fn main() -> std::io::Result<()> {
    let socket = UnixDatagram::unbound()?;
    socket.set_todevice(c"eno1")?;
    let name = socket.todevice()?;
    assert_eq!(Ok("eno1"), name.to_str());
    Ok(())
}

The assertion fails.

thread 'main' panicked at test_case.rs:8:5:
assertion `left == right` failed
  left: Ok("eno1")
 right: Ok("�
�")
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@rust-log-analyzer

This comment has been minimized.

@devnexen devnexen force-pushed the linux_bindtodevice branch from 3057398 to f7d3a1a Compare August 22, 2024 18:53
@kennytm kennytm dismissed their stale review August 23, 2024 12:24

undefined behavior is resolved

/// use std::os::unix::net::UnixStream;
///
/// fn main() -> std::io::Result<()> {
/// let socket = UnixStream::connect("/tmp/sock")?;
Copy link
Member

@joshtriplett joshtriplett Aug 27, 2024

Choose a reason for hiding this comment

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

Why is this using UnixStream? That...shouldn't work.

The examples should be things expected to work.

Copy link
Member

Choose a reason for hiding this comment

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

Wait, it looks like these are being added to UNIX sockets? Shouldn't these be for TCP/UDP, not UNIX?

@rustbot rustbot added O-hermit Operating System: Hermit O-SGX Target: SGX O-solid Operating System: SOLID O-wasi Operating system: Wasi, Webassembly System Interface O-windows Operating system: Windows labels Aug 27, 2024
@rust-log-analyzer

This comment has been minimized.

@devnexen devnexen force-pushed the linux_bindtodevice branch from 4308d3f to c97a6ea Compare August 27, 2024 20:47
@rust-log-analyzer

This comment has been minimized.

@devnexen devnexen force-pushed the linux_bindtodevice branch from c97a6ea to 7537a8f Compare August 27, 2024 21:04
@the8472
Copy link
Member

the8472 commented Sep 3, 2024

My understanding of the SO_BINDTODEVICE must be used before binding or connecting a socket. Currently std has no way to support this, TcpStream and TcpListener constructors create already-bound / already-connected sockets.

So to support this std would first have to gain a way to create unbound sockets, configure them and then turn them into their final type.

see rust-lang/libs-team#66 (comment)

Additionally device-binding is a platform-specific API that's not fully portable. So it'd have to go into extension traits under the os module.

So I have to reject this for now until we have a better story around unbound sockets.

For now you can use the socket2 crate, which has device-binding methods

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-hermit Operating System: Hermit O-SGX Target: SGX O-solid Operating System: SOLID O-unix Operating system: Unix-like O-wasi Operating system: Wasi, Webassembly System Interface O-windows Operating system: Windows S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants