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.

Sign up for GitHub

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

utils: Handle close_range more gracefully #4596

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

utils: Handle close_range more gracefully #4596

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
111e8dc
libcontainer: Use MaxInt32 as the last FD to match kernel size semantics
evanphx Jan 24, 2025
33315a0
libcontainer: if close_range fails, fall back to the old way
evanphx Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions libcontainer/utils/utils_unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,14 @@ func fdRangeFrom(minFd int, fn fdFunc) error {
func CloseExecFrom(minFd int) error {
// Use close_range(CLOSE_RANGE_CLOEXEC) if possible.
if haveCloseRangeCloexec() {
err := unix.CloseRange(uint(minFd), math.MaxUint, unix.CLOSE_RANGE_CLOEXEC)
return os.NewSyscallError("close_range", err)
err := unix.CloseRange(uint(minFd), math.MaxInt32, unix.CLOSE_RANGE_CLOEXEC)
evanphx marked this conversation as resolved.
Show resolved Hide resolved
if err == nil {
return nil
}

logrus.Debugf("close_range failed, closing range one at a time (error: %v)", err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I get this. If close range fails and we have detected closeRange is available, what does this fallback add?

The errors it can return don't seem like something using the regular close fallback will fix: https://man7.org/linux/man-pages/man2/close_range.2.html.

Am I missing something? Do you see where this new fallback can help?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more defensive, given the existing defensive attempts to see if close_range is available at all. We probe if we can use it, and thusly if for some reason the probe was wrong, we've got the fallback functionality at hand so seems prudent to use it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @evanphx here -- if we can use a fallback, it makes more sense to use it rather than give up and fail right away. Yes, we might still fail later, but oh well, we tried.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Should be all ready now.


// If close_range fails, we fall back to the standard loop.
}
// Otherwise, fall back to the standard loop.
return fdRangeFrom(minFd, unix.CloseOnExec)
Expand Down