Fix potential pipe deadlock in shell tool execution#140
Fix potential pipe deadlock in shell tool execution#140ilblackdragon merged 1 commit intonearai:mainfrom
Conversation
Summary of ChangesHello @AI-Reviewer-QS, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical deadlock scenario in the shell tool execution where commands generating output larger than OS pipe buffers could cause the process to hang indefinitely. The solution involves a robust restructuring of how child process output streams are handled, allowing for concurrent reading of stdout and stderr while waiting for the process to complete. This significantly enhances the reliability of shell command execution, particularly for long-running or verbose processes, and includes improved command approval mechanisms. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully addresses a critical deadlock vulnerability in shell command execution by concurrently reading stdout/stderr for large outputs, and includes a dedicated test case. However, a significant security concern remains: the pattern matching used to detect destructive commands is based on simple string containment, which is easily bypassed, allowing sensitive operations to proceed without explicit approval. I've also provided suggestions to enhance code clarity by using more idiomatic Rust for draining output, improve test accuracy by asserting the exact truncated output size, and promote better code reuse by extracting command extraction logic.
AI-Reviewer-QS
force-pushed
the
fix/shell-pipe-deadlock
branch
4 times, most recently
from
aeee6a7 to
a1c4ab5
Compare
sumleo
force-pushed
the
fix/shell-pipe-deadlock
branch
from
a1c4ab5 to
dabb097
Compare
|
You have LLM folder changes which are not relevant to this PR |
Drain stdout and stderr concurrently with child.wait() using tokio::join to prevent deadlocks when command output exceeds the OS pipe buffer (64KB on Linux, 16KB on macOS). Use AsyncReadExt::take() for memory-bounded reads and tokio::io::copy to sink for draining excess output. Add regression test that generates 128KB of output to verify the fix prevents deadlocks.
AI-Reviewer-QS
force-pushed
the
fix/shell-pipe-deadlock
branch
from
5a2d692 to
36bd9fc
Compare
|
Removed the unrelated LLM formatting changes — the PR now only touches |
Drain stdout and stderr concurrently with child.wait() using tokio::join to prevent deadlocks when command output exceeds the OS pipe buffer (64KB on Linux, 16KB on macOS). Use AsyncReadExt::take() for memory-bounded reads and tokio::io::copy to sink for draining excess output. Add regression test that generates 128KB of output to verify the fix prevents deadlocks.
Drain stdout and stderr concurrently with child.wait() using tokio::join to prevent deadlocks when command output exceeds the OS pipe buffer (64KB on Linux, 16KB on macOS). Use AsyncReadExt::take() for memory-bounded reads and tokio::io::copy to sink for draining excess output. Add regression test that generates 128KB of output to verify the fix prevents deadlocks.
2 participants
Summary
In
ShellTool::execute_direct(),child.wait()was called before draining stdout/stderr pipes. When a command produces output exceeding the OS pipe buffer size (~64KB on Linux, ~16KB on macOS), the child process blocks on the write, andwait()never returns — causing a deadlock.Fix: Restructure to drain stdout and stderr concurrently with
child.wait()usingtokio::join!. Pipes are taken from the child handle before entering the timeout block. Each pipe reader usestake(MAX_OUTPUT_SIZE)for memory-bounded reads, followed by a drain loop to consume any remaining output so the child can exit cleanly.Test: Added
test_large_output_commandthat generates 128KB of output (exceeding typical pipe buffer sizes) and verifies the command completes with exit code 0.Changes
src/tools/builtin/shell.rs: Restructure pipe reading and process waiting to run concurrently, add large-output testTest plan
cargo test tools::builtin::shell— all tests pass including new large-output testcargo clippy— no warningscargo fmt --check— formatted