Repository navigation
Conversation
![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: 429537c2a2
ℹ️ 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".
| + | ||
| + larray = strvec_len (args); | ||
| + | ||
| + memmove (args + 2, args, (++larray) * sizeof (char *)); |
There was a problem hiding this comment.
When EXEC_WRAPPER is set, args is the exact NULL-terminated vector produced for the command (for example strvec_from_word_list(words, 0, 0, ...) allocates only count + 1 slots). This in-place memmove shifts the existing entries two positions to the right without first resizing or allocating a larger argv, so any external command run under the patched bash can write past the argv allocation and corrupt memory before reaching execve; allocate/resize the vector before inserting the wrapper arguments.
Useful? React with 👍 / 👎.
| cd "$source_root" | ||
| git checkout "$bash_commit" | ||
| git apply "${workspace}/${bash_patch}" | ||
| ./configure --without-bash-malloc |
There was a problem hiding this comment.
In the Linux matrix these artifacts are published as x86_64-unknown-linux-musl/aarch64-unknown-linux-musl, but the invoked build script only runs a native ./configure --without-bash-malloc and make with the Ubuntu container's default toolchain. That produces glibc-linked binaries while the release asset and DotSlash manifest advertise musl targets, so users on musl-based Linux distributions will download an artifact that cannot run there; set a musl compiler/host for these Linux jobs or rename the artifacts to the actual libc target.
Useful? React with 👍 / 👎.
| + memmove (args + 2, args, (++larray) * sizeof (char *)); | ||
| + args[0] = exec_wrapper; | ||
| + args[1] = orig_command; | ||
| + command = exec_wrapper; |
There was a problem hiding this comment.
When EXEC_WRAPPER is present, this changes the execve target from the user's command to the wrapper before bash reaches its normal ENOEXEC handling. For an executable text script without a shebang, unpatched bash gets ENOEXEC from the script and interprets it as a shell script, but the patched bash successfully execs the wrapper and the wrapper's later execv failure is not handled by bash, so the same script stops working under shell escalation.
Useful? React with 👍 / 👎.
| + char *exec_wrapper = getenv("EXEC_WRAPPER"); | ||
| + if (exec_wrapper && *exec_wrapper && !whitespace (*exec_wrapper)) |
There was a problem hiding this comment.
Because the patch consults the mutable shell environment for every exec, any command that runs unset EXEC_WRAPPER or exports it empty before a protected subcommand causes this condition to fail and the later command is executed by bash directly, without contacting codex-execve-wrapper or the escalation policy. Cache the wrapper path at shell startup or otherwise make this value immutable for the session so shell builtins/startup files cannot bypass subcommand approval.
Useful? React with 👍 / 👎.
| + larray = strvec_len (args); | ||
| + | ||
| + memmove (args + 2, args, (++larray) * sizeof (char *)); |
There was a problem hiding this comment.
Can we resize args before shifting it? Upstream Bash's strvec_from_word_list allocates exactly argc + 1 slots, so this writes two pointers past the allocation on every wrapped exec. This keeps the existing argv intact:
| + larray = strvec_len (args); | |
| + | |
| + memmove (args + 2, args, (++larray) * sizeof (char *)); | |
| larray = strvec_len (args); | |
| args = strvec_resize (args, larray + 3); | |
| memmove (args + 2, args, (larray + 1) * sizeof (char *)); |
| matrix: | ||
| include: | ||
| - runner: ubuntu-24.04 | ||
| target: x86_64-unknown-linux-musl |
There was a problem hiding this comment.
These jobs publish native Ubuntu binaries under musl names. I checked the Bash binary produced by this exact source, patch, and Ubuntu 24.04 recipe: its interpreter is /lib64/ld-linux-x86-64.so.2 and it references GLIBC 2.38, so it will not run on musl or older supported Linux hosts. Can we build with a real musl toolchain and assert the ELF interpreter/linkage before upload, or rename and constrain these assets to the glibc baseline they actually support?
| name: Build bash (macOS) - ${{ matrix.variant }} - ${{ matrix.target }} | ||
| needs: metadata | ||
| runs-on: ${{ matrix.runner }} | ||
| timeout-minutes: 30 |
There was a problem hiding this comment.
Can we pin the deployment target here? I checked the Bash binary previously built from this exact source and recipe on macOS 15; it declares minos 15.0, while Codex supports macOS 12+.
| timeout-minutes: 30 | |
| timeout-minutes: 30 | |
| env: | |
| MACOSX_DEPLOYMENT_TARGET: "12.0" |
It would also be good to assert the resulting LC_BUILD_VERSION before upload.
| + memmove (args + 2, args, (++larray) * sizeof (char *)); | ||
| + args[0] = exec_wrapper; | ||
| + args[1] = orig_command; | ||
| + command = exec_wrapper; |
There was a problem hiding this comment.
This drops Bash's fallback for executable text files without a shebang. I reproduced it with the Bash binary built from this patch: the file runs normally with EXEC_WRAPPER unset, but fails with Exec format error once the wrapper is enabled because the later execv returns to the wrapper after Bash has been replaced. Can we preserve the ENOEXEC fallback in the real wrapper and add this case to the smoke test?
| size_t larray; | ||
|
|
||
| + char *exec_wrapper = getenv("EXEC_WRAPPER"); | ||
| + if (exec_wrapper && *exec_wrapper && !whitespace (*exec_wrapper)) |
There was a problem hiding this comment.
EXEC_WRAPPER is mutable shell state, so unset EXEC_WRAPPER; /usr/bin/true skips the wrapper completely. I reproduced that against the shipped Bash built from this patch: the command succeeds and the wrapper is never called. This stays inside the base sandbox, but it bypasses the subcommand approval/deny path. Can we cache the trusted wrapper path when the shell starts and fail closed if the session variables are cleared or overridden? A smoke test for unset and per-command reassignment would pin this down.
|
Closing this pull request because it has had no updates for more than 14 days. If you plan to continue working on it, feel free to reopen or open a new PR. |
2 participants
Why
Bash fork support was removed in #15644, but bringing it back should not require rebuilding an effectively unchanged patched shell for every Rust release. Following the zsh artifact split in #30114, this establishes the same independently versioned, prerelease artifact pipeline for Bash. A later change can consume these artifacts from the Codex package.
Bash and zsh also need one shared wrapper contract so an escalation socket remains usable through a process tree containing either patched shell.
What changed
rust-release-bashworkflow triggered by immutablecodex-bash-vX.Y.Ztags.EXEC_WRAPPERpatch and document its pinned upstream commit and release process.EXEC_WRAPPERinterception while preserving the inheritedCODEX_ESCALATE_SOCKETfile descriptor in both Bash and zsh artifact builds.codex-zsh-v0.1.0release.Tag protection
Verified with
gh api repos/openai/codex/rulesets/{id}that the repository rulesets for bothcodex-bash-v*.*.*andcodex-zsh-v*.*.*are active. Both protect creation, updates, deletion, force-pushes, and linear history, with matching repository-admin and organization-admin bypasses.Testing