Clamp savedY to valid bounds on terminal resize#467
Clamp savedY to valid bounds on terminal resize#467buzzboxmedia wants to merge 1 commit intomigueldeicaza:mainfrom
Conversation
Buffer.resize() clamps savedX but not savedY, allowing savedY to exceed the new row count. Terminal.resizeBuffers() can also produce a negative savedY when recomputing the offset after resize. Both cases cause an abort() in the Buffer.y setter on DEBUG builds when cmdRestoreCursor assigns buffer.y = buffer.savedY. Fix: clamp savedY to [0, newRows-1] in Buffer.resize() (matching the existing savedX clamp) and clamp to >= 0 in Terminal.resizeBuffers(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Points to buzzboxmedia/SwiftTerm fix/savedY-clamp branch which fixes an abort() crash when restoring cursor position after terminal resize. Upstream PR: migueldeicaza/SwiftTerm#467 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Mhm, I think that this has shown that there might be a bit of a misuses with yDisp, I think this might need a deeper fix. |
|
I think that this called into question a much bigger problem, which is that currently the code mixes savedY as either a viewport row (0..<rows) and sometimes an absolute buffer row (yBase-based) - and with these differences what you found was a step in the right direction, but not complete. I compared this to xterm.js which was the original inspiration, and they do not have that discrepancy - either it got fixed there, or I introduced the bug. This patch: https://gist.github.com/migueldeicaza/fb7470de4b4c9501c7eb660bc56a3ede Now treats savedY as absolute (yBase-baesd). Also, it now converts absolute savedY back to viewport row (savedY - yBase) and then clamps via restrictCursor(). While auditing I found another difference, softReset now also resets the saved cursor position Would you mind taking this for a spin? |
ways in the codebase, either a viewport relative value, or a buffer relative value (0..<rows vs yBase offset). I compared this to xterm.js which was the original code for this, and they do not have that discrepancy - either it got fixed there, or I introduced the bug. https://gist.github.com/migueldeicaza/fb7470de4b4c9501c7eb660bc56a3ede Now treats savedY as absolute (yBase-baesd). Also, it now converts absolute savedY back to viewport row (savedY - yBase) and then clamps via restrictCursor(). This should be a more comprehensive fix than the one reported in #467 While auditing I found another difference, softReset now also resets the saved cursor position
|
I created a branch |
Summary
Buffer.resize()clampssavedXtonewCols - 1but does not clampsavedY, allowing it to exceed the new row count after resizeTerminal.resizeBuffers()can produce a negativesavedYwhen recomputing the offset after resize (normalBuffer.y + dywheredywas captured before resize)abort()in theBuffer.ysetter on DEBUG builds whencmdRestoreCursorassignsbuffer.y = buffer.savedYFix
savedYto[0, newRows - 1]inBuffer.resize(), matching the existingsavedXclampsavedYto>= 0inTerminal.resizeBuffers()after recomputing the offsetTest plan
🤖 Generated with Claude Code