feat(docker): exec with enforced timeouts, and file transfer over exec

Two findings from running this against real gVisor, both of which changed the
implementation rather than just fixing it.

Docker's archive API cannot see tmpfs mounts. CopyToContainer and
CopyFromContainer resolve paths against the container's image layers, not its
live mount namespace, so a directory created by exec and confirmed by ls is
reported as "no such file" by docker cp — and a copy onto a writable tmpfs under
a read-only rootfs fails as "rootfs is marked read-only". Since openblox's
scratch space must be tmpfs (container-layer quotas need overlay2 on xfs with
pquota and hard-fail on ext4), the archive API is unusable for us in both
directions. WriteFile and ReadFile now stream through exec, which sees the real
mount namespace.

The destination path is passed as an argument rather than interpolated into the
shell script, so it lands in $0 and cannot escape the redirect. Building
`sh -c "cat > " + dest` would have been a command injection on any caller that
accepts a path from its user.

Cancelling a context does not interrupt a blocked read on a hijacked connection.
Exec previously returned ErrTimeout only after the command finished on its own —
correct error, useless deadline. StdCopy now runs in a goroutine and the
connection is closed on ctx.Done to unblock it. The integration suite dropped
from 126s to 8.8s, which is the two timeout tests no longer running their
sleep 60 to completion.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: Martin Yankovs

pkg/docker/backend.go

@@ -29,6 +29,8 @@ const (
 	labelCreatedAt = "sh.openblox.created-at"
 	labelIdle      = "sh.openblox.idle-timeout"
 	labelMaxAge    = "sh.openblox.max-age"
+	labelDefTmo    = "sh.openblox.default-timeout"
+	labelMaxTmo    = "sh.openblox.max-timeout"
 	labelUserPfx   = "sh.openblox.user."
 )
 
@@ -113,7 +115,14 @@ func (b *Backend) Open(ctx context.Context, name string) (sandbox.Sandbox, error
 	if inspect.Config == nil || inspect.Config.Labels[labelManaged] != "true" {
 		return nil, fmt.Errorf("%w: %q exists but is not managed by openblox", sandbox.ErrInvalid, name)
 	}
-	return &dockerSandbox{cli: b.cli, id: inspect.ID, info: infoFrom(inspect.ID, inspect.Config.Labels, inspect.Config.Image, inspect.State)}, nil
+	labels := inspect.Config.Labels
+	return &dockerSandbox{
+		cli:            b.cli,
+		id:             inspect.ID,
+		info:           infoFrom(inspect.ID, labels, inspect.Config.Image, inspect.State),
+		defaultTimeout: parseDurationLabel(labels[labelDefTmo], sandbox.DefaultCommandTimeout),
+		maxTimeout:     parseDurationLabel(labels[labelMaxTmo], sandbox.MaxCommandTimeout),
+	}, nil
 }
 
 // List returns every sandbox this backend manages.
@@ -175,6 +184,8 @@ func buildConfig(name string, spec sandbox.Spec) (*container.Config, *container.
 		labelCreatedAt: time.Now().UTC().Format(time.RFC3339Nano),
 		labelIdle:      spec.Lifetime.IdleTimeout.String(),
 		labelMaxAge:    spec.Lifetime.MaxAge.String(),
+		labelDefTmo:    spec.DefaultTimeout.String(),
+		labelMaxTmo:    spec.MaxTimeout.String(),
 	}
 	for k, v := range spec.Labels {
 		labels[labelUserPfx+k] = v
@@ -256,6 +267,14 @@ func stateFromStatus(status string) sandbox.State {
 	}
 }
 
