Add os::unix::net::SocketAddr::from_path by Thomasdezeeuw · Pull Request #93239...

Copy link

Contributor

m-ou-se commented 25 days ago

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 commented 25 days ago

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 commented 25 days ago

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 commented 24 days ago

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.

Thomasdezeeuw commented 24 days ago

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

m-ou-se commented 24 days ago

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

This comment has been hidden.

Thomasdezeeuw commented 21 days ago

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

klensy commented 21 days ago

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 commented 20 days ago

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 commented 20 days ago

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 commented 20 days ago

@bors r+

bors commented 20 days ago

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