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

specconv: fix null spec.Process making runc panic #1826

Merged
merged 1 commit into from
Mar 24, 2020

Conversation

jingxiaolu
Copy link
Contributor

Process of Spec struct is a pointer:

	Process *Process `json:"process,omitempty"`

When calling CreateLibcontainerConfig() with opts.Spec.Process: nil, runc will panic.

I added a new test case TestNullProcess in libcontainer/specconv, it will failed with:

=== RUN   TestNullProcess
--- FAIL: TestNullProcess (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0xc8 pc=0x540029]

goroutine 20 [running]:
testing.tRunner.func1(0xc42007e9c0)
        /usr/lib/golang/src/testing/testing.go:622 +0x29d
panic(0x57c280, 0x861a20)
        /usr/lib/golang/src/runtime/panic.go:489 +0x2cf
github.com/opencontainers/runc/libcontainer/specconv.CreateLibcontainerConfig(0xc42003af70, 0x3, 0xc42007ad00, 0xc42007c040)
        /home/workspace/src/github.com/opencontainers/runc/libcontainer/specconv/spec_linux.go:244 +0x7b9
github.com/opencontainers/runc/libcontainer/specconv.TestNullProcess(0xc42007e9c0)
        /home/workspace/src/github.com/opencontainers/runc/libcontainer/specconv/spec_linux_test.go:456 +0xa8
testing.tRunner(0xc42007e9c0, 0x5b9440)
        /usr/lib/golang/src/testing/testing.go:657 +0x96
created by testing.(*T).Run
        /usr/lib/golang/src/testing/testing.go:697 +0x2ca
FAIL    github.com/opencontainers/runc/libcontainer/specconv    0.009s

So I wrote this PR.

Please help to review and share your comments.

Signed-off-by: Jingxiao Lu [email protected]

@dongsupark
Copy link

Looks good to me.
Yeah, I've seen such a panic many times. Then I realized that this issue was already addressed via #1726, which is still not merged.

Is there any chance to get it merged, either this PR or #1726?
That would be really great.

@jingxiaolu
Copy link
Contributor Author

@dongsupark many thanks for your comments~

#1726 does more checking, but it seems the author quits on the PR. Should we continue on this PR?

@wking
Copy link
Contributor

wking commented Jun 20, 2018

#1726 does more checking, but it seems the author quits on the PR.

What did I quit on? I'm not aware of any outstanding change requests short of this thread, where I'm waiting on maintainer follow-up.

@jingxiaolu
Copy link
Contributor Author

@wking sorry for that~ so let's wait for your pr~

@AkihiroSuda
Copy link
Member

needs rebase

@jingxiaolu
Copy link
Contributor Author

Thanks for remainding~ @AkihiroSuda

The part of spec_linux.go is already fix by 16d55f1, but the test in spec_linux_test.go may still has its value. Merge it if you think it is valuable 😄

@AkihiroSuda
Copy link
Member

AkihiroSuda commented Mar 10, 2020

LGTM

Approved with PullApprove

@jingxiaolu jingxiaolu force-pushed the fix_specconv_process_nil branch from 30b301b to 62cfad9 Compare March 10, 2020 03:46
@AkihiroSuda
Copy link
Member

AkihiroSuda commented Mar 16, 2020

LGTM

Approved with PullApprove

@AkihiroSuda
Copy link
Member

ping @opencontainers/runc-maintainers
This one is easy to review.

@mrunalp
Copy link
Contributor

mrunalp commented Mar 24, 2020

LGTM

Approved with PullApprove

@mrunalp mrunalp merged commit 3087d43 into opencontainers:master Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants