fix(core): remove synchronous I/O from shell tool critical path#28397
fix(core): remove synchronous I/O from shell tool critical path#28397Daksh7785 wants to merge 2 commits into
Conversation
This fixes the UI stutter issue google-gemini#28395 where React Ink terminal frames were being paused due to synchronous fs calls like mkdtempSync and existsSync.
Summary of ChangesHello, 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 performance issues in the CLI's terminal UI by eliminating blocking synchronous filesystem calls. By migrating these operations to asynchronous promises, the application avoids stuttering and freezing caused by event loop contention during process spawning and permission validation. Highlights
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 the 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 counterproductive. 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. Footnotes
|
|
📊 PR Size: size/M
|
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
🛑 Action Required: Evaluation ApprovalSteering changes have been detected in this PR. To prevent regressions, a maintainer must approve the evaluation run before this PR can be merged. Maintainers:
Once approved, the evaluation results will be posted here automatically. |
![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 refactors the shell tool implementation in packages/core/src/tools/shell.ts to use asynchronous file system operations (fsPromises) instead of synchronous ones, and parallelizes the sandbox denial path resolution using Promise.all. However, a high-severity infinite loop vulnerability was identified in the path traversal logic on Windows, where reaching a root directory (e.g., C:\) can cause an infinite loop because path.dirname returns the same path. A code suggestion has been provided to safely break the loop when the parent directory equals the current path.
| while (currentPath.length > 1) { | ||
| const exists = await fsPromises | ||
| .access(currentPath) | ||
| .then(() => true) | ||
| .catch(() => false); | ||
| if (exists) { | ||
| const mode = this.context.config.getApprovalMode(); | ||
| const isReadonlyMode = | ||
| this.context.config.sandboxPolicyManager.getModeConfig( | ||
| mode, | ||
| )?.readonly ?? false; | ||
| const isAllowed = | ||
| this.context.config.isPathAllowed(currentPath); | ||
|
|
||
| if (!isAllowed || isReadonlyMode) { | ||
| writePaths.add(currentPath); | ||
| readPaths.add(currentPath); | ||
| } | ||
| break; | ||
| } | ||
| currentPath = path.dirname(currentPath); | ||
| } |
There was a problem hiding this comment.
An infinite loop / Denial of Service (DoS) vulnerability exists in the sandbox denial path resolution logic on Windows. When traversing up the directory tree using path.dirname(currentPath), if currentPath resolves to a non-existent root path (e.g., C:\ or a non-existent drive like Z:\) or an inaccessible UNC path, fsPromises.access will fail and path.dirname will return the same root path. Since the length of the root path is greater than 1, the loop condition while (currentPath.length > 1) remains true, causing an infinite loop that consumes 100% CPU, freezes the application, and hangs the shell tool execution indefinitely.
To remediate this, check if the parent directory returned by path.dirname(currentPath) is equal to currentPath (indicating that the root directory has been reached and no further traversal is possible) and break the loop.
while (currentPath.length > 1) {
const exists = await fsPromises
.access(currentPath)
.then(() => true)
.catch(() => false);
if (exists) {
const mode = this.context.config.getApprovalMode();
const isReadonlyMode =
this.context.config.sandboxPolicyManager.getModeConfig(
mode,
)?.readonly ?? false;
const isAllowed =
this.context.config.isPathAllowed(currentPath);
if (!isAllowed || isReadonlyMode) {
writePaths.add(currentPath);
readPaths.add(currentPath);
}
break;
}
const parent = path.dirname(currentPath);
if (parent === currentPath) {
break;
}
currentPath = parent;
}
gemini-cli
Bot
added
priority/p2
1 participant
Description
This PR replaces blocking synchronous filesystem operations (
fs.mkdtempSync,fs.existsSync,fs.statSync) inpackages/core/src/tools/shell.tswith their asynchronous counterparts fromnode:fs/promises.Problem
The React Ink terminal UI in the CLI was stuttering and freezing because the single-threaded Node.js event loop was being blocked by synchronous file system operations executed just before spawning processes.
Solution
fs.mkdtempSyncwithawait fsPromises.mkdtemp.await Promise.allalongsidefsPromises.accessandfsPromises.statto ensure non-blocking execution.node:fsimports.Fixes #28395