Skip to content

fix(table): prevent panic when YOffset is set to a negative value - #727

Open
bunlongheng wants to merge 1 commit into
charmbracelet:mainfrom
bunlongheng:fix-table-negative-yoffset-panic
Open

fix(table): prevent panic when YOffset is set to a negative value#727
bunlongheng wants to merge 1 commit into
charmbracelet:mainfrom
bunlongheng:fix-table-negative-yoffset-panic

Conversation

@bunlongheng

Copy link
Copy Markdown

What

Table.YOffset stores whatever integer it is given without validation. When the
value is negative, Render() panics with an out-of-range slice index.

The crash happens in resizer.visibleRowIndexes (table/resizing.go). The scroll
window is seeded from the offset:

firstVisibleRowIndex = r.yOffset
lastVisibleRowIndex = firstVisibleRowIndex - 1
for available > 0 && lastVisibleRowIndex < lastIndex {
    row := r.rowHeights[lastVisibleRowIndex+1+btoi(hasHeaders)] + btoi(r.borderRow)
    ...

With a negative yOffset, lastVisibleRowIndex+1+btoi(hasHeaders) is negative,
so r.rowHeights[...] indexes out of range and panics. Large positive offsets
are already handled gracefully (the loop bails and the function returns early),
but the lower bound was never guarded.

Why it matters

YOffset is the public scroll API for tables. In an interactive TUI the offset
is commonly driven directly by user key events. A naive offset-- on a scroll-up
without clamping at 0 produces a negative offset, and the next render takes down
the whole program. A single out-of-range value turns a rendering call into a hard
crash (denial of service).

Reproduction:

tbl := table.New().
    Headers("A", "B").
    Row("1", "2").
    Row("3", "4").
    Height(10).
    YOffset(-3)
_ = tbl.Render() // panic: runtime error: index out of range [-2]

Fix

Clamp negative offsets to 0 in the YOffset setter, mirroring the existing
clamping in the TabWidth setter. A negative scroll offset has no meaning (you
cannot scroll above the first row), so 0 is the correct floor. This keeps the
invalid state from ever reaching the resizer.

Test

Before: the reproduction above panics with index out of range [-2].
After: Render() returns normally. The full ./table/ test suite passes,
including the existing YOffset tests (which use positive offsets and are
unaffected).

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