Add os::unix::net::SocketAddr::from_path by Thomasdezeeuw · Pull Request #93239 · rust-lang/rust

Thomasdezeeuw · 2022-01-23T16:14:31Z

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.

m-ou-se · 2022-01-23T16:36:18Z

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 · 2022-01-23T16:58:28Z

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 · 2022-01-23T17:57:00Z

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.

library/std/src/os/unix/net/addr.rs

Thomasdezeeuw · 2022-01-24T14:34:48Z

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 · 2022-01-24T17:04:22Z

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

library/std/src/os/unix/net/addr.rs

m-ou-se · 2022-01-24T17:13:40Z

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

library/std/src/os/unix/net/addr.rs

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.

Thomasdezeeuw

(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

Thomasdezeeuw · 2022-01-27T14:14:09Z

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

klensy · 2022-01-27T15:06:53Z

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 · 2022-01-28T13:49:03Z

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 · 2022-01-28T14:01:48Z

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 · 2022-01-28T14:13:56Z

@bors r+

bors · 2022-01-28T14:13:58Z

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

…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

Kixunil · 2022-02-21T12:14:18Z

library/std/src/os/unix/net/addr.rs

-    // 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.


Perhaps add "we checked path is not too long"

Add os::unix::net::SocketAddr::unix

f2cdb57

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

rust-highfive assigned m-ou-se Jan 23, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 23, 2022

Thomasdezeeuw mentioned this pull request Jan 23, 2022

request: Make unix::net::SocketAddr create methods public #65275

Closed

the8472 reviewed Jan 23, 2022

View reviewed changes

library/std/src/os/unix/net/addr.rs Show resolved Hide resolved

Rename SocketAddr::unix to from_path

c1cd200

And change it to disallow NULL bytes.

m-ou-se reviewed Jan 24, 2022

View reviewed changes

library/std/src/os/unix/net/addr.rs Show resolved Hide resolved

klensy reviewed Jan 26, 2022

View reviewed changes

library/std/src/os/unix/net/addr.rs Outdated Show resolved Hide resolved

klensy reviewed Jan 26, 2022

View reviewed changes

library/std/src/os/unix/net/addr.rs Outdated Show resolved Hide resolved

klensy reviewed Jan 26, 2022

View reviewed changes

library/std/src/os/unix/net/addr.rs Show resolved Hide resolved

Thomasdezeeuw added 2 commits January 27, 2022 09:52

Make sockaddr_un safe and use copy_nonoverlapping

ca9a3c9

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.

Use sockaddr_un in unix SocketAddr::from_path

4acb8ac

Thomasdezeeuw commented Jan 27, 2022

View reviewed changes

library/std/src/os/unix/net/addr.rs Show resolved Hide resolved

Thomasdezeeuw changed the title ~~Add os::unix::net::SocketAddr::unix~~ Add os::unix::net::SocketAddr::from_path Jan 27, 2022

This comment has been minimized.

Sign in to view

klensy mentioned this pull request Jan 27, 2022

compiletest crashes on CI with: expected success, got: signal: 11 (core dumped) #93384

Closed

Thomasdezeeuw mentioned this pull request Jan 28, 2022

Tracking Issue for unix_socket_creation #93423

Closed

4 tasks

Update tracking issue for unix_socket_creation

35f578f

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

matthiaskrgr mentioned this pull request Jan 28, 2022

Rollup of 10 pull requests #93427

Merged

bors merged commit 18c8d0d into rust-lang:master Jan 29, 2022

rustbot added this to the 1.60.0 milestone Jan 29, 2022

Thomasdezeeuw deleted the socketaddr_creation branch January 29, 2022 12:54

Kixunil reviewed Feb 21, 2022

View reviewed changes

Uh oh!

Conversation

Thomasdezeeuw commented Jan 23, 2022

Uh oh!

m-ou-se commented Jan 23, 2022

Uh oh!

Thomasdezeeuw commented Jan 23, 2022

Uh oh!

m-ou-se commented Jan 23, 2022

Uh oh!

Uh oh!

Thomasdezeeuw commented Jan 24, 2022

Uh oh!

Thomasdezeeuw commented Jan 24, 2022

Uh oh!

Uh oh!

m-ou-se commented Jan 24, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Thomasdezeeuw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

Thomasdezeeuw commented Jan 27, 2022

Uh oh!

klensy commented Jan 27, 2022

Uh oh!

m-ou-se commented Jan 28, 2022

Uh oh!

Thomasdezeeuw commented Jan 28, 2022

Uh oh!

m-ou-se commented Jan 28, 2022

Uh oh!

bors commented Jan 28, 2022

Uh oh!

Kixunil Feb 21, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants