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 os::unix::net::SocketAddr::from_path #93239

Merged
merged 5 commits into from
Jan 29, 2022
Merged

Add os::unix::net::SocketAddr::from_path #93239

merged 5 commits into from
Jan 29, 2022

Conversation

Thomasdezeeuw
Copy link
Contributor

Creates a new SocketAddr from a path, supports both regular paths and
abstract namespaces.

Note that SocketAddr::from_abstract_namespace could be removed after this as SocketAddr::unix also supports abstract namespaces.

Updates #65275
Unblocks tokio-rs/mio#1527

r? @m-ou-se

Creates a new SocketAddr from a path, supports both regular paths and
abstract namespaces.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 23, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Jan 23, 2022

as SocketAddr::unix also supports abstract namespaces.

I don't think we should do that. Paths starting with a null byte aren't really Paths. That seems like a mis-use of the API. Also, we can gate from_abstract_namespace to only be available on platforms that support it, which is not possible by hiding that functionality through a Path api. The existing sockaddr_un() function used by bind, connect, etc. also purposely rejects abstract namespace addresses, to make sure the Paths are actually used as paths.

@Thomasdezeeuw
Copy link
Contributor Author

as SocketAddr::unix also supports abstract namespaces.

I don't think we should do that. Paths starting with a null byte aren't really Paths. That seems like a mis-use of the API.

I see you point, but Paths may contain NULL, just like strings, as it's valid UTF-8. For example Path::new("\0abstract-namespace") is perfectly valid.

Also, we can gate from_abstract_namespace to only be available on platforms that support it, which is not possible by hiding that functionality through a Path api. The existing sockaddr_un() function used by bind, connect, etc. also purposely rejects abstract namespace addresses, to make sure the Paths are actually used as paths.

👍 We could use it internally though, so that SocketAddr::from_abstract_namespace calls Socket::unix.

@m-ou-se
Copy link
Member

m-ou-se commented Jan 23, 2022

I see you point, but Paths may contain NULL, just like strings, as it's valid UTF-8. For example Path::new("\0abstract-namespace") is perfectly valid.

Depends what you mean with 'valid'. Such paths are not accepted by any api like bind or connect or File::open or Path::metadata(), or any other file system api, because they do not represent any possibly existing path.

Linux APIs do not treat AF_UNIX sockaddrs starting with a \0 as a path, but as something else. Having a Rust API that takes a Path that might end up being used as something completely different depending on its contents is just asking for (security) issues.

Using a leading \0 for abstract addresses was reasonable for a kernel/libc interface for backwards compatibility, but in Rust we can just have separate SocketAddr::from_path and SocketAddr::from_abstract_namespace, without any subtle suprises.

@Thomasdezeeuw
Copy link
Contributor Author

I see you point, but Paths may contain NULL, just like strings, as it's valid UTF-8. For example Path::new("\0abstract-namespace") is perfectly valid.

Depends what you mean with 'valid'. Such paths are not accepted by any api like bind or connect or File::open or Path::metadata(), or any other file system api, because they do not represent any possibly existing path.

Linux APIs do not treat AF_UNIX sockaddrs starting with a \0 as a path, but as something else. Having a Rust API that takes a Path that might end up being used as something completely different depending on its contents is just asking for (security) issues.

Using a leading \0 for abstract addresses was reasonable for a kernel/libc interface for backwards compatibility, but in Rust we can just have separate SocketAddr::from_path and SocketAddr::from_abstract_namespace, without any subtle suprises.

That's fair, I'll remove reject NULL bytes in the path in commit tonight.

And change it to disallow NULL bytes.
@Thomasdezeeuw
Copy link
Contributor Author

I've renamed it to from_path and it now returns an error if the path contains any NULL bytes.

@m-ou-se
Copy link
Member

m-ou-se commented Jan 24, 2022

(We should also add as_path for completeness, but that doesn't have to be part of this PR.)

The creation of libc::sockaddr_un is a safe operation, no need for it to
be unsafe.

This also uses the more performant copy_nonoverlapping instead of an
iterator.
Copy link
Contributor Author

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

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

(We should also add as_path for completeness, but that doesn't have to be part of this PR.)

@m-ou-se what would it do differently from as_pathname?

library/std/src/os/unix/net/addr.rs Show resolved Hide resolved
@Thomasdezeeuw Thomasdezeeuw changed the title Add os::unix::net::SocketAddr::unix Add os::unix::net::SocketAddr::from_path Jan 27, 2022
@rust-log-analyzer

This comment has been minimized.

@Thomasdezeeuw
Copy link
Contributor Author

I don't know why the CI failed, it seems it received some unexpected signal?

@klensy
Copy link
Contributor

klensy commented Jan 27, 2022

I don't know why the CI failed, it seems it received some unexpected signal?

It's not unique error, it's all over CI #93375 (comment)

@m-ou-se
Copy link
Member

m-ou-se commented Jan 28, 2022

what would it do differently from as_pathname?

Apologies, I missed that we already had that one.

It makes me wonder if we should name this new function from_pathname, but that's something we can discuss on the tracking issue.

Can you open a new tracking issue for from_path and use that one in the #[unstable] attribute?

I don't know why the CI failed, it seems it received some unexpected signal?

Looks spurious. I've restarted the CI.

@Thomasdezeeuw
Copy link
Contributor Author

what would it do differently from as_pathname?

Apologies, I missed that we already had that one.

It makes me wonder if we should name this new function from_pathname, but that's something we can discuss on the tracking issue.

Personally I don't really have a preference here.

Can you open a new tracking issue for from_path and use that one in the #[unstable] attribute?

I originally linked it to #65275, but I've created #93423.

I don't know why the CI failed, it seems it received some unexpected signal?

Looks spurious. I've restarted the CI.

@m-ou-se
Copy link
Member

m-ou-se commented Jan 28, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Jan 28, 2022

📌 Commit 35f578f has been approved by m-ou-se

@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 Jan 28, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 28, 2022
…askrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#92611 (Add links to the reference and rust by example for asm! docs and lints)
 - rust-lang#93158 (wasi: implement `sock_accept` and enable networking)
 - rust-lang#93239 (Add os::unix::net::SocketAddr::from_path)
 - rust-lang#93261 (Some unwinding related cg_ssa cleanups)
 - rust-lang#93295 (Avoid double panics when using `TempDir` in tests)
 - rust-lang#93353 (Unimpl {Add,Sub,Mul,Div,Rem,BitXor,BitOr,BitAnd}<$t> for Saturating<$t>)
 - rust-lang#93356 (Edit docs introduction for `std::cmp::PartialOrd`)
 - rust-lang#93375 (fix typo `documenation`)
 - rust-lang#93399 (rustbuild: Fix compiletest warning when building outside of root.)
 - rust-lang#93404 (Fix a typo from rust-lang#92899)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 18c8d0d into rust-lang:master Jan 29, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 29, 2022
@Thomasdezeeuw Thomasdezeeuw deleted the socketaddr_creation branch January 29, 2022 12:54
// null byte for pathname addresses is already there because we zeroed the
// struct
// SAFETY: `bytes` and `addr.sun_path` are not overlapping and
// both point to valid memory.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps add "we checked path is not too long"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants