Skip to content

runtime-v2: add validation for runtime name#2729

Merged
estesp merged 1 commit intocontainerd:masterfrom
Ace-Tang:master
Oct 23, 2018
Merged

runtime-v2: add validation for runtime name#2729
estesp merged 1 commit intocontainerd:masterfrom
Ace-Tang:master

Conversation

@Ace-Tang
Copy link
Copy Markdown
Contributor

@Ace-Tang Ace-Tang commented Oct 18, 2018

add validation for runtime name, if runtime name is invalid,
containerd will got panic.

Signed-off-by: Ace-Tang [email protected]

The problem is found when I test with kata-shim, and write the wrong runtime

#ctr run --runtime containerd-shim-kata-v2 -d reg.docker.alibaba-inc.com/base/busybox:latest c1 top
ctr: transport is closing: unavailable

containerd panic log

panic: runtime error: index out of range

goroutine 129 [running]:
github.com/containerd/containerd/runtime/v2/shim.BinaryName(0xc42003f4e0, 0x17, 0x3, 0x6)
    /home/ace/gosrc/src/github.com/containerd/containerd/runtime/v2/shim/util.go:76 +0x157
github.com/containerd/containerd/runtime/v2/shim.Command(0x7fcfef4c4f60, 0xc4205232c0, 0xc42003f4e0, 0x17, 0xc42035a000, 0x1f, 0xc4203cc640, 0x38, 0xc420315ac0, 0x3, ...)
    /home/ace/gosrc/src/github.com/containerd/containerd/runtime/v2/shim/util.go:51 +0x1af
github.com/containerd/containerd/runtime/v2.(*binary).Start(0xc42083d560, 0x7fcfef4c4f60, 0xc4205232c0, 0x0, 0x0, 0x0)
    /home/ace/gosrc/src/github.com/containerd/containerd/runtime/v2/binary.go:63 +0x155
github.com/containerd/containerd/runtime/v2.(*TaskManager).Create(0xc4203c7180, 0x7fcfef4c4f60, 0xc4205232c0, 0xc420157e08, 0x2, 0xc420523380, 0xc420315a80, 0x1, 0x1, 0xc420374210, ...)

fuweid
fuweid reviewed Oct 18, 2018
View reviewed changes
Comment thread runtime/v2/shim/util.go Outdated
Copy link
Copy Markdown
Member

@fuweid fuweid Oct 18, 2018

Choose a reason for hiding this comment

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

are we need to check the format like io.containerd.xx.yy here?

Copy link
Copy Markdown
Contributor Author

@Ace-Tang Ace-Tang Oct 18, 2018

Choose a reason for hiding this comment

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

From the logic, runtime name is not limited. But I also curious about whether name need
specific name prefix.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Oct 18, 2018 •
edited
Loading

Codecov Report

Merging #2729 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2729      +/-   ##
==========================================
- Coverage   43.75%   43.73%   -0.03%     
==========================================
  Files         100      100              
  Lines       10725    10731       +6     
==========================================
  Hits         4693     4693              
- Misses       5301     5307       +6     
  Partials      731      731
Flag Coverage Δ
#linux 47.4% <0%> (-0.03%) ⬇️
#windows 40.88% <0%> (-0.03%) ⬇️
Impacted Files Coverage Δ
runtime/v2/shim/util.go 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 133ac5c...7b1b16b. Read the comment docs.

@Ace-Tang
runtime-v2: add validation for runtime name
7b1b16b 
add validation for runtime name, if runtime name is invalid,
containerd will got panic.

Signed-off-by: Ace-Tang <[email protected]>
@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Oct 22, 2018

LGTM

1 similar comment
@ehazlett
Copy link
Copy Markdown
Member

ehazlett commented Oct 23, 2018

LGTM

@estesp estesp added this to the 1.2 milestone Oct 23, 2018
@estesp estesp merged commit 9fb7eed into containerd:master Oct 23, 2018
@thaJeztah thaJeztah mentioned this pull request Oct 25, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fuweid fuweid fuweid left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.2

Development

Successfully merging this pull request may close these issues.

6 participants

@Ace-Tang @codecov-io @crosbymichael @ehazlett @fuweid @estesp