cgroup.go: avoid panic on nil interface #207
Conversation
| @@ -323,6 +323,9 @@ func (c *cgroup) Tasks(subsystem Name, recursive bool) ([]Task, error) { | |||
|
|
|||
| func (c *cgroup) processes(subsystem Name, recursive bool, pType procType) ([]Process, error) { | |||
| s := c.getSubsystem(subsystem) | |||
| if s == nil { | |||
| return nil, ErrControllerNotActive | |||
| } | |||
| sp, err := c.path(subsystem) | |||
| if err != nil { | |||
| return nil, err | |||
There was a problem hiding this comment.
I was rethinking that error message. I think the following code might be more clear for the error
After Line 329,
if s == nil {
return nil, errors.Errorf("no such cgroup %s in subsystem %s", sp, subsystem)
}
WDYT?
There was a problem hiding this comment.
Could we define a new Error with this?
There was a problem hiding this comment.
os.ErrNotExist for errors.Wrapf ?
There was a problem hiding this comment.
I think os.ErrNotExist is fine 😃
There was a problem hiding this comment.
I was thinking something like
var ErrCgroupNotInSubsystem = errors.New("cgroups: cgroup not in subsystem")
There was a problem hiding this comment.
I was thinking something like
var ErrCgroupNotInSubsystem = errors.New("cgroups: cgroup not in subsystem")
The error doesn't contain the which subsystem and which cgroup. That is My concern
There was a problem hiding this comment.
Yes, we should make it clear
There was a problem hiding this comment.
Could we use fmt.Errorf("cgroups: %s doesn't exist in %s subsystem", sp, subsystem) ?
There was a problem hiding this comment.
Could we use
fmt.Errorf("cgroups: %s doesn't exist in %s subsystem", sp, subsystem)?
sgtm
There was a problem hiding this comment.
A new commit had been pushed.
Signed-off-by: linrl3 <[email protected]>
linrl3
force-pushed
the
topic/nil-interface-bug
branch
from
6aeb1eb to
d55de5d
Compare
Hi, this PR's related issue is #205, which is a potential nil interface panic case.
In https://github.com/containerd/cgroups/blob/main/cgroup.go#L330, if we pass a subsystem that doesn't exist into func (c *cgroup) Processes(subsystem Name, recursive bool), we would get a panic: interface conversion: interface is nil, not cgroups.pather.
So we'd check nil interface and return error if it's nil to avoid panic.