From b6f339a8e183b982cc7ddeea402f182d339672a8 Mon Sep 17 00:00:00 2001 From: SaladDay <1203511142@qq.com> Date: Tue, 6 Oct 2026 22:47:12 +0000 Subject: [PATCH] Reuse the process codec for the trampoline The trampoline now receives the operation's encoded StartRequest and decodes it with sandboxprocess.Decode, deriving argv, environment, TERM and PATH at the child boundary, so its private launch format and the second copy of PATH go. It reads the request with io.ReadAll on an owned *os.File. Startup failures wrap their cause with fmt.Errorf naming the step; StartupError and Step had no reader. --- apps/sandboxio/cmd/oac-sandbox-io/main.go | 2 +- .../internal/processservice/launch.go | 18 +- .../internal/processservice/service.go | 2 +- .../internal/processservice/service_test.go | 2 +- .../internal/processservice/trampoline.go | 160 +++++------------- .../sandboxio/internal/sandboxio/sandboxio.go | 34 +--- .../internal/sandboxio/sandboxio_test.go | 19 +++ 7 files changed, 80 insertions(+), 157 deletions(-) diff --git a/apps/sandboxio/cmd/oac-sandbox-io/main.go b/apps/sandboxio/cmd/oac-sandbox-io/main.go index 94666fd8a..bf3fb2f89 100644 --- a/apps/sandboxio/cmd/oac-sandbox-io/main.go +++ b/apps/sandboxio/cmd/oac-sandbox-io/main.go @@ -34,7 +34,7 @@ func main() { // Reap is the process's only wait. As a child subreaper it also reaps // the orphaned descendants of operations. if err := unix.Prctl(unix.PR_SET_CHILD_SUBREAPER, 1, 0, 0, 0); err != nil { - fail(&sandboxio.StartupError{Step: sandboxio.StepSubreaper, Err: err}) + fail(fmt.Errorf("become a child subreaper: %w", err)) } go processservice.Reap(context.Background()) diff --git a/apps/sandboxio/internal/processservice/launch.go b/apps/sandboxio/internal/processservice/launch.go index 951879f04..4e9af85d7 100644 --- a/apps/sandboxio/internal/processservice/launch.go +++ b/apps/sandboxio/internal/processservice/launch.go @@ -21,8 +21,8 @@ import ( // launch spawns the process and emits Started or StartFailed, then starts // the output readers. The leader's exit follows the output it left buffered; // see markLocked. -func (op *operation) launch(spec sp.ProcessSpec) { - l, f := op.spawn(spec) +func (op *operation) launch(req sp.StartRequest) { + l, f := op.spawn(req) op.mu.Lock() if f != nil { op.state, op.startFailure = sp.StateStartFailed, f @@ -59,9 +59,9 @@ type launched struct { streams []*stream } -// spawn starts the trampoline in a new session and waits until it has -// exec'd the target or reported why it could not. -func (op *operation) spawn(spec sp.ProcessSpec) (l launched, f *sp.Failure) { +// spawn starts the trampoline in a new session, hands it the request and +// waits until it has exec'd the target or reported why it could not. +func (op *operation) spawn(req sp.StartRequest) (l launched, f *sp.Failure) { ioFail := func(what string, err error) *sp.Failure { return sp.Fail(sp.CodeIO, sandboxwire.EffectNone, "%s: %v", what, err) } @@ -94,8 +94,8 @@ func (op *operation) spawn(spec sp.ProcessSpec) (l launched, f *sp.Failure) { if files[statusFD], statusR, f = pipe("status pipe", false); f != nil { return l, f } - if spec.PTY != nil { - master, tty, err := openPTY(*spec.PTY) + if req.Spec.PTY != nil { + master, tty, err := openPTY(*req.Spec.PTY) if err != nil { return l, ioFail("open terminal", err) } @@ -125,7 +125,7 @@ func (op *operation) spawn(spec sp.ProcessSpec) (l launched, f *sp.Failure) { Env: []string{trampolineEnv}, Files: files, // Setctty makes the child's descriptor 0, the terminal, its controlling terminal. - Sys: &syscall.SysProcAttr{Setsid: true, Setctty: spec.PTY != nil}, + Sys: &syscall.SysProcAttr{Setsid: true, Setctty: req.Spec.PTY != nil}, } reaping.RLock() p, err := os.StartProcess("/proc/self/exe", []string{trampolineArg0}, attr) @@ -157,7 +157,7 @@ func (op *operation) spawn(spec sp.ProcessSpec) (l launched, f *sp.Failure) { closeAll(child) child = nil - _, werr := launchW.Write(encodeLaunch(spec)) + _, werr := launchW.Write(sp.Encode(req)) launchW.Close() status, rerr := io.ReadAll(statusR) statusR.Close() diff --git a/apps/sandboxio/internal/processservice/service.go b/apps/sandboxio/internal/processservice/service.go index 7b69a8e35..9d9ea1a0d 100644 --- a/apps/sandboxio/internal/processservice/service.go +++ b/apps/sandboxio/internal/processservice/service.go @@ -187,7 +187,7 @@ func (s *Service) Start(_ context.Context, conn *sp.Conn, req sp.StartRequest) ( op.observeLocked(conn, 0) op.mu.Unlock() s.mu.Unlock() - op.launch(req.Spec) + op.launch(req) return sp.StartResponse{Disposition: sp.StartCreated}, nil } diff --git a/apps/sandboxio/internal/processservice/service_test.go b/apps/sandboxio/internal/processservice/service_test.go index 3ee75eca6..bed22bb22 100644 --- a/apps/sandboxio/internal/processservice/service_test.go +++ b/apps/sandboxio/internal/processservice/service_test.go @@ -642,7 +642,7 @@ func TestRevokeWhileStarting(t *testing.T) { h.svc.active.Add(1) h.svc.mu.Unlock() h.svc.AttachmentRevoked(h.att) - op.launch(spec) + op.launch(sp.StartRequest{OperationRef: sp.OperationRef{ServerInstanceID: h.svc.instance, OperationID: key.operation}, Spec: spec}) for deadline := time.Now().Add(10 * time.Second); op.inspect().State != sp.StateExited; time.Sleep(10 * time.Millisecond) { if time.Now().After(deadline) { t.Fatal("the launched operation was not cancelled") diff --git a/apps/sandboxio/internal/processservice/trampoline.go b/apps/sandboxio/internal/processservice/trampoline.go index ebf5194b0..5dd2a2005 100644 --- a/apps/sandboxio/internal/processservice/trampoline.go +++ b/apps/sandboxio/internal/processservice/trampoline.go @@ -3,9 +3,9 @@ package processservice import ( - "bytes" "encoding/binary" "errors" + "io" "os" "os/signal" "runtime" @@ -15,19 +15,20 @@ import ( "golang.org/x/sys/unix" - "github.com/MiniMax-AI/OpenAgentCore/internal/sandboxprocess" - "github.com/MiniMax-AI/OpenAgentCore/internal/sandboxwire" + sp "github.com/MiniMax-AI/OpenAgentCore/internal/sandboxprocess" ) // os/exec cannot set a child's umask, so every launch re-executes the service -// binary as a trampoline. It reads the launch from an inherited descriptor, -// applies the umask and working directory, and execs the target. The marker -// is its whole environment and argv; it is not a command-line interface. +// binary as a trampoline. It reads the operation's Start request from an +// inherited descriptor, applies the umask and working directory, and execs the +// target. The marker is its whole environment and argv; it is not a +// command-line interface. const ( trampolineArg0 = "oac-process-trampoline" trampolineEnv = "OAC_PROCESS_TRAMPOLINE=1" - // Descriptors the trampoline inherits: the launch, and a close-on-exec - // status pipe that reaches EOF without data exactly when exec succeeds. + // Descriptors the trampoline inherits: the Start request, and a + // close-on-exec status pipe that reaches EOF without data exactly when + // exec succeeds. launchFD = 3 statusFD = 4 ) @@ -64,31 +65,22 @@ func errnoOf(err error) syscall.Errno { func trampoline() (uint16, error) { unix.CloseOnExec(statusFD) - var buf bytes.Buffer - chunk := make([]byte, 64<<10) - for { - n, err := unix.Read(launchFD, chunk) - if err == unix.EINTR { - continue - } - if err != nil { - return stageLaunch, err - } - if n == 0 { - break - } - buf.Write(chunk[:n]) + f := os.NewFile(launchFD, "launch") + b, err := io.ReadAll(f) + f.Close() + if err != nil { + return stageLaunch, err } - unix.Close(launchFD) - l, err := decodeLaunch(buf.Bytes()) + m, err := sp.Decode(sp.OpStart, b) if err != nil { return stageLaunch, syscall.EINVAL } + spec := m.(sp.StartRequest).Spec if err := closeInherited(); err != nil { return stageDescriptors, err } - unix.Umask(int(l.umask)) - if err := unix.Chdir(l.cwd); err != nil { + unix.Umask(int(spec.Umask)) + if err := unix.Chdir(string(spec.Cwd)); err != nil { return stageChdir, err } // The Go runtime keeps SIGHUP and SIGINT ignored when it starts with them @@ -96,7 +88,7 @@ func trampoline() (uint16, error) { signal.Notify(make(chan os.Signal, 1), syscall.SIGHUP, syscall.SIGINT) var none unix.Sigset_t unix.PthreadSigmask(unix.SIG_SETMASK, &none, nil) - return stageExec, execTarget(l) + return stageExec, execTarget(spec) } // closeInherited makes exec close every descriptor from 3 up, so the target @@ -116,21 +108,34 @@ func closeInherited() error { return nil } -// execTarget execs the executable, searching PATH for a name without '/' the -// way execvpe does. -func execTarget(l launch) error { - if strings.Contains(l.executable, "/") { - return unix.Exec(l.executable, l.argv, l.env) +// execTarget execs the spec with TERM added for a terminal, searching PATH +// for a name without '/' the way execvpe does. Decode guarantees PATH then. +func execTarget(spec sp.ProcessSpec) error { + argv := make([]string, len(spec.Argv)) + for i, a := range spec.Argv { + argv[i] = string(a) + } + var env []string + var path string + for _, v := range spec.Env { + env = append(env, string(v.Name)+"="+string(v.Value)) + if string(v.Name) == "PATH" { + path = string(v.Value) + } + } + if spec.PTY != nil { + env = append(env, "TERM="+string(spec.PTY.Term)) } - if !l.hasPath { - return unix.ENOENT + exe := string(spec.Executable) + if strings.Contains(exe, "/") { + return unix.Exec(exe, argv, env) } denied := false - for _, dir := range strings.Split(l.path, ":") { + for _, dir := range strings.Split(path, ":") { if dir == "" { dir = "." } - err := unix.Exec(dir+"/"+l.executable, l.argv, l.env) + err := unix.Exec(dir+"/"+exe, argv, env) switch err { case unix.EACCES: denied = true @@ -144,86 +149,3 @@ func execTarget(l launch) error { } return unix.ENOENT } - -// launch is what the trampoline needs from a ProcessSpec. -type launch struct { - executable string - argv []string - env []string - cwd string - umask uint32 - hasPath bool - path string -} - -func encodeLaunch(spec sandboxprocess.ProcessSpec) []byte { - var e sandboxwire.Encoder - e.Bytes(spec.Executable) - e.Count(len(spec.Argv)) - for _, a := range spec.Argv { - e.Bytes(a) - } - env := make([][]byte, 0, len(spec.Env)+1) - var path []byte - for _, v := range spec.Env { - env = append(env, bytes.Join([][]byte{v.Name, v.Value}, []byte("="))) - if string(v.Name) == "PATH" { - path = v.Value - } - } - if spec.PTY != nil { - env = append(env, append([]byte("TERM="), spec.PTY.Term...)) - } - e.Count(len(env)) - for _, v := range env { - e.Bytes(v) - } - e.Bytes(spec.Cwd) - e.U32(spec.Umask) - e.Present(path != nil) - if path != nil { - e.Bytes(path) - } - return e.Payload() -} - -func decodeLaunch(b []byte) (launch, error) { - d := sandboxwire.NewDecoder(b) - var l launch - var err error - str := func() string { - var v []byte - if err == nil { - v, err = d.Bytes() - } - return string(v) - } - list := func() []string { - var n int - if err == nil { - n, err = d.Count(sandboxwire.MaxPayload) - } - out := make([]string, n) - for i := range out { - out[i] = str() - } - return out - } - l.executable = str() - l.argv = list() - l.env = list() - l.cwd = str() - if err == nil { - l.umask, err = d.U32() - } - if err == nil { - l.hasPath, err = d.Present() - } - if l.hasPath { - l.path = str() - } - if err == nil { - err = d.Finish() - } - return l, err -} diff --git a/apps/sandboxio/internal/sandboxio/sandboxio.go b/apps/sandboxio/internal/sandboxio/sandboxio.go index 1615235e2..50ce77242 100644 --- a/apps/sandboxio/internal/sandboxio/sandboxio.go +++ b/apps/sandboxio/internal/sandboxio/sandboxio.go @@ -10,6 +10,7 @@ package sandboxio import ( "context" "crypto/tls" + "fmt" "log" "sync/atomic" "time" @@ -26,26 +27,6 @@ import ( "github.com/MiniMax-AI/OpenAgentCore/internal/sandboxwire" ) -// Step names the startup step a StartupError failed in. -type Step string - -const ( - StepSubreaper Step = "become a child subreaper" - StepBootstrap Step = "read the bootstrap file" - StepProcessService Step = "start the process service" - StepFileService Step = "start the file service" -) - -// StartupError is a failure before the service serves. Its message names the -// step and never includes the credential. -type StartupError struct { - Step Step - Err error -} - -func (e *StartupError) Error() string { return string(e.Step) + ": " + e.Err.Error() } -func (e *StartupError) Unwrap() error { return e.Err } - // shutdownMargin is how long Shutdown waits past the grace limit for killed // processes to be observed gone. const shutdownMargin = 5 * time.Second @@ -53,8 +34,9 @@ const shutdownMargin = 5 * time.Second // Run serves the sandbox the bootstrap file at bootstrapPath names, with the // export world rooted at "/", until ctx ends or the relay refuses the link // for good. The caller has made the process a child subreaper running -// processservice.Reap. Run returns nil when ctx ended, a *StartupError when -// it could not start, and otherwise the relay's *sandboxlink.Error. +// processservice.Reap. Run returns nil when ctx ended, an error naming the +// step and never the credential when it could not start, and otherwise the +// relay's *sandboxlink.Error. func Run(ctx context.Context, bootstrapPath string) error { return run(ctx, bootstrapPath, options{root: "/"}) } @@ -69,20 +51,20 @@ type options struct { func run(ctx context.Context, bootstrapPath string, opt options) error { raw, err := runtimefs.ReadPrivatePath(bootstrapPath, sandboxbootstrap.MaxBytes) if err != nil { - return &StartupError{StepBootstrap, err} + return fmt.Errorf("read the bootstrap file: %w", err) } in, err := sandboxbootstrap.Decode(raw) if err != nil { - return &StartupError{StepBootstrap, err} + return fmt.Errorf("read the bootstrap file: %w", err) } procCfg := processservice.DefaultConfig() procs, err := processservice.New(procCfg) if err != nil { - return &StartupError{StepProcessService, err} + return fmt.Errorf("start the process service: %w", err) } files, err := fileservice.New(opt.root) if err != nil { - return &StartupError{StepFileService, err} + return fmt.Errorf("start the file service: %w", err) } defer files.Close() fileServer := sandboxfs.NewServer(files) diff --git a/apps/sandboxio/internal/sandboxio/sandboxio_test.go b/apps/sandboxio/internal/sandboxio/sandboxio_test.go index 8a9843de5..4afafd42d 100644 --- a/apps/sandboxio/internal/sandboxio/sandboxio_test.go +++ b/apps/sandboxio/internal/sandboxio/sandboxio_test.go @@ -263,3 +263,22 @@ func TestRefusedCredentialEndsTheService(t *testing.T) { t.Fatalf("run: %v, want AuthenticationFailed without the credential", err) } } + +// A failure before serving names its step, and the message never carries the +// credential. +func TestStartupFailureNamesTheStep(t *testing.T) { + valid, _ := writeBootstrap(t, "wss://relay.invalid/link", "serve-credential") + invalid := filepath.Join(t.TempDir(), "bootstrap.json") + if err := os.WriteFile(invalid, []byte(`{"version":1,"credential":"serve-credential"}`), 0o600); err != nil { + t.Fatal(err) + } + for _, c := range []struct{ bootstrap, root, want string }{ + {invalid, t.TempDir(), "read the bootstrap file: "}, + {valid, filepath.Join(t.TempDir(), "missing"), "start the file service: "}, + } { + err := run(context.Background(), c.bootstrap, options{root: c.root}) + if err == nil || !strings.HasPrefix(err.Error(), c.want) || strings.Contains(err.Error(), "serve-credential") { + t.Errorf("run: %v, want %q without the credential", err, c.want) + } + } +}