#1094

Windows legacy console: `move_up`/`move_left` underflow when count exceeds the cursor position

Open3x3xX3N0Nopened 3mo ago

Summary

cursor::sys::windows::move_up and move_left subtract the caller's count from the current cursor row/column with a bare u16 subtraction and no floor. If count exceeds the current row or column the subtraction underflows: panic in a debug build, wrap to ~65535 in a release build.

This is the legacy-console path only (reached through execute_winapi, i.e. when supports_ansi is false), so it is Windows-only and does not affect the ANSI path.

The sites

src/cursor/sys/windows.rs on the default branch:

pub(crate) fn move_up(count: u16) -> std::io::Result<()> {
    let (column, row) = position()?;
    move_to(column, row - count)?;          // :59  -- underflows when count > row
    Ok(())
}

pub(crate) fn move_left(count: u16) -> std::io::Result<()> {
    let (column, row) = position()?;
    move_to(column - count, row)?;          // :77  -- underflows when count > column
    Ok(())
}

MoveUp and MoveLeft in src/cursor.rs reach these through execute_winapi with the caller's count unchecked, so queue!(w, MoveUp(5)) with the cursor on row 2 is sufficient to trigger it.

In release the wrapped value then feeds move_to, which casts to i16, sees a negative, and returns an Argument Out of Range error — so the release outcome degrades to an error rather than a bogus cursor move. That is better than a silent wrong position, but it is still not the intended behaviour, and debug builds panic outright.

Why this reads as an oversight rather than a decision

The analogous case is guarded two impls away in the same file. MoveToNextLine and MoveToPreviousLine both test their count before calling into the sys layer, while MoveUp, MoveDown, MoveLeft and MoveRight call theirs unguarded. The author was thinking about the edge in one place and not the neighbouring one.

Severity, honestly bounded

Not memory-unsafe: u16 subtraction panics or wraps, it does not read out of bounds. The trigger is a caller passing a count larger than the current position, which is an ordinary thing for application code to do near the top or left edge of the screen. No untrusted input is involved.

Suggested fix

saturating_sub matches what the crate already chose for the coordinate parsers in src/event/sys/unix/parse.rs, and clamping at the edge is the natural terminal behaviour:

    move_to(column, row.saturating_sub(count))?;   // move_up
    move_to(column.saturating_sub(count), row)?;   // move_left

move_down and move_right add rather than subtract, so they are not affected by this, though a matching saturating_add would be consistent.

Scope note — the parser half of this report is already fixed

This finding originally covered eight bare - 1 subtractions in four coordinate parsers in src/event/sys/unix/parse.rs. Checked against the default branch before filing: those are all fixed — the parsers now use saturating_sub(1), parse_csi_normal_mouse folds the protocol offset into a single saturating_sub(33), and there is a regression test (test_parse_csi_normal_mouse_zero_coords_clamp) naming the old behaviour. Nothing is being re-reported there; this issue is narrowed to the two Windows sites, which are unchanged.

Provenance

Found by a line-by-line read of crossterm 0.28.1 during a cargo vet supply-chain certification, then re-checked against the default branch immediately before filing. Not observed on a live legacy Windows console — the underflow is reasoned from the source, and the release-path behaviour is derived from move_to's i16 cast rather than measured.

Activity

0

No activity yet.