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

Add abstract namespace support for Unix domain sockets #85379

Merged
merged 6 commits into from
Oct 16, 2021

Conversation

mdaverde
Copy link
Contributor

@mdaverde mdaverde commented May 16, 2021

Hello! The other day I wanted to mess around with UDS in Rust and found that abstract namespaces (unix(7)) on Linux still needed development. I took the approach of adding _addr specific public functions to reduce conflicts.

Feature name: unix_socket_abstract
Tracking issue: #85410
Further context: #42048

Non-platform specific additions

UnixListener::bind_addr(&SocketAddr) -> Result<UnixListener>

UnixStream::connect_addr(&SocketAddr) -> Result<()>

UnixDatagram::bind_addr(&SocketAddr) -> Result<UnixDatagram>

UnixDatagram::connect_addr(&SocketAddr) -> Result<()>

UnixDatagram::send_to_addr(&self, &[u8], &SocketAddr) -> Result<usize>

Platform-specific (Linux) additions

SocketAddr::from_abstract_namespace(&[u8]) -> SocketAddr

SockerAddr::as_abstract_namespace() -> Option<&[u8]>

Example

#![feature(unix_socket_abstract)]
use std::os::unix::net::{UnixListener, SocketAddr};

fn main() -> std::io::Result<()> {
    let addr = SocketAddr::from_abstract_namespace(b"namespace")?; // Linux only
    let listener = match UnixListener::bind_addr(&addr) {
        Ok(sock) => sock,
        Err(err) => {
            println!("Couldn't bind: {:?}", err);
            return Err(err);
        }
    };
    Ok(())
}

Further Details

The main inspiration for the implementation came from the nix-rust crate but there are also other historical attempts with similar approaches.

A comment I did have was with this change, we now allow a SocketAddr to be constructed explicitly rather than just used almost as a handle for the return of peer_addr and local_addr. We could consider adding other explicit constructors (e.g. SocketAddr::from_pathname, SockerAddr::from_unnamed).

Cheers!

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 16, 2021
@rust-log-analyzer

This comment has been minimized.

@joshtriplett
Copy link
Member

This looks reasonable to me!

You need to create a tracking issue, and change the issue number in the stability attributes to reference that tracking issue.

With that fixed, if CI passes and a try build passes, r=me.

@mdaverde
Copy link
Contributor Author

Updated the stability attributes to a new tracking issue! Although, I think I need maintainer approval to rerun the build?

@joshtriplett
Copy link
Member

@bors try

@bors
Copy link
Contributor

bors commented May 17, 2021

⌛ Trying commit a4d0489 with merge 1c03bdf6c40e17f64fa452dd50a3e41fcd70fb69...

@bors
Copy link
Contributor

bors commented May 17, 2021

☀️ Try build successful - checks-actions
Build commit: 1c03bdf6c40e17f64fa452dd50a3e41fcd70fb69 (1c03bdf6c40e17f64fa452dd50a3e41fcd70fb69)

@joshtriplett
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented May 18, 2021

📌 Commit a4d0489 has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 18, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 18, 2021
Add abstract namespace support for Unix domain sockets

Hello! The other day I wanted to mess around with UDS in Rust and found that abstract namespaces ([unix(7)](https://man7.org/linux/man-pages/man7/unix.7.html)) on Linux still needed development. I took the approach of adding `_addr` specific public functions to reduce conflicts.

Feature name: `unix_socket_abstract`
Tracking issue: rust-lang#85410
Further context: rust-lang#42048

## Non-platform specific additions

`UnixListener::bind_addr(&SocketAddr) -> Result<UnixListener>`

`UnixStream::connect_addr(&SocketAddr) -> Result<()>`

`UnixDatagram::bind_addr(&SocketAddr) -> Result<UnixDatagram>`

`UnixDatagram::connect_addr(&SocketAddr) -> Result<()>`

`UnixDatagram::send_to_addr(&self, &[u8], &SocketAddr) -> Result<usize>`

## Platform-specific (Linux) additions

`SocketAddr::from_abstract_namespace(&[u8]) -> SocketAddr`

`SockerAddr::as_abstract_namespace() -> Option<&[u8]>`

## Example

```rust
#![feature(unix_socket_abstract)]
use std::os::unix::net::{UnixListener, SocketAddr};

fn main() -> std::io::Result<()> {
    let addr = SocketAddr::from_abstract_namespace(b"namespace")?; // Linux only
    let listener = match UnixListener::bind_addr(&addr) {
        Ok(sock) => sock,
        Err(err) => {
            println!("Couldn't bind: {:?}", err);
            return Err(err);
        }
    };
    Ok(())
}
```

## Further Details

The main inspiration for the implementation came from the [nix-rust](https://github.com/nix-rust/nix/blob/master/src/sys/socket/addr.rs#L558) crate but there are also other [historical](rust-lang@c4db068) [attempts](https://github.com/tormol/uds/blob/master/src/addr.rs#L324) with similar approaches.

A comment I did have was with this change, we now allow a `SocketAddr` to be constructed explicitly rather than just used almost as a handle for the return of `peer_addr` and `local_addr`. We could consider adding other explicit constructors (e.g. `SocketAddr::from_pathname`, `SockerAddr::from_unnamed`).

Cheers!
@GuillaumeGomez
Copy link
Member

Failed in CI:

[RUSTC-TIMING] object test:false 10.149
error: unused import: `ptr`
 --> library/std/src/os/./unix/net/addr.rs:5:40
  |
5 | use crate::{ascii, fmt, io, iter, mem, ptr};
  |
  |
  = note: `-D unused-imports` implied by `-D warnings`
error: aborting due to previous error

[RUSTC-TIMING] std test:false 2.398
error: could not compile `std`

@bors: r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 18, 2021
@mdaverde
Copy link
Contributor Author

Hmm is that an allowable import? I believe I am using it on line :288 of /unix/net/addr.rs but maybe something changed:

            ptr::copy_nonoverlapping(
                namespace.as_ptr(),
                addr.sun_path.as_mut_ptr().offset(1) as *mut u8,
                namespace.len(),
            );

@GuillaumeGomez
Copy link
Member

Is it behind a cfg by any chance? Worst case, simply replace ptr with std::ptr and you're good to go. :)

@mdaverde
Copy link
Contributor Author

Ah yes, that was it. Went ahead and moved the use declaration within the fn block. Thank you!

@GuillaumeGomez
Copy link
Member

@bors: r=joshtriplett,GuillaumeGomez

@bors
Copy link
Contributor

bors commented May 18, 2021

📌 Commit b0b0267 has been approved by joshtriplett,GuillaumeGomez

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 18, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 18, 2021
…tt,GuillaumeGomez

Add abstract namespace support for Unix domain sockets

Hello! The other day I wanted to mess around with UDS in Rust and found that abstract namespaces ([unix(7)](https://man7.org/linux/man-pages/man7/unix.7.html)) on Linux still needed development. I took the approach of adding `_addr` specific public functions to reduce conflicts.

Feature name: `unix_socket_abstract`
Tracking issue: rust-lang#85410
Further context: rust-lang#42048

## Non-platform specific additions

`UnixListener::bind_addr(&SocketAddr) -> Result<UnixListener>`

`UnixStream::connect_addr(&SocketAddr) -> Result<()>`

`UnixDatagram::bind_addr(&SocketAddr) -> Result<UnixDatagram>`

`UnixDatagram::connect_addr(&SocketAddr) -> Result<()>`

`UnixDatagram::send_to_addr(&self, &[u8], &SocketAddr) -> Result<usize>`

## Platform-specific (Linux) additions

`SocketAddr::from_abstract_namespace(&[u8]) -> SocketAddr`

`SockerAddr::as_abstract_namespace() -> Option<&[u8]>`

## Example

```rust
#![feature(unix_socket_abstract)]
use std::os::unix::net::{UnixListener, SocketAddr};

fn main() -> std::io::Result<()> {
    let addr = SocketAddr::from_abstract_namespace(b"namespace")?; // Linux only
    let listener = match UnixListener::bind_addr(&addr) {
        Ok(sock) => sock,
        Err(err) => {
            println!("Couldn't bind: {:?}", err);
            return Err(err);
        }
    };
    Ok(())
}
```

## Further Details

The main inspiration for the implementation came from the [nix-rust](https://github.com/nix-rust/nix/blob/master/src/sys/socket/addr.rs#L558) crate but there are also other [historical](rust-lang@c4db068) [attempts](https://github.com/tormol/uds/blob/master/src/addr.rs#L324) with similar approaches.

A comment I did have was with this change, we now allow a `SocketAddr` to be constructed explicitly rather than just used almost as a handle for the return of `peer_addr` and `local_addr`. We could consider adding other explicit constructors (e.g. `SocketAddr::from_pathname`, `SockerAddr::from_unnamed`).

Cheers!
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 4, 2021
@bors
Copy link
Contributor

bors commented Oct 6, 2021

⌛ Testing commit 3b441c8 with merge 1aa771d03efaa4bcb086adbf0e9e38fb98a6f428...

@bors
Copy link
Contributor

bors commented Oct 6, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 6, 2021
@rust-log-analyzer

This comment has been minimized.

@kennytm kennytm 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 Oct 7, 2021
@mdaverde
Copy link
Contributor Author

Integrated the I/O safety changes and did a rebase 🙏

r? @joshtriplett

@joshtriplett
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 15, 2021

📌 Commit 15b1198 has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 15, 2021
@bors
Copy link
Contributor

bors commented Oct 15, 2021

⌛ Testing commit 15b1198 with merge 6cc0a76...

@bors
Copy link
Contributor

bors commented Oct 16, 2021

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing 6cc0a76 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 16, 2021
@bors bors merged commit 6cc0a76 into rust-lang:master Oct 16, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 16, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6cc0a76): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` merged-by-bors This PR was explicitly merged by bors. O-unix Operating system: Unix-like S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.