Conversation
bolinfest
changed the title
bolinfest
force-pushed
the
pr14989
branch
10 times, most recently
from
753a0bd to
2d5a531
Compare
bolinfest
changed the title
bolinfest
force-pushed
the
pr14989
branch
5 times, most recently
from
bfe54b7 to
40ba316
Compare
bolinfest
changed the title
bolinfest
force-pushed
the
pr14989
branch
3 times, most recently
from
5d9ef97 to
29d2549
Compare
bolinfest
changed the title
bolinfest
force-pushed
the
pr14989
branch
4 times, most recently
from
1ed9cf6 to
ad6148a
Compare
bolinfest
force-pushed
the
pr14989
branch
5 times, most recently
from
7bda055 to
9e9ec89
Compare
bolinfest
force-pushed
the
pr14989
branch
5 times, most recently
from
45b4632 to
98ea221
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98ea221446
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| SandboxedFsOperation::Read { ref path } => vec![ | ||
| CODEX_CORE_FS_OPS_ARG1.to_string(), | ||
| READ_FILE_OPERATION_ARG.to_string(), | ||
| path.to_string_lossy().to_string(), | ||
| ], |
There was a problem hiding this comment.
On Unix, AbsolutePathBuf/PathBuf can represent filenames that are not valid UTF-8, but this code converts the path with to_string_lossy() before passing it through argv. In that case the helper receives a different pathname (with replacement characters) and fails to open an existing file. fs-ops::parse_command_from_args already accepts OsString, so this re-exec path should preserve raw path bytes instead of round-tripping through String.
Useful? React with 👍 / 👎.
codex-rs/fs-ops/src/runner.rs
Outdated
| }) | ||
| .map_err(std::io::Error::other)?; | ||
|
|
||
| execute(command, stdin, stdout) |
There was a problem hiding this comment.
Parse failures are written to stderr, but runtime I/O failures from File::open/copy are returned directly here and then dropped by arg0_dispatch when it turns Err(_) into exit code 1. That means common cases like ENOENT/EACCES reach sandboxed_fs::parse_helper_failure with empty stdout/stderr and get reported as no error details emitted, which makes the helper much harder to diagnose once callers start using it.
Useful? React with 👍 / 👎.
codex-rs/core/src/sandboxed_fs.rs
Outdated
| expiration: ExecExpiration::Timeout(SANDBOXED_FS_TIMEOUT), | ||
| capture_policy: ExecCapturePolicy::FullBuffer, |
There was a problem hiding this comment.
This helper is configured with ExecCapturePolicy::FullBuffer, and core/src/exec.rs disables both the retained-bytes cap and ExecExpiration for that mode. Because fs-ops/src/runner.rs then copies the opened path to stdout until EOF, calling read_file on a FIFO, /dev/zero, or a procfs pseudo-file will never finish and can grow memory without bound. Since read_file accepts arbitrary paths, it needs a regular-file check and/or its own size/timeout guard before any consumer starts routing user paths through it.
Useful? React with 👍 / 👎.
bolinfest
force-pushed
the
pr14989
branch
6 times, most recently
from
b6af173 to
29633f0
Compare
1 participant
Why
Local filesystem reads were still happening in-process, which meant they could bypass the sandbox boundary that exec actually enforces. If filesystem access is supposed to be governed by the same sandbox machinery as command execution, the read itself needs to happen in a sandboxed child process rather than through host-side
std::fscalls.This PR is intentionally the infra-only base of the stack. It adds the helper and execution plumbing needed for sandbox-backed file reads; the
view_imagemigration lives in #15213, and stdin-backed write plumbing lives in #15187.What changed
codex-fs-opscrate, plus the Cargo/Bazel wiring for it, to host a small helper that currently supportsread PATHand streams the file to stdoutcodex-arg0to re-exec the current binary in that helper mode via--codex-run-as-fs-opsstderr, so callers get a concrete error instead of hanging on special files such as/dev/zeroor losing the underlying I/O failurecore/src/sandboxed_fs.rs, which builds a sandboxedCommandSpecfor that helper and exposesread_file()for follow-up callerscore/src/exec.rsandcore/src/sandboxing/mod.rswith a raw-output entry point so trusted internal helpers can capture stdout as bytes instead of lossy text while still reusing the existingFullBuffercapture policyTesting
cargo test -p codex-fs-opsStack created with Sapling. Best reviewed with ReviewStack.