Audit usage of `store.Inspect`
- Dominant language
- Go
- Stars
- 21.9k
- Forks
- 957
- Avg merge
- 2d 6h
- Merged PRs (30d)
- 53
Description
`store.Inspect` returns `nil` as long as the instance exists, and returns any other errors in `inst.Errors`: https://github.com/lima-vm/lima/blob/9877e145d1cdb995accd60253bf5227591256df4/pkg/store/instance.go#L65-L66
However, most call sites just check the returned `err`, and then use the returned instance data without looking at `inst.Errors`, e.g. (but there are plenty more) https://github.com/lima-vm/lima/blob/9877e145d1cdb995accd60253bf5227591256df4/cmd/limactl/delete.go#L37-L45
Note how the caller checks again if the returned error is `os.ErrNotExist` even though that is the only non-nil value that the function can return.
I think this shows that this API is not safe to use, and we should always return non-nil in the error case, and provide a different way to quickly check if the error is `os.ErrNotExist`, e.g. by adding an `inst.Exist()` predicate.
Using an `inst` that has errors means `FillDefaults` may not have been called, and code that relies on all the pointers inside the `LimaYAML` struct being non-nil can panic, e.g.
```console
$ limactl delete -f 0
INFO[0000] The driver process seems already stopped
INFO[0000] The host agent process seems already stopped
INFO[0000] Removing *.pid *.sock *.tmp under "/Users/jan/Library/Application Support/rancher-desktop/lima/0"
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x1109105bc]
goroutine 1 [running]:
github.com/lima-vm/lima/pkg/driverutil.CreateTargetDriverInstance(...)
/Users/jan/suse/lima/pkg/driverutil/instance.go:12
github.com/lima-vm/lima/pkg/instance.unregister({0x110f1c148, 0x111dcd2c0}, 0xc000429b80)
/Users/jan/suse/lima/pkg/instance/delete.go:35 +0x5c
```
Note how the output above doesn't even show the errors from loading the instance. You can see it here:
```console
$ limactl ls 0
WARN[0000] instance "0" has errors errors="[field `param` key \"CONTAINER_ENGINE\" is not used in any provision, probe, copyToHost, or portForward]"
NAME STATUS SSH VMTYPE ARCH CPUS MEMORY DISK DIR
0 :0 0 0B 0B ~/Library/Application Support/rancher-desktop/lima/0
```
So the `nil` de-reference above comes from `vmType` not having been filled in with the default: https://github.com/lima-vm/lima/blob/9877e145d1cdb995accd60253bf5227591256df4/pkg/limayaml/load.go#L78-L84
Contributor guide
Assessment
This issue has not been assessed yet.