Skip to content

Commit

Permalink
libct: fix a race with systemd removal
Browse files Browse the repository at this point in the history
For a previous attempt to fix that (and added test cases), see commit
9087f2e.

Alas, it's not always working because of cgroup directory TOCTOU.

To solve this and avoid the race, add an error _after_ the operation.
Implement it as a method that ignores the error that should be ignored.
Instead of currentStatus(), use faster runType(), since we are not
interested in Paused status here.

For Processes(), remove the pre-op check, and only use it after getting
an error, making the non-error path more straightforward.

For Signal(), add a second check after getting an error. The first check
is left as is because signalAllProcesses might print a warning if the
cgroup does not exist, and we'd like to avoid that.

This should fix an occasional failure like this one:

	not ok 84 kill detached busybox
	# (in test file tests/integration/kill.bats, line 27)
	#   `[ "$status" -eq 0 ]' failed
	....
	# runc kill test_busybox KILL (status=0):
	# runc kill -a test_busybox 0 (status=1):
	# time="2023-04-04T18:24:27Z" level=error msg="lstat /sys/fs/cgroup/devices/system.slice/runc-test_busybox.scope: no such file or directory"

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
  • Loading branch information
kolyshkin committed Apr 21, 2023
1 parent 3a2c0c2 commit fe278b9
Showing 1 changed file with 17 additions and 14 deletions.
31 changes: 17 additions & 14 deletions libcontainer/container_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,25 +112,27 @@ func (c *Container) OCIState() (*specs.State, error) {
return c.currentOCIState()
}

// ignoreCgroupError filters out cgroup-related errors that can be ignored,
// because the container is stopped and its cgroup is gone.
func (c *Container) ignoreCgroupError(err error) error {
if err == nil {
return nil
}
if errors.Is(err, os.ErrNotExist) && c.runType() == Stopped && !c.cgroupManager.Exists() {
return nil
}
return err
}

// Processes returns the PIDs inside this container. The PIDs are in the
// namespace of the calling process.
//
// Some of the returned PIDs may no longer refer to processes in the container,
// unless the container state is PAUSED in which case every PID in the slice is
// valid.
func (c *Container) Processes() ([]int, error) {
var pids []int
status, err := c.currentStatus()
if err != nil {
return pids, err
}
// for systemd cgroup, the unit's cgroup path will be auto removed if container's all processes exited
if status == Stopped && !c.cgroupManager.Exists() {
return pids, nil
}

pids, err = c.cgroupManager.GetAllPids()
if err != nil {
pids, err := c.cgroupManager.GetAllPids()
if err = c.ignoreCgroupError(err); err != nil {
return nil, fmt.Errorf("unable to get all container pids: %w", err)
}
return pids, nil
Expand Down Expand Up @@ -363,11 +365,12 @@ func (c *Container) Signal(s os.Signal, all bool) error {
return err
}
if all {
// for systemd cgroup, the unit's cgroup path will be auto removed if container's all processes exited
if status == Stopped && !c.cgroupManager.Exists() {
// Avoid calling signalAllProcesses which may print
// a warning trying to freeze a non-existing cgroup.
return nil
}
return signalAllProcesses(c.cgroupManager, s)
return c.ignoreCgroupError(signalAllProcesses(c.cgroupManager, s))
}
// to avoid a PID reuse attack
if status == Running || status == Created || status == Paused {
Expand Down

0 comments on commit fe278b9

Please sign in to comment.