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

os: racy use of pidfd #67641

Closed
prattmic opened this issue May 24, 2024 · 3 comments
Closed

os: racy use of pidfd #67641

prattmic opened this issue May 24, 2024 · 3 comments
Assignees
Milestone

Comments

@prattmic
Copy link
Member

https://go.dev/cl/570036 started using a pidfd for Process operations in order to avoid races with wait/signal targeting the wrong process. The pidfd is stored in the Process.handle field.

Unfortunately there still exists a race with Release. Release does an atomic swap of the handle with invalidHandle, then closes the handle. However, a Wait or Signal call may have loaded the handle prior to the swap and will proceed to use it after the close completes.

This will most likely result in EBADF from the syscalls, but in the worst case a concurrent FindProcess/etc could open a new pidfd which receives the same FD number we retrieved from handle, and then our syscalls will succeed but target the wrong process.

We could come up with a bespoke little custom lock with atomics to make this safe, but these operations are pretty heavy-weight. I think sync.RWMutex will be more than sufficient. I will work on a fix.

cc @kolyshkin @golang/runtime

@prattmic prattmic added this to the Go1.23 milestone May 24, 2024
@prattmic prattmic self-assigned this May 24, 2024
@prattmic
Copy link
Member Author

One snag with the obvious RWMutex approach: a Release call will block until a concurrent Wait returns, which could be arbitrarily long. There isn't an easy way around this: the FD isn't safe to close until the kernel has evaluated it within the wait system call, which we can't detect. I suppose we could have Release leave a note behind telling Wait to do the release before returning, but I'm not sure if that complexity is necessary.

@thediveo
Copy link
Contributor

can you dup the pidfd?

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/588675 mentions this issue: os: overhaul handling of PID vs pidfd within Process

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

3 participants