Skip to content

fix(tui): prevent underflow in dropdown navigation and scroll calculations#64

Closed
echobt wants to merge 7 commits intomainfrom
fix/dropdown-underflow
Closed

fix(tui): prevent underflow in dropdown navigation and scroll calculations#64
echobt wants to merge 7 commits intomainfrom
fix/dropdown-underflow

Conversation

@echobt
Copy link
Contributor

@echobt echobt commented Feb 4, 2026

Summary

Fixes #5182, #5180, #5176 - Dropdown underflow and panic issues.

Problems

  1. Division by zero when max_visible=0
  2. Subtraction underflow in scroll offset
  3. Index panic in visible_items

Solution

Added guards for zero values and used saturating arithmetic.

Changes

  • dropdown.rs: Added guard for max_visible=0 in select_next/select_prev
  • scroll.rs: Added guard for visible=0 and used saturating_sub in ensure_visible
  • scrollable_dropdown.rs:
    • Fixed visible_items() to handle edge cases safely with .get()
    • Added max_visible=0 guard in select_next/select_prev/calculate_scroll_offset
    • Used saturating_sub to prevent underflow

Fixes #5292 and #5293 - ToolResponseStore memory issues.

Problem:
1. Store grows without limit
2. Consumed responses not removed

Solution:
- Added ToolResponseStore with configurable max size (default: 500 entries)
- Entries are removed when consumed via take() method (#5293)
- Automatic periodic cleanup of expired entries based on TTL
- Eviction of oldest entries when capacity is reached (#5292)

Features:
- MAX_STORE_SIZE constant (500) to prevent unbounded growth
- DEFAULT_TTL (5 minutes) for automatic expiration
- CLEANUP_INTERVAL (1 minute) for periodic cleanup
- get() for peeking without removal
- take() for consuming and removing entries
- cleanup_expired() and cleanup_read() for manual cleanup
- Stats tracking for monitoring store behavior
- server.rs: Hold write lock during entire state check and modification
  in handle_initialize to prevent concurrent initialization races
- registry.rs: Use HashMap entry API to atomically check-and-insert
  plugin registration to prevent duplicate registration races

Fixes #5262, #5260
Fixes bypass attempts where blocked commands like 'rm -rf' could be
evaded using:
- Extra whitespace: 'rm  -rf' → 'rm -rf'
- Quoted parts: "'rm' -rf" → 'rm -rf'
- Path variants: '/bin/rm -rf' → 'rm -rf'

Added normalize_command() function that:
1. Collapses whitespace by splitting/joining
2. Strips surrounding quotes from command parts
3. Extracts basename for command (first part)

Also added comprehensive tests for bypass scenarios.
- Fix #5181: validate_path_safety() now checks path components instead of
  substring match, preventing false positives for filenames like 'file..txt'
- Fix #5183: expand_tilde() now handles bare '~' path in addition to '~/' prefix
- Updated tests to verify correct behavior
Fixes #5208, #5210, #5244 - Session storage error handling improvements.

Changes:
- list_sessions: Now properly collects and reports IO errors instead of silently
  ignoring them. Returns available sessions while logging errors for failed ones.
- SessionManager: Only updates in-memory state after successful storage operations.
  If storage fails, the session is marked as modified for later retry.
- Default impl: Improved panic message to help diagnose initialization failures
  (permissions, disk space).
…tions

Fixes #5182, #5180, #5176

- Add guard for max_visible=0 in dropdown select_next/select_prev
- Use saturating_sub in scroll.ensure_visible to prevent underflow
- Add bounds checking in scrollable_dropdown visible_items
- Guard against max_visible=0 in select_next/select_prev/calculate_scroll_offset
@greptile-apps
Copy link

greptile-apps bot commented Feb 4, 2026

Greptile Overview

Greptile Summary

This PR fixes critical underflow and division-by-zero issues in TUI dropdown navigation (fixes #5182, #5180, #5176) along with several other robustness improvements across the codebase.

Key Changes

TUI Dropdown Fixes (Core Issue)

  • Added max_visible == 0 guards in dropdown.rs, preventing modulo division by zero in navigation
  • Added visible == 0 guard in scroll.rs ensure_visible() to prevent underflow
  • Used nested saturating_sub operations: index.saturating_sub(visible.saturating_sub(1)) to safely handle edge cases
  • Fixed visible_items() in scrollable_dropdown.rs to use .get() for safe slicing instead of panicking index operations

Additional Robustness Improvements

  • Path Validation: Fixed validate_path_safety to check path components instead of substring matching, preventing false positives on filenames like file..txt
  • Command Validation: Added normalize_command to prevent validation bypass via whitespace, quotes, or path variants (e.g., /bin/rm vs rm)
  • Config Validation: Added custom deserializers for threshold_percent and sampling_ratio to enforce 0.0-1.0 range at deserialization time
  • Race Condition Fixes: Fixed TOCTOU races in MCP server initialization and plugin registration using atomic check-and-set patterns
  • Tool Response Storage: New bounded storage with capacity limits, TTL, and automatic cleanup (fixes #5292, #5293)
  • Session Storage: Enhanced error handling to continue on errors while logging issues
  • Type Safety: Changed token count fields from i64 to u64 throughout the codebase

Confidence Score: 5/5

  • This PR is safe to merge - all fixes use defensive programming patterns that prevent crashes without changing behavior
  • All changes are defensive improvements using saturating arithmetic, early returns, and safe slicing. The fixes directly address panic conditions with minimal code changes and include comprehensive test coverage
  • No files require special attention - all changes follow consistent defensive patterns

Important Files Changed

Filename Overview
src/cortex-tui-components/src/dropdown.rs Added max_visible == 0 guard in select_next/select_prev to prevent division by zero
src/cortex-tui-components/src/scroll.rs Added visible == 0 guard and nested saturating_sub in ensure_visible to prevent underflow
src/cortex-tui/src/widgets/scrollable_dropdown.rs Fixed visible_items() with .get(), added max_visible == 0 guards, and used nested saturating_sub throughout
src/cortex-engine/src/tools/response_store.rs New bounded storage for tool responses with TTL, capacity limits, and automatic cleanup (fixes #5292, #5293)
src/cortex-engine/src/validation.rs Added normalize_command function to prevent validation bypass via whitespace, quotes, or path variants
src/cortex-mcp-server/src/server.rs Fixed TOCTOU race in handle_initialize by holding write lock during check-and-set
src/cortex-plugins/src/registry.rs Fixed TOCTOU race in plugin registration using HashMap entry API for atomic check-and-insert
src/cortex-tui/src/session/manager.rs Enhanced error handling in add_tokens, pop_last_exchange, and add_message_internal with rollback on failure

Sequence Diagram

sequenceDiagram
    participant User
    participant TUI as TUI Component
    participant Dropdown as Dropdown/Scroll
    participant State as State Manager
    
    Note over User,State: Dropdown Navigation Flow (Fixed)
    User->>TUI: Press Up/Down Key
    TUI->>Dropdown: select_next() / select_prev()
    Dropdown->>Dropdown: Check max_visible == 0 guard
    alt max_visible is 0
        Dropdown-->>TUI: Return early (no crash)
    else max_visible > 0
        Dropdown->>Dropdown: Calculate new selected index
        Dropdown->>State: ensure_visible(index)
        State->>State: Check visible == 0 guard
        alt visible is 0
            State-->>Dropdown: Return early (no crash)
        else visible > 0
            State->>State: Calculate offset with saturating_sub
            Note over State: offset = index.saturating_sub(visible.saturating_sub(1))
            State-->>Dropdown: Offset updated safely
        end
        Dropdown-->>TUI: Selection updated
    end
    TUI->>User: UI updates without panic
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

8 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

echobt added a commit that referenced this pull request Feb 4, 2026
… prevention

This PR consolidates the following fixes:
- #38: Prevent usize to u16 overflow in interactive renderer
- #42: Prevent usize to u16 overflow in card count displays
- #58: Fix cursor positioning and underflow in selection list
- #59: Fix mention popup positioning and Unicode width calculation
- #60: Improve autocomplete popup positioning and width calculation
- #64: Prevent underflow in dropdown navigation and scroll calculations
- #66: Prevent panic in HelpBrowserState when sections empty

All changes target the TUI components to improve robustness:
- Added saturating casts for u16 conversions
- Fixed cursor positioning calculations
- Added bounds checking for empty sections
- Improved Unicode width handling for popups
@echobt
Copy link
Contributor Author

echobt commented Feb 4, 2026

Consolidated into #69 - fix(tui): consolidated TUI fixes for overflow, positioning, and panic prevention

@echobt echobt closed this Feb 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant