Skip to content
Merged
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
2 changes: 1 addition & 1 deletion cmd/ctr/commands/commands.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ var (
},
cli.StringFlag{
Name: "runtime",
Usage: "runtime name",
Usage: "runtime name or absolute path to runtime binary",
Value: defaults.DefaultRuntime,
},
cli.StringFlag{
Expand Down
6 changes: 6 additions & 0 deletions runtime/v2/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"os"
"os/exec"
"path/filepath"
"strings"
"sync"

"github.com/containerd/containerd/containers"
Expand Down Expand Up @@ -244,6 +245,11 @@ func (m *ShimManager) resolveRuntimePath(runtime string) (string, error) {
return runtime, nil
}

// Check if relative path to runtime binary provided
if strings.Contains(runtime, "/") {
return "", fmt.Errorf("invalid runtime name %s, correct runtime name should be either format like `io.containerd.runc.v1` or a full path to the binary", runtime)
}

// Preserve existing logic and resolve runtime path from runtime name.

name := shimbinary.BinaryName(runtime)
Expand Down
99 changes: 99 additions & 0 deletions runtime/v2/manager_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
//go:build !windows && !darwin
// +build !windows,!darwin

/*
Copyright The containerd Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package v2

import (
"os"
"testing"
)

// setupAbsoluteShimPath creates a temporary directory in $PATH with an empty
// shim executable file in it to test the exec.LookPath branch of resolveRuntimePath
func setupAbsoluteShimPath(t *testing.T) (string, error) {
tempShimDir := t.TempDir()

_, err := os.Create(tempShimDir + "/containerd-shim-runc-v2")
if err != nil {
return "", err
}

t.Setenv("PATH", tempShimDir+":"+os.Getenv("PATH"))
absoluteShimPath := tempShimDir + "/containerd-shim-runc-v2"

err = os.Chmod(absoluteShimPath, 0777)
if err != nil {
return "", err
}

return absoluteShimPath, nil
}

func TestResolveRuntimePath(t *testing.T) {
sm := &ShimManager{}
absoluteShimPath, err := setupAbsoluteShimPath(t)
if err != nil {
t.Errorf("Failed to create temporary shim path: %q", err)
}

tests := []struct {
runtime string
want string
}{
{ // Absolute path
runtime: absoluteShimPath,
want: absoluteShimPath,
},
{ // Binary name
runtime: "io.containerd.runc.v2",
want: absoluteShimPath,
},
{ // Invalid absolute path
runtime: "/fake/abs/path",
want: "",
},
{ // No name
runtime: "",
want: "",
},
{ // Relative Path
runtime: "./containerd-shim-runc-v2",
want: "",
},
{
runtime: "fake/containerd-shim-runc-v2",
want: "",
},
{
runtime: "./fake/containerd-shim-runc-v2",
want: "",
},
{ // Relative Path or Bad Binary Name
runtime: ".io.containerd.runc.v2",
want: "",
},
}

for _, c := range tests {
have, _ := sm.resolveRuntimePath(c.runtime)
if have != c.want {
t.Errorf("Expected %q, got %q", c.want, have)
}
}
}
2 changes: 1 addition & 1 deletion runtime/v2/shim/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func Command(ctx context.Context, config *CommandConfig) (*exec.Cmd, error) {
func BinaryName(runtime string) string {
// runtime name should format like $prefix.name.version
parts := strings.Split(runtime, ".")
if len(parts) < 2 {
if len(parts) < 2 || parts[0] == "" {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Still not robust to catch a case like foo/bar.

Probably, we should just do :

runtime := clicontext.String("runtime") 
if strings.Contains(runtime, "/") && !filepath.IsAbs(runtime) {
  return fmt.Errorf("runtime must be a runtime name (e.g., \"io.containerd.runc.v2\") or absolute path to the runtime binary (e.g., \"/usr/local/bin/containerd-shim-runc-v2\"), got %q", runtime)
}

Copy link
Copy Markdown
Contributor Author

@ginglis13 ginglis13 Feb 7, 2022

Choose a reason for hiding this comment

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

From my testing, the existing check len(parts) < 2 already covers this case as strings.Split("foo/bar", ".") will result in a slice of length 1 : ["foo/bar"]

It doesn't however cover foo/bar.baz.foo - I'll go ahead with this suggestion 🙂

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Would you like this to go in runtime/v2/manager.go here ? or would it belong better in cmd/ctr/commands/run/run_unix.go ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I prefer to have that under runtime/ rather than cmd/

return ""
}

Expand Down