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

Fix a number of potential fd leaks #1636

Merged
merged 8 commits into from
Nov 30, 2022

Conversation

Thomasdezeeuw
Copy link
Collaborator

We do this by simply creating a fd-managing types, e.g. UnixDatagram, from the fd once it's created.

Also first tries to parse the path as that can fail without doing a system call.

Fixes #1635

/cc @silence-coding

We do this by simply creating a fd-managing type, e.g. UnixDatagram,
from the fd once it's created.

Also first tries to parse the path as that can fail without doing a
system call.
@silence-coding
Copy link

@Thomasdezeeuw
In addition to these two places, you are advised to review other places. I just found another place

@silence-coding
Copy link

I think new_socket should return OwnedFd or OwnedSocket or something.

@Thomasdezeeuw
Copy link
Collaborator Author

I think new_socket should return OwnedFd or OwnedSocket or something.

Can't, we support older Rust versions.

@silence-coding
Copy link

I think new_socket should return OwnedFd or OwnedSocket or something.

Can't, we support older Rust versions.

So can we define our own OwnedFd or OwnedSocket? I feel that this is a bit against rust's deterministic release principle of resources.

@Darksonn
Copy link
Contributor

Looks reasonable to me. Is there any way to test this?

So can we define our own OwnedFd or OwnedSocket?

Immediately putting the file descriptor of the unix datagram into a UnixDatagram seems at least as good as first putting it into an OwnedFd and then converting it into a UnixDatagram.

@Thomasdezeeuw
Copy link
Collaborator Author

Looks reasonable to me. Is there any way to test this?

If we can somehow trigger an error in binding/listening/connecting. But outside of binding every port I can't really think of a reasonable solution at the moment.

Now all the fd creating code follows the following pattern.

let raw_fd = create_fd();
let managed_fd = wrap(raw_fd);
// Additional system calls
return managed_fd
Do it in new_socket so that the higher levels don't have to worry about
it.
Don't agree with the `&path` -> `path`, but it's fine.
@Thomasdezeeuw Thomasdezeeuw changed the title Don't leak fds on error in UDS on Unix Fix a number of potential fd leaks Nov 28, 2022
@silence-coding
Copy link

Looks reasonable to me. Is there any way to test this?

So can we define our own OwnedFd or OwnedSocket?

Immediately putting the file descriptor of the unix datagram into a UnixDatagram seems at least as good as first putting it into an OwnedFd and then converting it into a UnixDatagram.

I think the leakage scenario has been basically resolved. Can we consider solving the known issues first and redesigning this issue in the future to ensure that no new leakage occurs in the future?

@Thomasdezeeuw
Copy link
Collaborator Author

I think the leakage scenario has been basically resolved. Can we consider solving the known issues first and redesigning this issue in the future to ensure that no new leakage occurs in the future?

I don't think we'll need any further changes beyond this. What did you have in mind?

@Thomasdezeeuw Thomasdezeeuw merged commit bdaaab6 into tokio-rs:master Nov 30, 2022
@Thomasdezeeuw Thomasdezeeuw deleted the issue#1635_uds_fd_leak branch November 30, 2022 16:07
@silence-coding
Copy link

I think the leakage scenario has been basically resolved. Can we consider solving the known issues first and redesigning this issue in the future to ensure that no new leakage occurs in the future?

I don't think we'll need any further changes beyond this. What did you have in mind?

No, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The UnixStream has an FD leakage problem.
3 participants