Skip to content
This repository has been archived by the owner on Feb 24, 2020. It is now read-only.

image: Validate docker url in dockerfetcher. #2134

Closed
wants to merge 1 commit into from

Conversation

ronin13
Copy link

@ronin13 ronin13 commented Feb 5, 2016

Passing an invalid docker url such as docker:run crashes rkt otherwise
with

sudo ./build-rkt-1.0.0+git/bin/rkt run docker:busybox
image: using image from file /usr/lib/rkt/stage1.aci
image: remote fetching from URL "docker:busybox"
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x0 pc=0x5e4314]

goroutine 1 [running, locked to thread]:
github.com/coreos/rkt/rkt/image.(*dockerFetcher).GetHash(0xc82033b2f0, 0xc820322080, 0x0, 0x0, 0x0, 0x0)
        /home/raghu/repo/go/src/github.com/coreos/rkt/build-rkt-1.0.0+git/gopath/src/github.com/coreos/rkt/rkt/image/dockerfetcher.go:54 +0x1f4
github.com/coreos/rkt/rkt/image.(*Fetcher).maybeFetchDockerURLFromRemote(0xc82033ba50, 0xc820322080, 0x0, 0x0, 0x0, 0x0)
        /home/raghu/repo/go/src/github.com/coreos/rkt/build-rkt-1.0.0+git/gopath/src/github.com/coreos/rkt/rkt/image/fetcher.go:249 +0x1ab
github.com/coreos/rkt/rkt/image.(*Fetcher).fetchSingleImageByDockerURL(0xc82033ba50, 0xc820322080, 0x0, 0x0, 0x0, 0x0)
        /home/raghu/repo/go/src/github.com/coreos/rkt/build-rkt-1.0.0+git/gopath/src/github.com/coreos/rkt/rkt/image/fetcher.go:187 +0x12b

Full trace:
https://gist.github.com/ronin13/bb82648c5d48ee68e808

This happens because ParseDockerURL output is not checked for nil.

With the fix it shows like:

sudo ./build-rkt-1.0.0+git/bin/rkt run --interactive docker:busybox
image: using image from local store for image name coreos.com/rkt/stage1-coreos:1.0.0+git6146a78
image: remote fetching from URL "docker:busybox"
run: Invalid docker URL

@@ -51,6 +51,9 @@ type dockerFetcher struct {
func (f *dockerFetcher) GetHash(u *url.URL) (string, error) {
ensureLogger(f.Debug)
dockerURL := d2acommon.ParseDockerURL(path.Join(u.Host, u.Path))
if dockerURL == nil {
return "", fmt.Errorf("Invalid docker URL")
Copy link
Member

Choose a reason for hiding this comment

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

We don't capitalize error strings.

Also, maybe we can help the user a bit more by showing the valid syntax:

fmt.Errorf("invalid docker URL %q. Syntax docker://[REGISTRYURL/]IMAGE_NAME[:TAG]", u)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, fixed it.

Passing an invalid docker url such as docker:run crashes rkt otherwise
with
```
sudo ./build-rkt-1.0.0+git/bin/rkt run docker:busybox
image: using image from file /usr/lib/rkt/stage1.aci
image: remote fetching from URL "docker:busybox"
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x0 pc=0x5e4314]

goroutine 1 [running, locked to thread]:
github.com/coreos/rkt/rkt/image.(*dockerFetcher).GetHash(0xc82033b2f0, 0xc820322080, 0x0, 0x0, 0x0, 0x0)
        /home/raghu/repo/go/src/github.com/coreos/rkt/build-rkt-1.0.0+git/gopath/src/github.com/coreos/rkt/rkt/image/dockerfetcher.go:54 +0x1f4
github.com/coreos/rkt/rkt/image.(*Fetcher).maybeFetchDockerURLFromRemote(0xc82033ba50, 0xc820322080, 0x0, 0x0, 0x0, 0x0)
        /home/raghu/repo/go/src/github.com/coreos/rkt/build-rkt-1.0.0+git/gopath/src/github.com/coreos/rkt/rkt/image/fetcher.go:249 +0x1ab
github.com/coreos/rkt/rkt/image.(*Fetcher).fetchSingleImageByDockerURL(0xc82033ba50, 0xc820322080, 0x0, 0x0, 0x0, 0x0)
        /home/raghu/repo/go/src/github.com/coreos/rkt/build-rkt-1.0.0+git/gopath/src/github.com/coreos/rkt/rkt/image/fetcher.go:187 +0x12b
```

Full trace:
https://gist.github.com/ronin13/bb82648c5d48ee68e808

This happens because ParseDockerURL output is not checked for nil.

With the fix it shows like:
```
sudo ./build-rkt-1.0.0+git/bin/rkt run --interactive docker:busybox
image: using image from local store for image name coreos.com/rkt/stage1-coreos:1.0.0+git6146a78
image: remote fetching from URL "docker:busybox"
run: Invalid docker URL
```
@krnowak
Copy link
Collaborator

krnowak commented Feb 8, 2016

Maybe docker2aci should be changed to return an error here?

Returning nil as a sign of an error is rather confusing for a developer (like me) that is used to receive explicit error return.

@jonboulle
Copy link
Contributor

+1 to @krnowak

@iaguis
Copy link
Member

iaguis commented Feb 8, 2016

Alright: appc/docker2aci#119

iaguis added a commit to kinvolk/rkt that referenced this pull request Feb 19, 2016
@iaguis iaguis mentioned this pull request Feb 19, 2016
iaguis added a commit to kinvolk/rkt that referenced this pull request Feb 19, 2016
iaguis added a commit to kinvolk/rkt that referenced this pull request Feb 19, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants