From 6e00fa690db9eb9e0b88ba4380983911e7a343e5 Mon Sep 17 00:00:00 2001 From: Thales Maciel Date: Mon, 16 Mar 2026 16:28:17 -0300 Subject: [PATCH] Reject invalid VM CPU and memory values VM create and vm set accepted zero or negative CPU and memory values, which either got stored directly or silently fell back to defaults and only surfaced as failures later. This tightens validation so bad settings are rejected at the user boundary and again in the daemon before any VM record is persisted. Change vm.create CPU and memory request fields to optional pointers so omitted values still mean defaults, while explicit non-positive values can be distinguished and rejected. Update Cobra create/set parsing, keep the TUI aligned with the new API shape, and add regression tests for CLI parsing, daemon-side validation, and the create-defaults path. Validation: go test ./... and make build. Left my-rootfs.ext4 untracked. --- internal/api/types.go | 4 +-- internal/cli/banger.go | 68 +++++++++++++++++++++++++++++++++----- internal/cli/cli_test.go | 53 +++++++++++++++++++++++++++++ internal/cli/tui.go | 4 +-- internal/cli/tui_test.go | 2 +- internal/daemon/vm.go | 32 +++++++++++++++--- internal/daemon/vm_test.go | 58 ++++++++++++++++++++++++++++++++ 7 files changed, 202 insertions(+), 19 deletions(-) diff --git a/internal/api/types.go b/internal/api/types.go index 22ad6a8..eb9ba2c 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -16,8 +16,8 @@ type ShutdownResult struct { type VMCreateParams struct { Name string `json:"name,omitempty"` ImageName string `json:"image_name,omitempty"` - VCPUCount int `json:"vcpu_count,omitempty"` - MemoryMiB int `json:"memory_mib,omitempty"` + VCPUCount *int `json:"vcpu_count,omitempty"` + MemoryMiB *int `json:"memory_mib,omitempty"` SystemOverlaySize string `json:"system_overlay_size,omitempty"` WorkDiskSize string `json:"work_disk_size,omitempty"` NATEnabled bool `json:"nat_enabled,omitempty"` diff --git a/internal/cli/banger.go b/internal/cli/banger.go index 35a16e9..8eb9550 100644 --- a/internal/cli/banger.go +++ b/internal/cli/banger.go @@ -163,12 +163,25 @@ func newVMKillCommand() *cobra.Command { } func newVMCreateCommand() *cobra.Command { - var params api.VMCreateParams + var ( + name string + imageName string + vcpu int + memory int + systemOverlaySize string + workDiskSize string + natEnabled bool + noStart bool + ) cmd := &cobra.Command{ Use: "create", Short: "Create a VM", Args: noArgsUsage("usage: banger vm create"), RunE: func(cmd *cobra.Command, args []string) error { + params, err := vmCreateParamsFromFlags(cmd, name, imageName, vcpu, memory, systemOverlaySize, workDiskSize, natEnabled, noStart) + if err != nil { + return err + } if err := system.EnsureSudo(cmd.Context()); err != nil { return err } @@ -183,14 +196,14 @@ func newVMCreateCommand() *cobra.Command { return printVMSummary(cmd.OutOrStdout(), result.VM) }, } - cmd.Flags().StringVar(¶ms.Name, "name", "", "vm name") - cmd.Flags().StringVar(¶ms.ImageName, "image", "", "image name or id") - cmd.Flags().IntVar(¶ms.VCPUCount, "vcpu", 0, "vcpu count") - cmd.Flags().IntVar(¶ms.MemoryMiB, "memory", 0, "memory in MiB") - cmd.Flags().StringVar(¶ms.SystemOverlaySize, "system-overlay-size", "", "system overlay size") - cmd.Flags().StringVar(¶ms.WorkDiskSize, "disk-size", "", "work disk size") - cmd.Flags().BoolVar(¶ms.NATEnabled, "nat", false, "enable NAT") - cmd.Flags().BoolVar(¶ms.NoStart, "no-start", false, "create without starting") + cmd.Flags().StringVar(&name, "name", "", "vm name") + cmd.Flags().StringVar(&imageName, "image", "", "image name or id") + cmd.Flags().IntVar(&vcpu, "vcpu", 0, "vcpu count") + cmd.Flags().IntVar(&memory, "memory", 0, "memory in MiB") + cmd.Flags().StringVar(&systemOverlaySize, "system-overlay-size", "", "system overlay size") + cmd.Flags().StringVar(&workDiskSize, "disk-size", "", "work disk size") + cmd.Flags().BoolVar(&natEnabled, "nat", false, "enable NAT") + cmd.Flags().BoolVar(&noStart, "no-start", false, "create without starting") return cmd } @@ -648,9 +661,15 @@ func vmSetParamsFromFlags(idOrName string, vcpu, memory int, diskSize string, na } params := api.VMSetParams{IDOrName: idOrName, WorkDiskSize: diskSize} if vcpu >= 0 { + if err := validatePositiveSetting("vcpu", vcpu); err != nil { + return api.VMSetParams{}, err + } params.VCPUCount = &vcpu } if memory >= 0 { + if err := validatePositiveSetting("memory", memory); err != nil { + return api.VMSetParams{}, err + } params.MemoryMiB = &memory } if nat || noNat { @@ -663,6 +682,37 @@ func vmSetParamsFromFlags(idOrName string, vcpu, memory int, diskSize string, na return params, nil } +func vmCreateParamsFromFlags(cmd *cobra.Command, name, imageName string, vcpu, memory int, systemOverlaySize, workDiskSize string, natEnabled, noStart bool) (api.VMCreateParams, error) { + params := api.VMCreateParams{ + Name: name, + ImageName: imageName, + SystemOverlaySize: systemOverlaySize, + WorkDiskSize: workDiskSize, + NATEnabled: natEnabled, + NoStart: noStart, + } + if cmd.Flags().Changed("vcpu") { + if err := validatePositiveSetting("vcpu", vcpu); err != nil { + return api.VMCreateParams{}, err + } + params.VCPUCount = &vcpu + } + if cmd.Flags().Changed("memory") { + if err := validatePositiveSetting("memory", memory); err != nil { + return api.VMCreateParams{}, err + } + params.MemoryMiB = &memory + } + return params, nil +} + +func validatePositiveSetting(label string, value int) error { + if value <= 0 { + return fmt.Errorf("%s must be a positive integer", label) + } + return nil +} + func sshCommandArgs(cfg model.DaemonConfig, guestIP string, extra []string) ([]string, error) { if guestIP == "" { return nil, errors.New("vm has no guest IP") diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 6c7e5d7..6a25834 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -75,12 +75,65 @@ func TestVMSetParamsFromFlags(t *testing.T) { } } +func TestVMCreateParamsFromFlagsUsesNilForOmittedCPUAndMemory(t *testing.T) { + cmd := NewBangerCommand() + vm, _, err := cmd.Find([]string{"vm"}) + if err != nil { + t.Fatalf("find vm: %v", err) + } + create, _, err := vm.Find([]string{"create"}) + if err != nil { + t.Fatalf("find create: %v", err) + } + + params, err := vmCreateParamsFromFlags(create, "devbox", "default", 0, 0, "8G", "16G", false, false) + if err != nil { + t.Fatalf("vmCreateParamsFromFlags: %v", err) + } + if params.VCPUCount != nil || params.MemoryMiB != nil { + t.Fatalf("expected omitted cpu/memory to stay nil: %+v", params) + } +} + +func TestVMCreateParamsFromFlagsRejectsNonPositiveCPUAndMemory(t *testing.T) { + cmd := NewBangerCommand() + vm, _, err := cmd.Find([]string{"vm"}) + if err != nil { + t.Fatalf("find vm: %v", err) + } + create, _, err := vm.Find([]string{"create"}) + if err != nil { + t.Fatalf("find create: %v", err) + } + if err := create.Flags().Set("vcpu", "0"); err != nil { + t.Fatalf("set vcpu flag: %v", err) + } + if _, err := vmCreateParamsFromFlags(create, "devbox", "default", 0, 0, "", "", false, false); err == nil || !strings.Contains(err.Error(), "vcpu must be a positive integer") { + t.Fatalf("vmCreateParamsFromFlags(vcpu=0) error = %v", err) + } + if err := create.Flags().Set("memory", "-1"); err != nil { + t.Fatalf("set memory flag: %v", err) + } + if _, err := vmCreateParamsFromFlags(create, "devbox", "default", 1, -1, "", "", false, false); err == nil || !strings.Contains(err.Error(), "memory must be a positive integer") { + t.Fatalf("vmCreateParamsFromFlags(memory=-1) error = %v", err) + } +} + func TestVMSetParamsFromFlagsConflict(t *testing.T) { if _, err := vmSetParamsFromFlags("devbox", -1, -1, "", true, true); err == nil { t.Fatal("expected nat conflict error") } } +func TestVMSetParamsFromFlagsRejectsNonPositiveCPUAndMemory(t *testing.T) { + if _, err := vmSetParamsFromFlags("devbox", 0, -1, "", false, false); err == nil || !strings.Contains(err.Error(), "vcpu must be a positive integer") { + t.Fatalf("vmSetParamsFromFlags(vcpu=0) error = %v", err) + } + if _, err := vmSetParamsFromFlags("devbox", -1, 0, "", false, false); err == nil || !strings.Contains(err.Error(), "memory must be a positive integer") { + t.Fatalf("vmSetParamsFromFlags(memory=0) error = %v", err) + } +} + func TestSSHCommandArgs(t *testing.T) { args, err := sshCommandArgs(model.DaemonConfig{SSHKeyPath: "/bundle/id_ed25519"}, "172.16.0.2", []string{"--", "uname", "-a"}) if err != nil { diff --git a/internal/cli/tui.go b/internal/cli/tui.go index 6f6e1f8..67a9f13 100644 --- a/internal/cli/tui.go +++ b/internal/cli/tui.go @@ -399,8 +399,8 @@ func (f *vmForm) createRequest() (actionRequest, error) { params := api.VMCreateParams{ Name: strings.TrimSpace(f.fields[0].value()), ImageName: strings.TrimSpace(f.fields[1].value()), - VCPUCount: vcpu, - MemoryMiB: memory, + VCPUCount: &vcpu, + MemoryMiB: &memory, SystemOverlaySize: strings.TrimSpace(f.fields[4].value()), WorkDiskSize: strings.TrimSpace(f.fields[5].value()), NATEnabled: isYes(f.fields[6].value()), diff --git a/internal/cli/tui_test.go b/internal/cli/tui_test.go index 18e37f6..092c48f 100644 --- a/internal/cli/tui_test.go +++ b/internal/cli/tui_test.go @@ -25,7 +25,7 @@ func TestCreateVMFormSubmit(t *testing.T) { if action.create.Name != "devbox" || action.create.ImageName != "default" { t.Fatalf("unexpected create params: %+v", action.create) } - if action.create.VCPUCount != 4 || action.create.MemoryMiB != 2048 { + if action.create.VCPUCount == nil || *action.create.VCPUCount != 4 || action.create.MemoryMiB == nil || *action.create.MemoryMiB != 2048 { t.Fatalf("unexpected cpu/memory: %+v", action.create) } if action.create.SystemOverlaySize != "12G" || action.create.WorkDiskSize != "24G" { diff --git a/internal/daemon/vm.go b/internal/daemon/vm.go index 21f9b2d..27978be 100644 --- a/internal/daemon/vm.go +++ b/internal/daemon/vm.go @@ -28,6 +28,12 @@ func (d *Daemon) CreateVM(ctx context.Context, params api.VMCreateParams) (vm mo } op.done(vmLogAttrs(vm)...) }() + if err := validateOptionalPositiveSetting("vcpu", params.VCPUCount); err != nil { + return model.VMRecord{}, err + } + if err := validateOptionalPositiveSetting("memory", params.MemoryMiB); err != nil { + return model.VMRecord{}, err + } imageName := params.ImageName if imageName == "" { @@ -76,8 +82,8 @@ func (d *Daemon) CreateVM(ctx context.Context, params api.VMCreateParams) (vm mo } now := model.Now() spec := model.VMSpec{ - VCPUCount: defaultInt(params.VCPUCount, model.DefaultVCPUCount), - MemoryMiB: defaultInt(params.MemoryMiB, model.DefaultMemoryMiB), + VCPUCount: optionalIntOrDefault(params.VCPUCount, model.DefaultVCPUCount), + MemoryMiB: optionalIntOrDefault(params.MemoryMiB, model.DefaultMemoryMiB), SystemOverlaySizeByte: systemOverlaySize, WorkDiskSizeBytes: workDiskSize, NATEnabled: params.NATEnabled, @@ -462,6 +468,9 @@ func (d *Daemon) SetVM(ctx context.Context, params api.VMSetParams) (vm model.VM } running := vm.State == model.VMStateRunning && system.ProcessRunning(vm.Runtime.PID, vm.Runtime.APISockPath) if params.VCPUCount != nil { + if err := validateOptionalPositiveSetting("vcpu", params.VCPUCount); err != nil { + return model.VMRecord{}, err + } if running { return model.VMRecord{}, errors.New("vcpu changes require the VM to be stopped") } @@ -469,6 +478,9 @@ func (d *Daemon) SetVM(ctx context.Context, params api.VMSetParams) (vm model.VM vm.Spec.VCPUCount = *params.VCPUCount } if params.MemoryMiB != nil { + if err := validateOptionalPositiveSetting("memory", params.MemoryMiB); err != nil { + return model.VMRecord{}, err + } if running { return model.VMRecord{}, errors.New("memory changes require the VM to be stopped") } @@ -864,13 +876,23 @@ func bridgePrefix(bridgeIP string) string { return strings.Join(parts[:3], ".") } -func defaultInt(value, fallback int) int { - if value > 0 { - return value +func optionalIntOrDefault(value *int, fallback int) int { + if value != nil { + return *value } return fallback } +func validateOptionalPositiveSetting(label string, value *int) error { + if value == nil { + return nil + } + if *value <= 0 { + return fmt.Errorf("%s must be a positive integer", label) + } + return nil +} + func (d *Daemon) mapdnsBinary() string { if value := strings.TrimSpace(d.config.MapDNSBin); value != "" { return value diff --git a/internal/daemon/vm_test.go b/internal/daemon/vm_test.go index 9e0f9e9..098fc17 100644 --- a/internal/daemon/vm_test.go +++ b/internal/daemon/vm_test.go @@ -13,6 +13,7 @@ import ( "banger/internal/api" "banger/internal/model" + "banger/internal/paths" "banger/internal/store" ) @@ -218,6 +219,63 @@ func TestSetVMDiskResizeFailsPreflightWhenToolsMissing(t *testing.T) { } } +func TestCreateVMRejectsNonPositiveCPUAndMemory(t *testing.T) { + d := &Daemon{} + if _, err := d.CreateVM(context.Background(), api.VMCreateParams{VCPUCount: ptr(0)}); err == nil || !strings.Contains(err.Error(), "vcpu must be a positive integer") { + t.Fatalf("CreateVM(vcpu=0) error = %v", err) + } + if _, err := d.CreateVM(context.Background(), api.VMCreateParams{MemoryMiB: ptr(-1)}); err == nil || !strings.Contains(err.Error(), "memory must be a positive integer") { + t.Fatalf("CreateVM(memory=-1) error = %v", err) + } +} + +func TestCreateVMUsesDefaultsWhenCPUAndMemoryOmitted(t *testing.T) { + ctx := context.Background() + db := openDaemonStore(t) + image := testImage("default") + if err := db.UpsertImage(ctx, image); err != nil { + t.Fatalf("UpsertImage: %v", err) + } + d := &Daemon{ + store: db, + layout: paths.Layout{ + VMsDir: t.TempDir(), + }, + config: model.DaemonConfig{ + DefaultImageName: image.Name, + BridgeIP: model.DefaultBridgeIP, + }, + } + + vm, err := d.CreateVM(ctx, api.VMCreateParams{Name: "defaults", ImageName: image.Name, NoStart: true}) + if err != nil { + t.Fatalf("CreateVM: %v", err) + } + if vm.Spec.VCPUCount != model.DefaultVCPUCount { + t.Fatalf("VCPUCount = %d, want %d", vm.Spec.VCPUCount, model.DefaultVCPUCount) + } + if vm.Spec.MemoryMiB != model.DefaultMemoryMiB { + t.Fatalf("MemoryMiB = %d, want %d", vm.Spec.MemoryMiB, model.DefaultMemoryMiB) + } +} + +func TestSetVMRejectsNonPositiveCPUAndMemory(t *testing.T) { + ctx := context.Background() + db := openDaemonStore(t) + vm := testVM("validate", "image-validate", "172.16.0.13") + if err := db.UpsertVM(ctx, vm); err != nil { + t.Fatalf("UpsertVM: %v", err) + } + d := &Daemon{store: db} + + if _, err := d.SetVM(ctx, api.VMSetParams{IDOrName: vm.ID, VCPUCount: ptr(0)}); err == nil || !strings.Contains(err.Error(), "vcpu must be a positive integer") { + t.Fatalf("SetVM(vcpu=0) error = %v", err) + } + if _, err := d.SetVM(ctx, api.VMSetParams{IDOrName: vm.ID, MemoryMiB: ptr(0)}); err == nil || !strings.Contains(err.Error(), "memory must be a positive integer") { + t.Fatalf("SetVM(memory=0) error = %v", err) + } +} + func TestCollectStatsIgnoresMalformedMetricsFile(t *testing.T) { t.Parallel()