Skip to content
This repository has been archived by the owner on Nov 8, 2022. It is now read-only.

snap panics on empty config sections #874

Closed
nanliu opened this issue Apr 22, 2016 · 5 comments
Closed

snap panics on empty config sections #874

nanliu opened this issue Apr 22, 2016 · 5 comments

Comments

@nanliu
Copy link
Contributor

nanliu commented Apr 22, 2016

If you have an empty section in snapd.conf.yaml, snapd will panic:
such as:

---
scheduler:

Error:

snapd --config /etc/snap/snapd.conf.yaml
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x0 pc=0x40b08f]

goroutine 1 [running]:
panic(0xb28b20, 0xc82000e0d0)
    /usr/local/Cellar/go/1.6.1/libexec/src/runtime/panic.go:464 +0x3e6
main.applyCmdLineFlags(0xc8201eeac0, 0xc8200ddb80)
    /Users/nanliu/src/intelsdi-x/snap_packaging/artifacts/src/github.com/intelsdi-x/snap/snapd.go:627 +0x66f
main.action(0xc8200ddb80)
    /Users/nanliu/src/intelsdi-x/snap_packaging/artifacts/src/github.com/intelsdi-x/snap/snapd.go:222 +0xe3
github.com/codegangsta/cli.(*App).Run(0xc8200ba180, 0xc82000a120, 0x3, 0x3, 0x0, 0x0)
    /Users/nanliu/src/intelsdi-x/snap_packaging/artifacts/src/github.com/codegangsta/cli/app.go:130 +0xadb
main.main()
    /Users/nanliu/src/intelsdi-x/snap_packaging/artifacts/src/github.com/intelsdi-x/snap/snapd.go:208 +0xa8e
@geauxvirtual
Copy link
Contributor

We should handle this better, but with YAML, if nothing will be in a config section, it can be commented out completely or removed from the file.

Also, there is no need to specify .yaml or .json behind the default configuration file. snapd will automatically pick up a configuration file located with the name /etc/snap/snapd.conf whether it is JSON or YAML without needing to specify the --config flag.

@IRCody
Copy link
Contributor

IRCody commented Apr 22, 2016

I don't think any config file should be able to make us panic, valid or invalid.

@tjmcs
Copy link
Contributor

tjmcs commented Apr 22, 2016

Agreed, @IRCody. I am currently looking into this issue; I think it's the fact that those "empty" sections map to nil values; stay tuned...

@geauxvirtual
Copy link
Contributor

The problem is in how JSON unmarhalling happens (yes, JSON is used for the YAML file). Even if a section is labeled as omitempty in the code, having scheduler: appear in the file doesn't omit it because it's empty. It's only omitted if it doesn't appear in the file. Specific unmarshalling code would need to be added to check for if the element exists but is empty.

@tjmcs
Copy link
Contributor

tjmcs commented Apr 22, 2016

Yeah, that was my assumption as well, @geauxvirtual; I'll dig into it a bit more and see what it would take to check for empty fields after the configuration file is parsed (skipping those in the merge process?). As @IRCody said I really don't think that a valid YAML (or JSON) file should cause a panic, so even though we can get around it by commenting that section out we should fix this issue.

jcooklin added a commit that referenced this issue Apr 30, 2016
Fixes #874 (snap panics on empty config sections; take 2)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants