Audit usage of store.Inspect

[jandubois]

store.Inspect returns nil as long as the instance exists, and returns any other errors in inst.Errors:

lima/pkg/store/instance.go

Lines 65 to 66 in 9877e14

	// Inspect returns err only when the instance does not exist (os.ErrNotExist).
	// Other errors are returned as *Instance.Errors.

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)

lima/cmd/limactl/delete.go

Lines 37 to 45 in 9877e14

	inst, err := store.Inspect(instName)
	if err != nil {
	if errors.Is(err, os.ErrNotExist) {
	logrus.Warnf("Ignoring non-existent instance %q", instName)
	continue
	}
	return err
	}
	if err := instance.Delete(cmd.Context(), inst, force); err != nil {

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.

$ 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:

$ 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:

lima/pkg/limayaml/load.go

Lines 78 to 84 in 9877e14

	// It should be called before the `y` parameter is passed to FillDefault() that execute template.
	if err := ValidateParamIsUsed(&y); err != nil {
	return nil, err
	}
	FillDefault(&y, &d, &o, filePath)
	return &y, nil

[jandubois]

It also seems wrong that limayaml.Load() calls ValidateParamIsUsed() even though it explicitly states that it doesn't validate:

lima/pkg/limayaml/load.go

Lines 42 to 45 in 9877e14

	// Load loads the yaml and fulfills unspecified fields with the default values.
	//
	// Load does not validate. Use Validate for validation.
	func Load(b []byte, filePath string) (*LimaYAML, error) {

But there are various other errors that can happen while loading the instance data, so the need to change the store.Inspect API does not go away by moving this particular validation to load.Validate() (which is unfortunately not desirable for other reasons; I feel like we may eventually want to remove this particular check).