Skip to content

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

Description

@3x3xX3N0N

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions