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

Set the close-on-exec flag for the duplicate epoll_fd #1491

Merged
merged 1 commit into from
Jun 9, 2021

Conversation

Tim-Zhang
Copy link
Contributor

@Tim-Zhang Tim-Zhang commented May 25, 2021

Set the close-on-exec flag for the duplicate epoll_fd

The close-on-exec flag (FD_CLOEXEC; see fcntl(2)) for the
duplicate descriptor created by dup(2) is off.

We can use fcntl + F_DUPFD_CLOEXEC to dup the epoll_fd to fix this
issue.

Fixes: tokio-rs/tokio#3809

Signed-off-by: Tim Zhang tim@hyper.sh

@MikailBag
Copy link

Modified code does not resolve problem completely, because fork can happen between duplicating fd and calling cloexec.

I think it is safer to use single syscall: fcntl with the F_DUPDF_CLOEXEC cmd

@Tim-Zhang
Copy link
Contributor Author

@MikailBag Thank you for your advice, I have updated the code

@Tim-Zhang Tim-Zhang changed the title Set FD_CLOEXEC for duped epoll_fd Set the close-on-exec flag for the duplicate epoll_fd May 25, 2021
The close-on-exec flag (FD_CLOEXEC; see fcntl(2)) for the
duplicate descriptor created by dup(2) is off.

We can use fcntl + F_DUPFD_CLOEXEC to dup the epoll_fd to fix this
issue.

Fixes: tokio-rs/tokio#3809

Signed-off-by: Tim Zhang <tim@hyper.sh>
Copy link
Collaborator

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

LGTM. CI is blocked on rust-lang/rustup#2774.

@Tim-Zhang
Copy link
Contributor Author

/retest

@Tim-Zhang
Copy link
Contributor Author

Hi @Thomasdezeeuw , How can I trigger the retest of FreeBSD?

@Thomasdezeeuw
Copy link
Collaborator

Hi @Thomasdezeeuw , How can I trigger the retest of FreeBSD?

The problem is that Rustup is currently broken on FreeBSD, see rust-lang/rustup#2774. The good news is that it's (likely) going to be fixed in 1.24.3, which already has a beta release: https://internals.rust-lang.org/t/rustup-1-24-3-seeking-beta-testers-particularly-freebsd/14813.

I'll restart the CI once Rustup 1.24.3 stable is released and merge this.

@Tim-Zhang
Copy link
Contributor Author

@Thomasdezeeuw Got it, thanks.

@Thomasdezeeuw Thomasdezeeuw merged commit c52635c into tokio-rs:master Jun 9, 2021
@Thomasdezeeuw
Copy link
Collaborator

Thanks @Tim-Zhang, sorry for the delay.

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.

epoll_fd leaks to child processes
3 participants