fix(core): Resolve TOCTOU Data Loss Race Condition in MemoryTool Execution#20752
fix(core): Resolve TOCTOU Data Loss Race Condition in MemoryTool Execution#20752KumarADITHYA123 wants to merge 1 commit intogoogle-gemini:mainfrom
Conversation
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 significantly enhances the robustness of the 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 effectively addresses a critical TOCTOU race condition in the MemoryTool, preventing potential data loss when the GEMINI.md file is modified externally during a tool operation. The solution is robust, involving re-reading the file before execution and handling conflicts gracefully. The addition of specific test cases to cover these race condition scenarios is excellent. I have one suggestion to refactor the new conditional logic in the execute method to improve its clarity and maintainability.
| const hasChanged = | ||
| this.originalContent !== undefined && | ||
| currentContent !== this.originalContent; | ||
|
|
||
| if (hasChanged) { | ||
| if (modified_by_user && modified_content !== undefined) { | ||
| throw new Error( | ||
| 'Conflict: GEMINI.md was modified externally while you were editing the prompt. Please review and retry to avoid data loss.', | ||
| ); | ||
| } else { | ||
| contentToWrite = computeNewContent(currentContent, fact); | ||
| successMessage = `Okay, I've remembered that: "${sanitizedFact}"`; | ||
| } | ||
| } else { | ||
| // User approved the proposed change without modification. | ||
| // The source of truth is the exact content proposed during confirmation. | ||
| if (this.proposedNewContent === undefined) { | ||
| // This case can be hit in flows without a confirmation step (e.g., --auto-confirm). | ||
| // As a fallback, we recompute the content now. This is safe because | ||
| // computeNewContent sanitizes the input. | ||
| const currentContent = await readMemoryFileContent(); | ||
| this.proposedNewContent = computeNewContent(currentContent, fact); | ||
| if (modified_by_user && modified_content !== undefined) { | ||
| // User modified the content, so that is the source of truth. | ||
| contentToWrite = modified_content; | ||
| successMessage = `Okay, I've updated the memory file with your modifications.`; | ||
| } else { | ||
| // User approved the proposed change without modification. | ||
| // The source of truth is the exact content proposed during confirmation. | ||
| if (this.proposedNewContent === undefined) { | ||
| // This case can be hit in flows without a confirmation step (e.g., --auto-confirm). | ||
| // As a fallback, we recompute the content now. This is safe because | ||
| // computeNewContent sanitizes the input. | ||
| this.proposedNewContent = computeNewContent(currentContent, fact); | ||
| } | ||
| contentToWrite = this.proposedNewContent; | ||
| successMessage = `Okay, I've remembered that: "${sanitizedFact}"`; | ||
| } | ||
| contentToWrite = this.proposedNewContent; | ||
| successMessage = `Okay, I've remembered that: "${sanitizedFact}"`; | ||
| } |
There was a problem hiding this comment.
The nested if/else structure to handle the race condition is logically correct but adds significant cognitive complexity by intertwining conflict detection with the different logic paths for writing content.
This can be refactored to be more linear and easier to reason about. By first checking for the unrecoverable conflict and exiting, and then handling the remaining cases, we can flatten the logic. This improves long-term maintainability and reduces the risk of introducing bugs in this critical section.
const hasChanged =
this.originalContent !== undefined &&
currentContent !== this.originalContent;
// Handle the unrecoverable conflict first.
if (hasChanged && modified_by_user && modified_content !== undefined) {
throw new Error(
'Conflict: GEMINI.md was modified externally while you were editing the prompt. Please review and retry to avoid data loss.',
);
}
// Determine the content to write and the success message for the remaining cases.
if (modified_by_user && modified_content !== undefined) {
// Case: User provided custom modifications, and no external conflict occurred.
contentToWrite = modified_content;
successMessage = `Okay, I've updated the memory file with your modifications.`;
} else {
// Case: No user modifications. Handle external changes or use cached content.
if (hasChanged) {
// The file changed externally, so we recompute the content from the new state.
contentToWrite = computeNewContent(currentContent, fact);
} else {
// The file has not changed, so we can safely use the cached proposed content.
if (this.proposedNewContent === undefined) {
// Fallback for flows without a confirmation step (e.g., --auto-confirm).
this.proposedNewContent = computeNewContent(currentContent, fact);
}
contentToWrite = this.proposedNewContent;
}
successMessage = `Okay, I've remembered that: "${sanitizedFact}"`;
}
gemini-cli
bot
added
the
area/agent
|
@gemini-code-assist review this pr |
There was a problem hiding this comment.
Code Review
This pull request addresses a TOCTOU race condition in the MemoryTool. The implementation re-validates the file state before writing to prevent data loss from concurrent modifications. My review includes a suggestion to refine the error handling for the new conflict detection logic to provide a clearer error message to the user.
| expect(result.llmContent).toContain( | ||
| 'Conflict: GEMINI.md was modified externally', | ||
| ); |
There was a problem hiding this comment.
To accompany the suggested change of returning a ToolResult directly for the conflict error, this test's assertions should be updated to be more specific. Instead of a loose toContain check, it's better to assert the exact llmContent and returnDisplay values to ensure the error message is formatted correctly and prevent future regressions.
const expectedError = 'Conflict: GEMINI.md was modified externally while you were editing the prompt. Please review and retry to avoid data loss.';
expect(result.returnDisplay).toBe(expectedError);
expect(result.llmContent).toBe(
JSON.stringify({ success: false, error: expectedError })
);| if (hasChanged && modified_by_user && modified_content !== undefined) { | ||
| throw new Error( | ||
| 'Conflict: GEMINI.md was modified externally while you were editing the prompt. Please review and retry to avoid data loss.', | ||
| ); | ||
| } |
There was a problem hiding this comment.
While throwing an error here is functionally correct, it results in a redundant and verbose error message for the user due to the generic catch block (e.g., Error saving memory: Conflict...).
To provide a cleaner and more direct error message, this if block can be modified to return the ToolResult object directly instead of throwing. This avoids the generic error prefixing and presents the specific, user-friendly conflict message you've crafted.
if (hasChanged && modified_by_user && modified_content !== undefined) {
const errorMessage =
'Conflict: GEMINI.md was modified externally while you were editing the prompt. Please review and retry to avoid data loss.';
return {
llmContent: JSON.stringify({
success: false,
error: errorMessage
}),
returnDisplay: errorMessage,
error: {
message: errorMessage,
type: ToolErrorType.MEMORY_TOOL_EXECUTION_ERROR
}
};
}
KumarADITHYA123
force-pushed
the
fix-core-toctou-memorytool
branch
from
b8215f8 to
69aec1c
Compare
|
@gemini-code-assist review this pr |
There was a problem hiding this comment.
Code Review
This pull request enhances the MemoryTool by implementing robust conflict detection and resolution for the GEMINI.md memory file. It introduces logic to detect external modifications to the memory file between the confirmation and execution phases of a memory operation. If the file changes externally and the user also provided custom modifications, the tool now aborts the operation and returns an error to prevent data loss. If the file changes externally but the user did not provide custom modifications, the tool dynamically recomputes the content based on the latest file state, effectively merging the new fact. A review comment suggests an improvement opportunity: when the tool silently merges a new fact due to external file modifications, the user should receive a more informative success message to improve the user experience.
| if (hasChanged) { | ||
| // The file changed externally, so we recompute the content from the new state. | ||
| contentToWrite = computeNewContent(currentContent, fact); | ||
| } else { | ||
| // The file has not changed, so we can safely use the cached proposed content. | ||
| if (this.proposedNewContent === undefined) { | ||
| // Fallback for flows without a confirmation step (e.g., --auto-confirm). | ||
| this.proposedNewContent = computeNewContent(currentContent, fact); | ||
| } | ||
| contentToWrite = this.proposedNewContent; | ||
| } | ||
| contentToWrite = this.proposedNewContent; | ||
| successMessage = `Okay, I've remembered that: "${sanitizedFact}"`; |
There was a problem hiding this comment.
When the memory file is modified externally (hasChanged is true), the tool silently merges the new fact. This could be confusing for the user as they are not notified of the merge. While this prevents data loss, the lack of feedback about the merge operation is a significant user experience issue. It would be better to provide a more informative success message in this scenario to make the user aware that their change was applied on top of other modifications.
if (hasChanged) {
// The file changed externally, so we recompute the content from the new state.
contentToWrite = computeNewContent(currentContent, fact);
successMessage = `Okay, I've remembered that: "${sanitizedFact}". Note: The memory file was modified externally and your change was merged.`;
} else {
// The file has not changed, so we can safely use the cached proposed content.
if (this.proposedNewContent === undefined) {
// Fallback for flows without a confirmation step (e.g., --auto-confirm).
this.proposedNewContent = computeNewContent(currentContent, fact);
}
contentToWrite = this.proposedNewContent;
successMessage = `Okay, I've remembered that: "${sanitizedFact}"`;
}
KumarADITHYA123
force-pushed
the
fix-core-toctou-memorytool
branch
from
69aec1c to
cbcbbbc
Compare
|
@gemini-code-assist LGTM? |
|
Hi @KumarADITHYA123, thanks for asking! My initial summary in comment #1 serves as my automated review of this pull request. The "LGTM" (Looks Good To Me) is typically a signal from a human reviewer indicating their approval for merging. As an automated assistant, I don't provide a direct "LGTM" for merging, but rather highlight key changes and potential areas of concern. If you'd like a more detailed, line-by-line code review with specific suggestions, please use the |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses a critical Time-of-Check to Time-of-Use (TOCTOU) race condition in the MemoryTool to prevent data loss. The implemented solution correctly re-validates the memory file at the time of execution. The logic properly handles conflicts by either merging changes or aborting the operation when necessary. The new tests adequately cover the introduced scenarios. The changes are well-implemented and resolve the described vulnerability.
There was a problem hiding this comment.
Code Review
This pull request addresses a Time-of-Check to Time-of-Use (TOCTOU) race condition in MemoryTool. The fix involves caching the file content at confirmation time and re-reading it at execution time to detect any modifications that occurred in the interim. The logic correctly handles two key scenarios: it automatically merges the user's intended change if the file was modified externally without a direct user edit, and it aborts the operation with a conflict error if both an external modification and a user edit occurred, thus preventing data loss. The new tests adequately cover these conflict resolution paths. I have reviewed the implementation and have not found any high or critical severity issues.
|
@jacob314,@spencer426 @mrpmohiburrahman The automated review is complete and flags zero remaining issues. I've pushed a single squashed commit that incorporates the requested conflict resolution updates:
Let me know if you need any adjustments to the re-validation logic before merging. |
|
Hi there! Thank you for your interest in contributing to Gemini CLI. To ensure we maintain high code quality and focus on our prioritized roadmap, we have updated our contribution policy (see Discussion #17383). We only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'. All other community pull requests are subject to closure after 14 days if they do not align with our current focus areas. For this reason, we strongly recommend that contributors only submit pull requests against issues explicitly labeled as 'help-wanted'. This pull request is being closed as it has been open for 14 days without a 'help wanted' designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding and for being part of our community! |
1 participant
Fixes #20746
ARCHITECTURAL OVERVIEW
This pull request addresses a critical Time-of-Check to Time-of-Use (TOCTOU) vulnerability within the MemoryTool lifecycle.
Prior to this patch, the MemoryTool eagerly read the GEMINI.md file during the confirmation and diff-generation phase, caching the proposed overwrite state. If the user confirmed the edit, the tool blindly wrote the cached string to disk in the execution phase.
If the user modified GEMINI.md directly during the confirmation prompt, or if a multi-agent parallel execution modified the file context concurrently, the MemoryTool would forcefully overwrite and vaporize those intermediate changes, leading to silent data loss.
THE FIX
The solution introduces a strict re-validation layer at the execution boundary.
A. If the file has mutated and the user did not manually edit the proposed memory chunk, I seamlessly recompute the diff against the newly discovered state and apply it safely.
B. If the user injected a manual string edit during the prompt and the backing file has changed, I gracefully abort the operation to prevent a blind overwrite, prompting the user to resolve the collision safely.
TEST COVERAGE
Two novel test cases have been added to memoryTool.test.ts to explicitly simulate the TOCTOU concurrency race and verify the dynamic re-evaluation boundary.
I am happy to adjust the conflict resolution strategy if the maintainers prefer an alternative approach to handling user-modified collisions. Thank you for your time reviewing this architectural patch.