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

*: Avoid creation panics when 'process' and 'linux' are unset #1726

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
30 changes: 16 additions & 14 deletions libcontainer/specconv/spec_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -238,24 +238,26 @@ func CreateLibcontainerConfig(opts *CreateOpts) (*configs.Config, error) {
config.Seccomp = seccomp
}
}
if spec.Process.SelinuxLabel != "" {
config.ProcessLabel = spec.Process.SelinuxLabel
}
if spec.Process != nil && spec.Process.OOMScoreAdj != nil {
config.OomScoreAdj = *spec.Process.OOMScoreAdj
}
if spec.Process.Capabilities != nil {
config.Capabilities = &configs.Capabilities{
Bounding: spec.Process.Capabilities.Bounding,
Effective: spec.Process.Capabilities.Effective,
Permitted: spec.Process.Capabilities.Permitted,
Inheritable: spec.Process.Capabilities.Inheritable,
Ambient: spec.Process.Capabilities.Ambient,
if spec.Process != nil {
if spec.Process.SelinuxLabel != "" {
config.ProcessLabel = spec.Process.SelinuxLabel
}
if spec.Process.OOMScoreAdj != nil {
config.OomScoreAdj = *spec.Process.OOMScoreAdj
}
if spec.Process.Capabilities != nil {
config.Capabilities = &configs.Capabilities{
Bounding: spec.Process.Capabilities.Bounding,
Effective: spec.Process.Capabilities.Effective,
Permitted: spec.Process.Capabilities.Permitted,
Inheritable: spec.Process.Capabilities.Inheritable,
Ambient: spec.Process.Capabilities.Ambient,
}
}
}
createHooks(spec, config)
config.Version = specs.Version
if spec.Linux.IntelRdt != nil {
if spec.Linux != nil && spec.Linux.IntelRdt != nil {
config.IntelRdt = &configs.IntelRdt{}
if spec.Linux.IntelRdt.L3CacheSchema != "" {
config.IntelRdt.L3CacheSchema = spec.Linux.IntelRdt.L3CacheSchema
Expand Down
14 changes: 8 additions & 6 deletions libcontainer/standard_init_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
package libcontainer

import (
"errors"
"fmt"
"os"
"os/exec"
Expand Down Expand Up @@ -152,12 +153,6 @@ func (l *linuxStandardInit) Init() error {
if unix.Getppid() != l.parentPid {
return unix.Kill(unix.Getpid(), unix.SIGKILL)
}
// Check for the arg before waiting to make sure it exists and it is
// returned as a create time error.
name, err := exec.LookPath(l.config.Args[0])
if err != nil {
return err
}
// Close the pipe to signal that we have completed our init.
l.pipe.Close()
// Wait for the FIFO to be opened on the other side before exec-ing the
Expand Down Expand Up @@ -186,6 +181,13 @@ func (l *linuxStandardInit) Init() error {
return newSystemErrorWithCause(err, "init seccomp")
}
}
if len(l.config.Args) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

We want to validate this on creation not after we have done all this work. Change the above code to if process == nil error out

Copy link
Member

Choose a reason for hiding this comment

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

In other words, there is no reason to do 90% of container creation if we get to the end and we cannot run the container because process == nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In other words, there is no reason to do 90% of container creation if we get to the end and we cannot run the container because process == nil

The point of this PR is allowing create for containers with no process (catching up with opencontainers/runtime-spec#701). You can't start those containers, which is why we have this check here, but we certainly don't want to error those out during create.

return errors.New("no process arguments configured")
}
name, err := exec.LookPath(l.config.Args[0])
if err != nil {
return err
}
if err := syscall.Exec(name, l.config.Args[0:], os.Environ()); err != nil {
return newSystemErrorWithCause(err, "exec user process")
}
Expand Down
18 changes: 13 additions & 5 deletions utils_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,10 @@ func getDefaultImagePath(context *cli.Context) string {

// newProcess returns a new libcontainer Process with the arguments from the
// spec and stdio from the current process.
func newProcess(p specs.Process) (*libcontainer.Process, error) {
func newProcess(p *specs.Process) (*libcontainer.Process, error) {
if p == nil {
return &libcontainer.Process{}, nil
}
lp := &libcontainer.Process{
Args: p.Args,
Env: p.Env,
Expand Down Expand Up @@ -261,7 +264,7 @@ func (r *runner) run(config *specs.Process) (int, error) {
r.destroy()
return -1, err
}
process, err := newProcess(*config)
process, err := newProcess(config)
if err != nil {
r.destroy()
return -1, err
Expand Down Expand Up @@ -291,7 +294,8 @@ func (r *runner) run(config *specs.Process) (int, error) {
// with detaching containers, and then we get a tty after the container has
// started.
handler := newSignalHandler(r.enableSubreaper, r.notifySocket)
tty, err := setupIO(process, rootuid, rootgid, config.Terminal, detach, r.consoleSocket)
terminal := config != nil && config.Terminal
tty, err := setupIO(process, rootuid, rootgid, terminal, detach, r.consoleSocket)
if err != nil {
r.destroy()
return -1, err
Expand Down Expand Up @@ -353,17 +357,21 @@ func (r *runner) terminate(p *libcontainer.Process) {

func (r *runner) checkTerminal(config *specs.Process) error {
detach := r.detach || (r.action == CT_ACT_CREATE)
terminal := config != nil && config.Terminal
// Check command-line for sanity.
if detach && config.Terminal && r.consoleSocket == "" {
if detach && terminal && r.consoleSocket == "" {
return fmt.Errorf("cannot allocate tty if runc will detach without setting console socket")
}
if (!detach || !config.Terminal) && r.consoleSocket != "" {
if (!detach || !terminal) && r.consoleSocket != "" {
return fmt.Errorf("cannot use console socket if runc will not detach or allocate tty")
}
return nil
}

func validateProcessSpec(spec *specs.Process) error {
if spec == nil {
Copy link
Member

Choose a reason for hiding this comment

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

this should be an error case

return nil
}
if spec.Cwd == "" {
return fmt.Errorf("Cwd property must not be empty")
}
Expand Down