fix: avoid shell parsing in docker cleanup

tests/tools/test_docker_environment.py

@@ -29,6 +29,15 @@ def _run(cmd, **kwargs):
     return calls
 
 
+class _ImmediateThread:
+    def __init__(self, target, daemon=False, **kwargs):
+        self.target = target
+        self.daemon = daemon
+
+    def start(self):
+        self.target()
+
+
 def _make_dummy_env(**kwargs):
     """Helper to construct DockerEnvironment with minimal required args."""
     return docker_env.DockerEnvironment(
@@ -203,25 +212,23 @@ def test_auto_mount_replaces_persistent_workspace_bind(monkeypatch, tmp_path):
 
 
 def test_non_persistent_cleanup_removes_container(monkeypatch):
-    """When persistent=false, cleanup() must schedule docker stop + rm."""
+    """When persistent=false, cleanup() must stop and remove the container."""
     monkeypatch.setattr(docker_env, "find_docker", lambda: "/usr/bin/docker")
     calls = _mock_subprocess_run(monkeypatch)
-
-    popen_cmds = []
-    monkeypatch.setattr(
-        docker_env.subprocess, "Popen",
-        lambda cmd, **kw: (popen_cmds.append(cmd), type("P", (), {"poll": lambda s: 0, "wait": lambda s, **k: None, "returncode": 0, "stdout": iter([]), "stdin": None})())[1],
-    )
+    monkeypatch.setattr(docker_env.threading, "Thread", _ImmediateThread)
 
     env = _make_dummy_env(persistent_filesystem=False, task_id="ephemeral-task")
     assert env._container_id
     container_id = env._container_id
 
     env.cleanup()
 
-    # Should have stop and rm calls via Popen
-    stop_cmds = [c for c in popen_cmds if container_id in str(c) and "stop" in str(c)]
-    assert len(stop_cmds) >= 1, f"cleanup() should schedule docker stop for {container_id}"
+    stop_cmds = [c for c in calls if c[0] == ["/usr/bin/docker", "stop", "--time", "60", container_id]]
+    rm_cmds = [c for c in calls if c[0] == ["/usr/bin/docker", "rm", "-f", container_id]]
+    assert len(stop_cmds) == 1, f"cleanup() should stop {container_id}"
+    assert len(rm_cmds) == 1, f"cleanup() should remove {container_id}"
+    assert "shell" not in stop_cmds[0][1]
+    assert "shell" not in rm_cmds[0][1]
 
 
 class _FakePopen:

tests/tools/test_docker_environment_cleanup.py

@@ -0,0 +1,87 @@
+"""Tests for DockerEnvironment cleanup command construction."""
+
+from types import SimpleNamespace
+
+from tools.environments import docker as docker_env
+from tools.environments.docker import DockerEnvironment
+
+
+class _ImmediateThread:
+    def __init__(self, target, daemon=False, **kwargs):
+        self.target = target
+        self.daemon = daemon
+
+    def start(self):
+        self.target()
+
+
+def _make_env(*, persistent=False):
+    env = DockerEnvironment.__new__(DockerEnvironment)
+    env._container_id = "abc; touch /tmp/pwn"
+    env._docker_exe = "/tmp/docker;evil"
+    env._persistent = persistent
+    env._workspace_dir = None
+    env._home_dir = None
+    return env
+
+
+def test_cleanup_uses_argv_form_without_shell(monkeypatch):
+    calls = []
+
+    def fake_run(args, **kwargs):
+        calls.append((args, kwargs))
+        return SimpleNamespace(returncode=0)
+
+    monkeypatch.setattr(docker_env.threading, "Thread", _ImmediateThread)
+    monkeypatch.setattr(docker_env.subprocess, "run", fake_run)
+
+    env = _make_env(persistent=False)
+    env.cleanup()
+
+    assert env._container_id is None
+    assert [call[0] for call in calls] == [
+        ["/tmp/docker;evil", "stop", "--time", "60", "abc; touch /tmp/pwn"],
+        ["/tmp/docker;evil", "rm", "-f", "abc; touch /tmp/pwn"],
+    ]
+    for args, kwargs in calls:
+        assert isinstance(args, list)
+        assert "shell" not in kwargs
+        assert kwargs["stdout"] is docker_env.subprocess.DEVNULL
+        assert kwargs["stderr"] is docker_env.subprocess.DEVNULL
+
+
+def test_cleanup_persistent_container_removes_only_when_stop_fails(monkeypatch):
+    calls = []
+
+    def fake_run(args, **kwargs):
+        calls.append((args, kwargs))
+        return SimpleNamespace(returncode=1 if args[1] == "stop" else 0)
+
+    monkeypatch.setattr(docker_env.threading, "Thread", _ImmediateThread)
+    monkeypatch.setattr(docker_env.subprocess, "run", fake_run)
+
+    env = _make_env(persistent=True)
+    env.cleanup()
+
+    assert [call[0] for call in calls] == [
+        ["/tmp/docker;evil", "stop", "--time", "60", "abc; touch /tmp/pwn"],
+        ["/tmp/docker;evil", "rm", "-f", "abc; touch /tmp/pwn"],
+    ]
+
+
+def test_cleanup_persistent_container_keeps_stopped_container(monkeypatch):
+    calls = []
+
+    def fake_run(args, **kwargs):
+        calls.append((args, kwargs))
+        return SimpleNamespace(returncode=0)
+
+    monkeypatch.setattr(docker_env.threading, "Thread", _ImmediateThread)
+    monkeypatch.setattr(docker_env.subprocess, "run", fake_run)
+
+    env = _make_env(persistent=True)
+    env.cleanup()
+
+    assert [call[0] for call in calls] == [
+        ["/tmp/docker;evil", "stop", "--time", "60", "abc; touch /tmp/pwn"],
+    ]

tools/environments/docker.py

@@ -11,6 +11,7 @@
 import shutil
 import subprocess
 import sys
+import threading
 import uuid
 from typing import Optional
 
@@ -629,26 +630,42 @@ def _storage_opt_supported() -> bool:
     def cleanup(self):
         """Stop and remove the container. Bind-mount dirs persist if persistent=True."""
         if self._container_id:
-            try:
-                # Stop in background so cleanup doesn't block
-                stop_cmd = (
-                    f"(timeout 60 {self._docker_exe} stop {self._container_id} || "
-                    f"{self._docker_exe} rm -f {self._container_id}) >/dev/null 2>&1 &"
-                )
-                subprocess.Popen(stop_cmd, shell=True)
-            except Exception as e:
-                logger.warning("Failed to stop container %s: %s", self._container_id, e)
+            container_id = self._container_id
+            docker_exe = self._docker_exe
+            persistent = self._persistent
+            self._container_id = None
 
-            if not self._persistent:
-                # Also schedule removal (stop only leaves it as stopped)
+            def _cleanup_container() -> None:
                 try:
-                    subprocess.Popen(
-                        f"sleep 3 && {self._docker_exe} rm -f {self._container_id} >/dev/null 2>&1 &",
-                        shell=True,
+                    stop_result = subprocess.run(
+                        [docker_exe, "stop", "--time", "60", container_id],
+                        stdout=subprocess.DEVNULL,
+                        stderr=subprocess.DEVNULL,
+                        timeout=65,
+                        check=False,
                     )
-                except Exception:
-                    pass
-            self._container_id = None
+                    if not persistent or stop_result.returncode != 0:
+                        subprocess.run(
+                            [docker_exe, "rm", "-f", container_id],
+                            stdout=subprocess.DEVNULL,
+                            stderr=subprocess.DEVNULL,
+                            timeout=15,
+                            check=False,
+                        )
+                except Exception as e:
+                    logger.warning("Failed to clean up container %s: %s", container_id, e)
+
+            # Stop in a background worker so cleanup doesn't block, but avoid
+            # shell=True so configured docker paths and container IDs are never
+            # shell-parsed. Keep the worker non-daemon so interpreter shutdown
+            # cannot silently abandon cleanup after it has been scheduled.
+            try:
+                threading.Thread(
+                    target=_cleanup_container,
+                    name=f"docker-cleanup-{container_id[:12]}",
+                ).start()
+            except Exception:
+                _cleanup_container()
 
         if not self._persistent:
             for d in (self._workspace_dir, self._home_dir):