Conversation
This commit introduces a memory limit to the `ModificationTracker` to prevent unbounded memory growth in the undo history. Changes: - Added `calculateSize` helper to estimate the memory footprint of modifications. - Updated `ModificationTracker` to track total size of history entries. - Added `maxMemory` configuration (default 50MB). - Implemented eviction logic in `record()` to remove oldest entries when memory limit is exceeded. - Updated `deserialize` to recalculate entry sizes. - Added unit tests in `tests/unit/undo_memory_limit.test.ts`. This fixes a potential security vulnerability where large modifications could cause the extension to consume excessive memory. Co-authored-by: zknpr <96851588+zknpr@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @zknpr, 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 and security of the application by introducing memory limits to the undo history mechanism. Previously, large undo operations could lead to excessive memory consumption and potential Denial of Service. The changes now actively monitor and control the memory footprint of undo entries, ensuring that the system remains stable and responsive even under heavy usage, while preserving the essential undo/redo functionality. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 security vulnerability by introducing a memory limit to the undo history, preventing unbounded memory growth. The implementation, including the size calculation and eviction logic, is well-conceived and the accompanying tests cover the new functionality appropriately. I've identified a couple of areas where the implementation could be made more robust against potential state desynchronization, which could undermine the memory accounting. I've also included a note on a potential performance bottleneck in the eviction logic for your consideration. Overall, this is a solid and important fix.
|
|
||
| if (entry) { | ||
| this.futureStack.push(entry); | ||
| this.futureStackSizes.push(size || 0); |
There was a problem hiding this comment.
There's a potential issue here if timelineSizes becomes desynchronized from timeline, causing size to be undefined. Pushing 0 in this case would lead to an incorrect currentSize over time, partially negating the memory-limiting fix. To make this more robust, it's safer to recalculate the size of the entry if it's missing from the timelineSizes array. This ensures memory accounting remains accurate even if a desynchronization bug occurs.
| this.futureStackSizes.push(size || 0); | |
| this.futureStackSizes.push(size ?? calculateSize(entry)); |
|
|
||
| if (entry) { | ||
| this.timeline.push(entry); | ||
| this.timelineSizes.push(size || 0); |
There was a problem hiding this comment.
Similar to the stepBack method, if size is undefined due to a potential desynchronization between futureStack and futureStackSizes, pushing 0 will lead to incorrect memory accounting. Recalculating the size as a fallback will make the system more resilient against state corruption.
| this.timelineSizes.push(size || 0); | |
| this.timelineSizes.push(size ?? calculateSize(entry)); |
| while ( | ||
| (this.timeline.length > 0) && | ||
| (this.timeline.length > this.maxEntries || this.currentSize > this.maxMemory) | ||
| ) { | ||
| // Don't remove the just-added entry if it's the only one, to preserve ability to undo at least one step if possible. | ||
| // However, if strict memory limit is required, we might need to, but let's be practical. | ||
| if (this.timeline.length === 1) { | ||
| break; | ||
| } | ||
|
|
||
| const removedEntrySize = this.timelineSizes.shift(); | ||
| this.timeline.shift(); | ||
|
|
||
| if (removedEntrySize !== undefined) { | ||
| this.currentSize -= removedEntrySize; | ||
| } | ||
|
|
||
| // Enforce capacity limit | ||
| if (this.timeline.length > this.maxEntries) { | ||
| const overflow = this.timeline.length - this.maxEntries; | ||
| this.timeline.splice(0, overflow); | ||
| this.checkpointIndex = Math.max(0, this.checkpointIndex - overflow); | ||
| // Adjust checkpoint index since we shifted the array | ||
| this.checkpointIndex = Math.max(0, this.checkpointIndex - 1); | ||
| } |
There was a problem hiding this comment.
Using shift() inside a while loop can lead to poor performance for large arrays. Each shift() operation is O(n) because it requires re-indexing all subsequent elements. In a scenario where many small, old entries need to be evicted to make space for one large new entry, this loop's complexity could approach O(k*n), where k is the number of evicted items and n is the array length. While perhaps acceptable for the default maxEntries, this could become a performance bottleneck if the undo history grows large. For a history structure, a more performant data structure like a doubly-linked list would provide O(1) for additions and removals from both ends.
|
Merged as part of v1.3.0 release in PR #65 |
Thank you! Glad to have this security fix merged. |
1 participant
This PR addresses a security vulnerability where the undo history could grow unbounded in memory, potentially leading to a Denial of Service via memory exhaustion.
Risk:
Previously, the undo history was only limited by the number of entries (
maxEntries), not their size. An attacker or a user performing large operations (e.g., bulk updates or inserting large blobs) could fill the history with very large objects, causing the extension to crash or the system to run out of memory.Solution:
I have implemented a memory tracking mechanism in
ModificationTracker(src/core/undo-history.ts):calculateSizefunction estimates the memory usage of each modification entry, accounting for primitives, objects, arrays, andUint8Arrays.maxMemoryparameter (defaulting to 50MB) is added to theModificationTracker.maxMemory(or the count exceedsmaxEntries), the oldest entries are removed until the history fits within the limits.Verification:
tests/unit/undo_memory_limit.test.tswhich verifies:maxMemoryis exceeded.npm testpasses (after fixing dependency resolution withnpm install).PR created automatically by Jules for task 7224620587777115643 started by @zknpr