From f1ee61b10db4f9add8aa44ad4f6929e783885040 Mon Sep 17 00:00:00 2001 From: goyalpalak18 Date: Mon, 26 Jan 2026 23:32:28 +0530 Subject: [PATCH 1/2] fix: pre-build VMM command before reporting container as started Add BuildExecCmd() method to VMM interface to validate command construction before sending StartSuccess message. This prevents reporting a container as "Running" when the VMM command cannot be built (e.g., due to invalid arguments or failed config writes). The fix preserves the existing PID 1 execution model where the reexec process uses syscall.Exec to become the VMM. --- pkg/unikontainers/hypervisors/firecracker.go | 13 ++++++++++--- pkg/unikontainers/hypervisors/hedge.go | 4 ++++ pkg/unikontainers/hypervisors/hvt.go | 12 ++++++++++-- pkg/unikontainers/hypervisors/qemu.go | 10 +++++++++- pkg/unikontainers/hypervisors/spt.go | 12 ++++++++++-- pkg/unikontainers/types/types.go | 4 ++++ pkg/unikontainers/unikontainers.go | 18 ++++++++++++------ 7 files changed, 59 insertions(+), 14 deletions(-) diff --git a/pkg/unikontainers/hypervisors/firecracker.go b/pkg/unikontainers/hypervisors/firecracker.go index a1ced99b..a2849d3b 100644 --- a/pkg/unikontainers/hypervisors/firecracker.go +++ b/pkg/unikontainers/hypervisors/firecracker.go @@ -97,7 +97,7 @@ func (fc *Firecracker) Path() string { return fc.binaryPath } -func (fc *Firecracker) Execve(args types.ExecArgs, ukernel types.Unikernel) error { +func (fc *Firecracker) BuildExecCmd(args types.ExecArgs, ukernel types.Unikernel) ([]string, error) { // FIXME: Note for getting unikernel specific options. // Due to the way FC operates, we have not encountered any guest specific // options yet. However, we need to revisit how we can use guest specific @@ -189,12 +189,19 @@ func (fc *Firecracker) Execve(args types.ExecArgs, ukernel types.Unikernel) erro } FCConfigJSON, _ := json.Marshal(FCConfig) if err := os.WriteFile(JSONConfigFile, FCConfigJSON, 0o644); err != nil { //nolint: gosec - return fmt.Errorf("failed to save Firecracker json config: %w", err) + return nil, fmt.Errorf("failed to save Firecracker json config: %w", err) } vmmLog.WithField("Json", string(FCConfigJSON)).Debug("Firecracker json config") exArgs := strings.Split(cmdString, " ") - vmmLog.WithField("Firecracker command", exArgs).Debug("Ready to execve Firecracker") + return exArgs, nil +} +func (fc *Firecracker) Execve(args types.ExecArgs, ukernel types.Unikernel) error { + exArgs, err := fc.BuildExecCmd(args, ukernel) + if err != nil { + return err + } + vmmLog.WithField("Firecracker command", exArgs).Debug("Ready to execve Firecracker") return syscall.Exec(fc.Path(), exArgs, args.Environment) //nolint: gosec } diff --git a/pkg/unikontainers/hypervisors/hedge.go b/pkg/unikontainers/hypervisors/hedge.go index 436b4786..3a734664 100644 --- a/pkg/unikontainers/hypervisors/hedge.go +++ b/pkg/unikontainers/hypervisors/hedge.go @@ -50,6 +50,10 @@ func (h *Hedge) Path() string { return "" } +func (h *Hedge) BuildExecCmd(_ types.ExecArgs, _ types.Unikernel) ([]string, error) { + return nil, fmt.Errorf("hedge not implemented yet") +} + func (h *Hedge) Execve(_ types.ExecArgs, _ types.Unikernel) error { return fmt.Errorf("hedge not implemented yet") } diff --git a/pkg/unikontainers/hypervisors/hvt.go b/pkg/unikontainers/hypervisors/hvt.go index ff1aa821..fcb5e3be 100644 --- a/pkg/unikontainers/hypervisors/hvt.go +++ b/pkg/unikontainers/hypervisors/hvt.go @@ -148,7 +148,7 @@ func (h *HVT) Ok() error { return nil } -func (h *HVT) Execve(args types.ExecArgs, ukernel types.Unikernel) error { +func (h *HVT) BuildExecCmd(args types.ExecArgs, ukernel types.Unikernel) ([]string, error) { hvtMem := BytesToStringMB(args.MemSizeB) cmdString := h.binaryPath + " --mem=" + hvtMem if args.Net.TapDev != "" { @@ -164,12 +164,20 @@ func (h *HVT) Execve(args types.ExecArgs, ukernel types.Unikernel) error { cmdString = appendNonEmpty(cmdString, " ", extraMonArgs.OtherArgs) cmdString += " " + args.UnikernelPath + " " + args.Command cmdArgs := strings.Split(cmdString, " ") + return cmdArgs, nil +} + +func (h *HVT) Execve(args types.ExecArgs, ukernel types.Unikernel) error { + cmdArgs, err := h.BuildExecCmd(args, ukernel) + if err != nil { + return err + } if args.Seccomp { err := applySeccompFilter() if err != nil { return err } } - vmmLog.WithField("hvt command", cmdString).Debug("Ready to execve hvt") + vmmLog.WithField("hvt command", cmdArgs).Debug("Ready to execve hvt") return syscall.Exec(h.binaryPath, cmdArgs, args.Environment) //nolint: gosec } diff --git a/pkg/unikontainers/hypervisors/qemu.go b/pkg/unikontainers/hypervisors/qemu.go index 3283df13..e67e6c1d 100644 --- a/pkg/unikontainers/hypervisors/qemu.go +++ b/pkg/unikontainers/hypervisors/qemu.go @@ -55,7 +55,7 @@ func (q *Qemu) Path() string { return q.binaryPath } -func (q *Qemu) Execve(args types.ExecArgs, ukernel types.Unikernel) error { +func (q *Qemu) BuildExecCmd(args types.ExecArgs, ukernel types.Unikernel) ([]string, error) { qemuMem := BytesToStringMB(args.MemSizeB) cmdString := q.binaryPath + " -m " + qemuMem + "M" cmdString += " -L /usr/share/qemu" // Set the path for qemu bios/data @@ -137,6 +137,14 @@ func (q *Qemu) Execve(args types.ExecArgs, ukernel types.Unikernel) error { exArgs := strings.Split(cmdString, " ") exArgs = append(exArgs, "-append", args.Command) + return exArgs, nil +} + +func (q *Qemu) Execve(args types.ExecArgs, ukernel types.Unikernel) error { + exArgs, err := q.BuildExecCmd(args, ukernel) + if err != nil { + return err + } vmmLog.WithField("qemu command", exArgs).Debug("Ready to execve qemu") return syscall.Exec(q.Path(), exArgs, args.Environment) //nolint: gosec } diff --git a/pkg/unikontainers/hypervisors/spt.go b/pkg/unikontainers/hypervisors/spt.go index d99fd3dd..f4bfa30b 100644 --- a/pkg/unikontainers/hypervisors/spt.go +++ b/pkg/unikontainers/hypervisors/spt.go @@ -60,7 +60,7 @@ func (s *SPT) Ok() error { return nil } -func (s *SPT) Execve(args types.ExecArgs, ukernel types.Unikernel) error { +func (s *SPT) BuildExecCmd(args types.ExecArgs, ukernel types.Unikernel) ([]string, error) { sptMem := BytesToStringMB(args.MemSizeB) cmdString := s.binaryPath + " --mem=" + sptMem if args.Net.TapDev != "" { @@ -76,6 +76,14 @@ func (s *SPT) Execve(args types.ExecArgs, ukernel types.Unikernel) error { cmdString = appendNonEmpty(cmdString, " ", extraMonArgs.OtherArgs) cmdString += " " + args.UnikernelPath + " " + args.Command cmdArgs := strings.Split(cmdString, " ") - vmmLog.WithField("spt command", cmdString).Debug("Ready to execve spt") + return cmdArgs, nil +} + +func (s *SPT) Execve(args types.ExecArgs, ukernel types.Unikernel) error { + cmdArgs, err := s.BuildExecCmd(args, ukernel) + if err != nil { + return err + } + vmmLog.WithField("spt command", cmdArgs).Debug("Ready to execve spt") return syscall.Exec(s.binaryPath, cmdArgs, args.Environment) //nolint: gosec } diff --git a/pkg/unikontainers/types/types.go b/pkg/unikontainers/types/types.go index 3f434e51..154a34ce 100644 --- a/pkg/unikontainers/types/types.go +++ b/pkg/unikontainers/types/types.go @@ -25,6 +25,10 @@ type Unikernel interface { } type VMM interface { + // BuildExecCmd builds and validates the VMM command arguments without executing. + // This is used to verify the command can be built before reporting container as started. + BuildExecCmd(args ExecArgs, ukernel Unikernel) ([]string, error) + // Execve replaces the current process with the VMM using syscall.Exec. Execve(args ExecArgs, ukernel Unikernel) error Stop(int) error Path() string diff --git a/pkg/unikontainers/unikontainers.go b/pkg/unikontainers/unikontainers.go index 85dc1e2a..3e84997b 100644 --- a/pkg/unikontainers/unikontainers.go +++ b/pkg/unikontainers/unikontainers.go @@ -493,12 +493,18 @@ func (u *Unikontainer) Exec(metrics m.Writer) error { uniklog.Debug("calling vmm execve") metrics.Capture(m.TS18) - // metrics.Wait() - // TODO: We set the state to running and notify urunc Start that the monitor - // started, but we might encounter issues with the monitor execution. We need - // to revisit this and check if a failed monitor execution affects this approach. - // If it affects then we need to re-design the whole spawning of the monitor. - // Notify urunc start + + // Pre-build the VMM command to verify it can be constructed successfully. + // This ensures we don't report the container as started if command building fails. + _, err = vmm.BuildExecCmd(vmmArgs, unikernel) + if err != nil { + uniklog.WithError(err).Error("failed to build VMM command") + return err + } + + // Notify urunc start that the monitor is ready to execute. + // We send this after BuildExecCmd succeeds to avoid reporting a container + // as started when the VMM command cannot be built. err = u.SendMessage(StartSuccess) if err != nil { return err From b7283212fff9150046b33478e05f1f606894cbf0 Mon Sep 17 00:00:00 2001 From: goyalpalak18 Date: Tue, 27 Jan 2026 15:21:39 +0530 Subject: [PATCH 2/2] refactor: centralize Exec logic and remove redundant command building --- pkg/unikontainers/hypervisors/firecracker.go | 11 +++-------- pkg/unikontainers/hypervisors/hedge.go | 3 ++- pkg/unikontainers/hypervisors/hvt.go | 15 +++++---------- pkg/unikontainers/hypervisors/qemu.go | 11 +++-------- pkg/unikontainers/hypervisors/spt.go | 11 +++-------- pkg/unikontainers/types/types.go | 7 +++++-- pkg/unikontainers/unikontainers.go | 15 ++++++++++++--- 7 files changed, 33 insertions(+), 40 deletions(-) diff --git a/pkg/unikontainers/hypervisors/firecracker.go b/pkg/unikontainers/hypervisors/firecracker.go index a2849d3b..2787a813 100644 --- a/pkg/unikontainers/hypervisors/firecracker.go +++ b/pkg/unikontainers/hypervisors/firecracker.go @@ -20,7 +20,6 @@ import ( "os" "path/filepath" "strings" - "syscall" "github.com/urunc-dev/urunc/pkg/unikontainers/types" ) @@ -197,11 +196,7 @@ func (fc *Firecracker) BuildExecCmd(args types.ExecArgs, ukernel types.Unikernel return exArgs, nil } -func (fc *Firecracker) Execve(args types.ExecArgs, ukernel types.Unikernel) error { - exArgs, err := fc.BuildExecCmd(args, ukernel) - if err != nil { - return err - } - vmmLog.WithField("Firecracker command", exArgs).Debug("Ready to execve Firecracker") - return syscall.Exec(fc.Path(), exArgs, args.Environment) //nolint: gosec +// PreExec performs pre-execution setup. Firecracker has no special pre-exec requirements. +func (fc *Firecracker) PreExec(_ types.ExecArgs) error { + return nil } diff --git a/pkg/unikontainers/hypervisors/hedge.go b/pkg/unikontainers/hypervisors/hedge.go index 3a734664..329f82db 100644 --- a/pkg/unikontainers/hypervisors/hedge.go +++ b/pkg/unikontainers/hypervisors/hedge.go @@ -54,7 +54,8 @@ func (h *Hedge) BuildExecCmd(_ types.ExecArgs, _ types.Unikernel) ([]string, err return nil, fmt.Errorf("hedge not implemented yet") } -func (h *Hedge) Execve(_ types.ExecArgs, _ types.Unikernel) error { +// PreExec performs pre-execution setup. Hedge is not fully implemented. +func (h *Hedge) PreExec(_ types.ExecArgs) error { return fmt.Errorf("hedge not implemented yet") } diff --git a/pkg/unikontainers/hypervisors/hvt.go b/pkg/unikontainers/hypervisors/hvt.go index fcb5e3be..5b25d425 100644 --- a/pkg/unikontainers/hypervisors/hvt.go +++ b/pkg/unikontainers/hypervisors/hvt.go @@ -18,7 +18,6 @@ import ( "os/exec" "runtime" "strings" - "syscall" seccomp "github.com/elastic/go-seccomp-bpf" "github.com/urunc-dev/urunc/pkg/unikontainers/types" @@ -167,17 +166,13 @@ func (h *HVT) BuildExecCmd(args types.ExecArgs, ukernel types.Unikernel) ([]stri return cmdArgs, nil } -func (h *HVT) Execve(args types.ExecArgs, ukernel types.Unikernel) error { - cmdArgs, err := h.BuildExecCmd(args, ukernel) - if err != nil { - return err - } +// PreExec performs pre-execution setup for HVT. +// HVT requires applying seccomp filters before syscall.Exec if Seccomp is enabled. +func (h *HVT) PreExec(args types.ExecArgs) error { if args.Seccomp { - err := applySeccompFilter() - if err != nil { + if err := applySeccompFilter(); err != nil { return err } } - vmmLog.WithField("hvt command", cmdArgs).Debug("Ready to execve hvt") - return syscall.Exec(h.binaryPath, cmdArgs, args.Environment) //nolint: gosec + return nil } diff --git a/pkg/unikontainers/hypervisors/qemu.go b/pkg/unikontainers/hypervisors/qemu.go index e67e6c1d..dd5b1380 100644 --- a/pkg/unikontainers/hypervisors/qemu.go +++ b/pkg/unikontainers/hypervisors/qemu.go @@ -18,7 +18,6 @@ import ( "fmt" "runtime" "strings" - "syscall" "github.com/urunc-dev/urunc/pkg/unikontainers/types" ) @@ -140,11 +139,7 @@ func (q *Qemu) BuildExecCmd(args types.ExecArgs, ukernel types.Unikernel) ([]str return exArgs, nil } -func (q *Qemu) Execve(args types.ExecArgs, ukernel types.Unikernel) error { - exArgs, err := q.BuildExecCmd(args, ukernel) - if err != nil { - return err - } - vmmLog.WithField("qemu command", exArgs).Debug("Ready to execve qemu") - return syscall.Exec(q.Path(), exArgs, args.Environment) //nolint: gosec +// PreExec performs pre-execution setup. QEMU has no special pre-exec requirements. +func (q *Qemu) PreExec(_ types.ExecArgs) error { + return nil } diff --git a/pkg/unikontainers/hypervisors/spt.go b/pkg/unikontainers/hypervisors/spt.go index f4bfa30b..26b7cde1 100644 --- a/pkg/unikontainers/hypervisors/spt.go +++ b/pkg/unikontainers/hypervisors/spt.go @@ -17,7 +17,6 @@ package hypervisors import ( "os/exec" "strings" - "syscall" "github.com/urunc-dev/urunc/pkg/unikontainers/types" ) @@ -79,11 +78,7 @@ func (s *SPT) BuildExecCmd(args types.ExecArgs, ukernel types.Unikernel) ([]stri return cmdArgs, nil } -func (s *SPT) Execve(args types.ExecArgs, ukernel types.Unikernel) error { - cmdArgs, err := s.BuildExecCmd(args, ukernel) - if err != nil { - return err - } - vmmLog.WithField("spt command", cmdArgs).Debug("Ready to execve spt") - return syscall.Exec(s.binaryPath, cmdArgs, args.Environment) //nolint: gosec +// PreExec performs pre-execution setup. SPT has no special pre-exec requirements. +func (s *SPT) PreExec(_ types.ExecArgs) error { + return nil } diff --git a/pkg/unikontainers/types/types.go b/pkg/unikontainers/types/types.go index 154a34ce..28221ca2 100644 --- a/pkg/unikontainers/types/types.go +++ b/pkg/unikontainers/types/types.go @@ -27,9 +27,12 @@ type Unikernel interface { type VMM interface { // BuildExecCmd builds and validates the VMM command arguments without executing. // This is used to verify the command can be built before reporting container as started. + // The returned slice contains the command path as the first element followed by arguments. BuildExecCmd(args ExecArgs, ukernel Unikernel) ([]string, error) - // Execve replaces the current process with the VMM using syscall.Exec. - Execve(args ExecArgs, ukernel Unikernel) error + // PreExec performs any monitor-specific setup that must happen after BuildExecCmd + // succeeds but before syscall.Exec is called. For example, HVT applies seccomp + // filters here. Most monitors can return nil (no-op). + PreExec(args ExecArgs) error Stop(int) error Path() string UsesKVM() bool diff --git a/pkg/unikontainers/unikontainers.go b/pkg/unikontainers/unikontainers.go index 3e84997b..ad462976 100644 --- a/pkg/unikontainers/unikontainers.go +++ b/pkg/unikontainers/unikontainers.go @@ -494,9 +494,9 @@ func (u *Unikontainer) Exec(metrics m.Writer) error { uniklog.Debug("calling vmm execve") metrics.Capture(m.TS18) - // Pre-build the VMM command to verify it can be constructed successfully. + // Build the VMM command once and verify it can be constructed successfully. // This ensures we don't report the container as started if command building fails. - _, err = vmm.BuildExecCmd(vmmArgs, unikernel) + execCmd, err := vmm.BuildExecCmd(vmmArgs, unikernel) if err != nil { uniklog.WithError(err).Error("failed to build VMM command") return err @@ -510,7 +510,16 @@ func (u *Unikontainer) Exec(metrics m.Writer) error { return err } - return vmm.Execve(vmmArgs, unikernel) + // Perform any monitor-specific pre-exec setup (e.g., seccomp filters for HVT). + err = vmm.PreExec(vmmArgs) + if err != nil { + uniklog.WithError(err).Error("failed to perform pre-exec setup") + return err + } + + // Execute the VMM using the command we built earlier. + uniklog.WithField("command", execCmd).Debug("Ready to execve VMM") + return syscall.Exec(vmm.Path(), execCmd, vmmArgs.Environment) //nolint: gosec } func setupUser(user specs.User) error {