+func parseDurationLabel(v string, fallback time.Duration) time.Duration {
+	d, err := time.ParseDuration(v)
+	if err != nil || d <= 0 {
+		return fallback
+	}
+	return d
+}
+
 func parseTimeLabel(v string) time.Time {
 	t, err := time.Parse(time.RFC3339Nano, v)
 	if err != nil {

pkg/docker/sandbox.go

@@ -1,15 +1,19 @@
 package docker
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"fmt"
 	"io"
 	"io/fs"
+	"path"
 	"time"
 
+	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/client"
+	"github.com/docker/docker/pkg/stdcopy"
 
 	"github.com/blox-eng/openblox/pkg/sandbox"
 )
@@ -22,6 +26,9 @@ type dockerSandbox struct {
 	cli  *client.Client
 	id   string
 	info sandbox.Info
+
+	defaultTimeout time.Duration
+	maxTimeout     time.Duration
 }
 
 func (s *dockerSandbox) Info() sandbox.Info { return s.info }
@@ -34,16 +41,201 @@ func (s *dockerSandbox) Stop(ctx context.Context) error {
 	return nil
 }
 
-func (s *dockerSandbox) Exec(context.Context, sandbox.Command) (sandbox.Result, error) {
-	return sandbox.Result{}, fmt.Errorf("Exec: %w", errNotImplemented)
+// resolveTimeout applies this sandbox's default and ceiling to a request.
+func (s *dockerSandbox) resolveTimeout(requested time.Duration) time.Duration {
+	if requested <= 0 {
+		requested = s.defaultTimeout
+	}
+	if s.maxTimeout > 0 && requested > s.maxTimeout {
+		return s.maxTimeout
+	}
+	return requested
+}
+
+// Exec runs a command to completion inside the sandbox.
+func (s *dockerSandbox) Exec(ctx context.Context, cmd sandbox.Command) (sandbox.Result, error) {
+	if err := cmd.Validate(); err != nil {
+		return sandbox.Result{}, err
+	}
+
+	timeout := s.resolveTimeout(cmd.Timeout)
+	ctx, cancel := context.WithTimeout(ctx, timeout)
+	defer cancel()
+
+	execID, attached, err := s.attach(ctx, cmd)
+	if err != nil {
+		return sandbox.Result{}, err
+	}
+	defer attached.Close()
+
+	var stdout, stderr bytes.Buffer
+	copyDone := make(chan error, 1)
+	go func() {
+		// The attach stream is multiplexed unless a TTY was allocated; StdCopy
+		// splits it back into the two streams.
+		_, err := stdcopy.StdCopy(&stdout, &stderr, attached.Reader)
+		copyDone <- err
+	}()
+
+	select {
+	case err := <-copyDone:
+		if err != nil {
+			return sandbox.Result{}, fmt.Errorf("read exec output in %q: %w", s.info.Name, err)
+		}
+	case <-ctx.Done():
+		// The attach stream is a hijacked connection: cancelling the context does
+		// not interrupt a blocked read on it. Closing the connection is what
+		// unblocks StdCopy, so without this a timed-out command would still take
+		// as long as the command itself.
+		attached.Close()
+		<-copyDone
+		return sandbox.Result{}, fmt.Errorf("%w: command in %q exceeded %s", sandbox.ErrTimeout, s.info.Name, timeout)
+	}
+
+	// Inspect with a fresh context: the exec finished, and reusing an expired
+	// one would turn a completed command into a spurious failure.
+	inspectCtx, inspectCancel := context.WithTimeout(context.WithoutCancel(ctx), 10*time.Second)
+	defer inspectCancel()
+
+	inspect, err := s.cli.ContainerExecInspect(inspectCtx, execID)
+	if err != nil {
+		return sandbox.Result{}, fmt.Errorf("inspect exec in %q: %w", s.info.Name, err)
+	}
+
+	// A non-zero exit is the command's result, not our error.
+	return sandbox.Result{
+		Stdout:   stdout.Bytes(),
+		Stderr:   stderr.Bytes(),
+		ExitCode: inspect.ExitCode,
+	}, nil
+}
+
+func (s *dockerSandbox) attach(ctx context.Context, cmd sandbox.Command) (string, types.HijackedResponse, error) {
+	created, err := s.cli.ContainerExecCreate(ctx, s.id, container.ExecOptions{
+		Cmd:          cmd.Argv,
+		Env:          cmd.Env,
+		WorkingDir:   cmd.Dir,
+		AttachStdin:  cmd.Stdin != nil,
+		AttachStdout: true,
+		AttachStderr: true,
+	})
+	if err != nil {
+		return "", types.HijackedResponse{}, fmt.Errorf("exec create in %q: %w", s.info.Name, err)
+	}
+
+	attached, err := s.cli.ContainerExecAttach(ctx, created.ID, container.ExecAttachOptions{})
+	if err != nil {
+		return "", types.HijackedResponse{}, fmt.Errorf("exec attach in %q: %w", s.info.Name, err)
+	}
+
+	if cmd.Stdin != nil {
+		// Copy in the background: a guest that never reads stdin would otherwise
+		// block us before the timeout could fire.
+		go func() {
+			defer func() { _ = attached.CloseWrite() }()
+			_, _ = io.Copy(attached.Conn, cmd.Stdin)
+		}()
+	}
+	return created.ID, attached, nil
+}
+
+// WriteFile writes src to a path inside the sandbox, creating parent directories.
+//
+// This streams through exec rather than Docker's archive API. The archive API
+// resolves paths against the container's image layers and cannot see tmpfs
+// mounts — and openblox's writable scratch space is tmpfs, because
+// container-layer disk quotas are unavailable on most hosts. So CopyToContainer
+// reports "no such file" for a directory that demonstrably exists inside the
+// sandbox. Exec sees the real mount namespace.
+func (s *dockerSandbox) WriteFile(ctx context.Context, dest string, mode fs.FileMode, src io.Reader) error {
+	if !path.IsAbs(dest) {
+		return fmt.Errorf("%w: path %q is not absolute", sandbox.ErrInvalid, dest)
+	}
+
+	if err := s.run(ctx, "create directory", []string{"mkdir", "-p", path.Dir(dest)}); err != nil {
+		return err
+	}
+
+	// The destination is passed as an argument, not interpolated into the shell
+	// script, so it lands in $0 and cannot break out of the redirect. Building
+	// `sh -c "cat > " + dest` instead would be a command injection on any caller
+	// that accepts a path from its user.
+	res, err := s.Exec(ctx, sandbox.Command{
+		Argv:  []string{"sh", "-c", `cat > "$0"`, dest},
+		Stdin: src,
+	})
+	if err != nil {
+		return fmt.Errorf("write %q in %q: %w", dest, s.info.Name, err)
+	}
+	if res.ExitCode != 0 {
+		return fmt.Errorf("write %q in %q: exit %d: %s",
+			dest, s.info.Name, res.ExitCode, bytes.TrimSpace(res.Stderr))
+	}
+
+	return s.run(ctx, "set mode", []string{"chmod", fmt.Sprintf("%04o", mode.Perm()), dest})
+}
+
+// ReadFile opens a path inside the sandbox. The caller must close the reader.
+//
+// Like WriteFile, this goes through exec rather than the archive API, which
+// cannot see the tmpfs scratch mounts.
+func (s *dockerSandbox) ReadFile(ctx context.Context, src string) (io.ReadCloser, error) {
+	if !path.IsAbs(src) {
+		return nil, fmt.Errorf("%w: path %q is not absolute", sandbox.ErrInvalid, src)
+	}
+
+	// Probe first. The body is streamed, so a missing file would otherwise
+	// surface as an empty read rather than an error the caller can act on.
+	probe, err := s.Exec(ctx, sandbox.Command{Argv: []string{"test", "-f", src}})
+	if err != nil {
+		return nil, fmt.Errorf("stat %q in %q: %w", src, s.info.Name, err)
+	}
+	if probe.ExitCode != 0 {
+		return nil, fmt.Errorf("%w: %q in sandbox %q", sandbox.ErrNotFound, src, s.info.Name)
+	}
+
+	streamCtx, cancel := context.WithCancel(context.WithoutCancel(ctx))
+	_, attached, err := s.attach(streamCtx, sandbox.Command{Argv: []string{"cat", "--", src}})
+	if err != nil {
+		cancel()
+		return nil, err
+	}
+
+	pr, pw := io.Pipe()
+	go func() {
+		// stderr is discarded: the probe above already established the file is
+		// readable, and a partial read surfaces as a short body.
+		_, err := stdcopy.StdCopy(pw, io.Discard, attached.Reader)
+		_ = pw.CloseWithError(err)
+	}()
+
+	return &execStream{Reader: pr, attached: attached, cancel: cancel}, nil
+}
+
+type execStream struct {
+	io.Reader
+	attached types.HijackedResponse
+	cancel   context.CancelFunc
 }
 
-func (s *dockerSandbox) WriteFile(context.Context, string, fs.FileMode, io.Reader) error {
-	return fmt.Errorf("WriteFile: %w", errNotImplemented)
+func (e *execStream) Close() error {
+	e.attached.Close()
+	e.cancel()
+	return nil
 }
 
-func (s *dockerSandbox) ReadFile(context.Context, string) (io.ReadCloser, error) {
-	return nil, fmt.Errorf("ReadFile: %w", errNotImplemented)
+// run executes a command and turns a non-zero exit into an error. For internal
+// helpers a non-zero exit is a failure, unlike a caller's own command.
+func (s *dockerSandbox) run(ctx context.Context, what string, argv []string) error {
+	res, err := s.Exec(ctx, sandbox.Command{Argv: argv})
+	if err != nil {
+		return fmt.Errorf("%s in %q: %w", what, s.info.Name, err)
+	}
+	if res.ExitCode != 0 {
+		return fmt.Errorf("%s in %q: exit %d: %s",
+			what, s.info.Name, res.ExitCode, bytes.TrimSpace(res.Stderr))
+	}
+	return nil
 }
 
 func (s *dockerSandbox) StartProcess(context.Context, string, sandbox.Command) error {