Skip to content

fix: fail the launch when the egress socket cannot be bound - #16

Merged
sstriker merged 4 commits into
mainfrom
fix/egress-socket-bind
Oct 24, 2026
Merged

sstriker merged 4 commits into
mainfrom
fix/egress-socket-bind

Conversation

@sstriker

Copy link
Copy Markdown
Contributor

This PR fixes one of the gaps that the harness-container survey (#2) found in our code: a socket path past 108 bytes (docs/harness-containers.md, "A socket path past 108 bytes").

The problem

The egress socket is at <root>/state/run/egress-<pid>.sock. A unix socket path takes at most 108 bytes on Linux and 104 on macOS, the closing NUL included. The launcher started serve_unix as a task, waited up to two seconds for the socket file, and continued when the file did not come. The bind error stayed in the task. With a long root, the user saw only an unrelated bind-mount ENOENT and spawn /opt/memory/memory: Invalid argument.

The change

  • dm_egress:
    • Egress::bind_unix(path) checks the length first. SUN_PATH comes from libc::sockaddr_un, so it is right on every unix. The error names the path and the number of bytes.
    • Before it binds, it looks at the path. If a socket is there and no process listens on it, it removes the socket. If a process accepts connections on it, it refuses: "in use by another launch". If something other than a socket is there, it refuses and leaves it in place.
    • Egress::serve(listener) serves a listener that is already bound.
  • The launcher, Session::start:
    • It binds before it starts the serve task, so a failed bind stops the launch with its own error. The wait for the file is gone.
    • The unconfined TCP runner keeps its listener. Before, it closed the listener and bound the address again.
    • Decision: the unconfined runner binds no unix socket. Nothing in unconfined mode reads it, because the harness reaches the egress over TCP. If it bound the socket, an unconfined launch from a long root would now fail, where it works today. That would hit macOS first, where temporary paths are long.
  • The relay binds its loopback port before it starts the harness. If it cannot, it exits 1 with relay: cannot listen on <addr>. Before, the harness ran anyway.

Failing first, against main

cargo nextest run -p dm-egress -p amem with the new tests:

  • Too-long path: the error was egress: path must be shorter than SUN_LEN, with no length.
  • Confined launch from a long root reproduced the doc's symptom:
    memory: sandbox: confinement: bind /tmp/.../memory/state/run/egress-4095098.sock -> /tmp/memory-sandbox-4095098/run/memory/egress.sock: ENOENT
    error: confinement: spawn /opt/memory/memory: Invalid argument (os error 22)
    
  • Relay: left: Some(0) right: Some(1). The harness ran although the relay could not listen.
  • amem inside with a 120-character requests directory: exit 1 with No such file or directory, not the length. This test needs no namespaces, so it runs on CI.
  • Unconfined launch: state/run while the harness ran: . .. egress-1195928.sock. On main, the unconfined runner still bound the socket.

Against ea76792, before the second commit:

  • A second bind_unix on a live socket succeeded, and took the connections. On a regular file, it deleted the file.

Mutations:

  • finish() without its socket removal: the session takes its socket away: ["egress-1242654.sock"].
  • bind_unix without the stale-socket removal: Address already in use.

make check: 630 tests, 630 passed, 0 skipped.

Adversarial pass

A fresh agent attacked ea76792 and found nothing that blocks the change. It verified these points:

  • The length check counts bytes. Multibyte paths are counted correctly, and there is no off-by-one error in either direction.
  • No race is left: bind and listen happen before the task starts, and the relay listens before the harness starts.
  • The kept TCP listener is not inherited by the harness.
  • Nothing in unconfined mode reads the socket path.

It found these gaps, all fixed in 58fa841:

  • One test also passed on main.
  • The main path had no test that runs on CI. The confined test skips there, as it does on main.
  • The stale-socket removal and the cleanup in finish() had no tests.
  • bind_unix replaced a live socket.
  • SUN_PATH was 104 on every unix other than Linux.

Posted by Claude on behalf of @sstriker

sstriker and others added 2 commits October 24, 2026 14:08
The launcher started the egress socket in a task. Then it waited two
seconds for the socket file and continued. When the bind failed, the
error stayed in the task. The user saw an unrelated bind-mount ENOENT
and "spawn /opt/memory/memory: Invalid argument".

Now the launcher binds the socket before it starts the task. A failed
bind stops the launch with its own error. Egress::bind_unix refuses a
path that is too long for a unix socket. The error gives the path, its
length and the limit: 108 bytes on Linux and 104 on macOS, with the
closing NUL.

The unconfined runner uses TCP and does not use the socket. It does
not bind the socket now, so a long root does not stop an unconfined
launch. The unconfined runner also keeps its TCP listener and serves
on it. Before, it closed the listener and bound the address again in
the task.

The relay binds its loopback port before it starts the harness. When
the bind fails, the relay stops with exit 1 and does not start the
harness.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The egress removed any file at the socket path before it bound the
socket. This removed a regular file or a symlink. It also removed a
socket that another launch used, and the new socket then took that
launch's new connections.

Now Egress::bind_unix examines the path first:
- Nothing is there: it binds.
- A socket is there and no listener answers on it: it removes the
  socket and binds.
- A socket is there and a listener answers on it: it stops with "in use
  by another launch".
- Something that is not a socket is there: it stops and keeps it.

SUN_PATH now comes from the size of libc::sockaddr_un minus the offset
of sun_path. This gives the correct limit on each unix system, without
unsafe code.

New tests:
- An unconfined launch binds no socket while the harness runs.
- "amem inside" with a requests directory that is too long fails, and
  the error gives the length. This test does not need namespaces, so
  CI runs it.
- After "amem inside" and after a confined launch, the socket is gone.
- A stale socket is replaced; a live socket, a regular file and a
  symlink are refused and kept.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sstriker
sstriker requested review from andrewmusselman and a balanced review from Copilot October 24, 2026 13:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The change alters the security-critical egress/confinement socket-binding and launch sequencing, which warrants final human review even though no concrete defects were found.

No open issues 🎉

What changed in this PR

This PR fixes a launch failure mode found by the harness-container survey (#2): when the egress unix socket path (<root>/state/run/egress-<pid>.sock) exceeds the platform limit (108 bytes on Linux, 104 on macOS), the old code started the serve task, waited up to two seconds for the socket file, and continued silently, leaving the user with an unrelated, confusing error. The change makes socket binding happen synchronously before the serve task starts, so a bind failure stops the launch with a clear, length-naming error. It also centralizes serving behind a generic serve(listener) and applies the same bind-before-run discipline to the unconfined TCP runner and the relay.

Changes:

  • Add Egress::bind_unix (path-length check via a SUN_PATH const computed from libc::sockaddr_un, plus stale/live/non-socket handling in remove_stale_socket) and a generic Egress::serve(listener); refactor serve_unix/serve_tcp to use them.
  • Rework Session::start to bind before spawning the serve task, make serve_unix optional (unconfined binds no unix socket), keep the unconfined TCP listener instead of rebinding, and make relay bind its loopback port before running the harness.
  • Add comprehensive tests for the length boundary, stale/live/non-socket cases, confined/unconfined launches from a long root, the relay failure path, and socket cleanup.
File Description
crates/​egress/​src/​lib.rs New SocketPath error, SUN_PATH const, bind_unix, generic serve, remove_stale_socket, and supporting unit tests.
crates/​memory/​src/​main.rs Bind socket before spawning serve task; optional serve_unix; unconfined keeps its TCP listener; relay binds before running the harness.
crates/​memory/​tests/​launch.rs New integration tests for long-root launches, relay failure, socket cleanup; inside helper extracted.
crates/​egress/​Cargo.toml Adds unix-only libc dependency for sockaddr_un sizing.
Cargo.lock Records libc as a dependency of dm-egress.

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The change modifies confinement-sensitive subsystems (the egress "one way out", launcher socket-binding order, and the in-sandbox relay), which warrant final human review despite clean, well-tested code.

No open issues 🎉

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The changes modify the egress socket boundary and the in-sandbox relay, which are security-critical confinement paths that warrant final human review despite the thorough tests.

No open issues 🎉

Review effort: Balanced

@sstriker
sstriker merged commit e8a3148 into main Oct 24, 2026
9 checks passed
@sstriker
sstriker deleted the fix/egress-socket-bind branch October 24, 2026 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants