lima-vm / lima-vm/lima

Audit usage of `store.Inspect`

Open
#2,668 1 comment 0 reactions 0 assignees View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.