From 7e16680a26dbd160eef634e76898ef4c9d4811da Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 04:32:24 +0000 Subject: [PATCH] Terminal: draw control characters as ^X; bound GUI config values - C0 controls and DEL were passed to ncurses, which acted on them: a carriage return moved to column 0 and the line padding then erased the text, so every line of a CRLF file drew blank, and a backspace ate the previous character. They are now drawn in caret notation (^M, ^H, ^?) and counted as two cells. C1 controls (and stray bytes 0x80-0x9F, which decode to them; U+009B is CSI) are drawn as '?'. - The width rule lives in TermWidth.h and is shared by the renderer's drawing loop, its cursor placement and the command layer's horizontal scrolling and mouse mapping, which also disagreed on NUL and invalid bytes. - GUI config: columns/rows must be 1..1000 and the font size a finite 4..200 (TOML 'size = inf' and huge values were accepted, leading to UB in float-to-int conversion and int overflow downstream). Verified in tmux: a CRLF file now shows 'first line^M' where the baseline showed a blank line. Audit findings B9 (and the CR/BS case from the follow-up review). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015rcVQiiWyTzoNrsvLSaAfB --- Command.cc | 18 +++++++------- GUIConfig.cc | 30 +++++++++++++++++----- TermWidth.h | 40 ++++++++++++++++++++++++++++++ TerminalRenderer.cc | 44 +++++++++++++++++---------------- tests/test_audit_regressions.cc | 13 ++++++++++ 5 files changed, 109 insertions(+), 36 deletions(-) create mode 100644 TermWidth.h diff --git a/Command.cc b/Command.cc index fc671ff..2c3b57b 100644 --- a/Command.cc +++ b/Command.cc @@ -12,6 +12,7 @@ #include "Command.h" #include "RegexGuard.h" +#include "TermWidth.h" #include "syntax/HighlighterRegistry.h" #include "syntax/NullHighlighter.h" #include "Editor.h" @@ -134,17 +135,16 @@ measure_char(std::string_view line, std::size_t i, std::size_t rx, std::size_t t wchar_t wch = 0; const auto rc = std::mbrtowc(&wch, line.data() + i, line.size() - i, &state); if (rc == static_cast(-1) || rc == static_cast(-2) || rc == 0) { - len = 1; - width = (rc == 0) ? 0 : 1; - return; - } - len = rc; - if (wch == L'\t') { - width = tabw - (rx % tabw); + // Invalid byte or NUL: the renderer draws the byte value itself. + wch = static_cast(line[i]); + len = 1; } else { - const int w = wcwidth(wch); - width = (w < 0) ? 1 : static_cast(w); + len = rc; } + if (wch == L'\t') + width = tabw - (rx % tabw); + else + width = static_cast(kte::CellWidth(wch)); } diff --git a/GUIConfig.cc b/GUIConfig.cc index daf1f54..dbb1ebb 100644 --- a/GUIConfig.cc +++ b/GUIConfig.cc @@ -1,3 +1,4 @@ +#include #include #include #include @@ -20,6 +21,7 @@ #include "ext/tomlplusplus/toml.hpp" + #if defined(__clang__) # pragma clang diagnostic pop #elif defined(__GNUC__) @@ -27,6 +29,22 @@ #endif +// Reject values that would overflow later arithmetic (columns * font size) +// or that are not usable: "size = inf" is valid TOML. +static bool +valid_dimension(long long v) +{ + return v > 0 && v <= 1000; +} + + +static bool +valid_font_size(double v) +{ + return std::isfinite(v) && v >= 4.0 && v <= 200.0; +} + + static void trim(std::string &s) { @@ -91,10 +109,10 @@ GUIConfig::LoadFromTOML(const std::string &path) if (auto v = (*win)["fullscreen"].value()) fullscreen = *v; if (auto v = (*win)["columns"].value()) { - if (*v > 0) columns = static_cast(*v); + if (valid_dimension(*v)) columns = static_cast(*v); } if (auto v = (*win)["rows"].value()) { - if (*v > 0) rows = static_cast(*v); + if (valid_dimension(*v)) rows = static_cast(*v); } } @@ -105,7 +123,7 @@ GUIConfig::LoadFromTOML(const std::string &path) if (auto v = (*sec)["name"].value()) font = *v; if (auto v = (*sec)["size"].value()) { - if (*v > 0.0) font_size = static_cast(*v); + if (valid_font_size(*v)) font_size = static_cast(*v); } if (auto v = (*sec)["code"].value()) { code_font = *v; @@ -192,21 +210,21 @@ GUIConfig::LoadFromINI(const std::string &path) try { v = std::stoi(val); } catch (...) {} - if (v > 0) + if (valid_dimension(v)) columns = v; } else if (key == "rows") { int v = rows; try { v = std::stoi(val); } catch (...) {} - if (v > 0) + if (valid_dimension(v)) rows = v; } else if (key == "font_size" || key == "fontsize") { float v = font_size; try { v = std::stof(val); } catch (...) {} - if (v > 0.0f) { + if (valid_font_size(v)) { font_size = v; } } else if (key == "font") { diff --git a/TermWidth.h b/TermWidth.h new file mode 100644 index 0000000..674e354 --- /dev/null +++ b/TermWidth.h @@ -0,0 +1,40 @@ +// TermWidth.h - terminal cell width of a decoded character +// +// Shared by TerminalRenderer (drawing and cursor placement) and the command +// layer (horizontal scrolling, mouse column mapping) so they always agree. +// Tabs are handled by the callers, which know the current column. +#pragma once + +#include + +namespace kte { +// C0 controls and DEL are drawn as caret notation ("^M", "^?"): two cells. +// ncurses would otherwise act on them (carriage return, backspace) and +// corrupt the line. +inline bool +IsCaretControl(wchar_t wch) +{ + return (wch >= 0 && wch < 0x20) || wch == 0x7f; +} + + +// C1 controls (U+0080..U+009F, also what a stray byte in that range decodes +// to) are drawn as '?': sent raw, U+009B is a terminal CSI. +inline bool +IsC1Control(wchar_t wch) +{ + return wch >= 0x80 && wch <= 0x9f; +} + + +inline int +CellWidth(wchar_t wch) +{ + if (IsCaretControl(wch)) + return 2; + if (IsC1Control(wch)) + return 1; + const int w = wcwidth(wch); + return w < 0 ? 1 : w; +} +} // namespace kte diff --git a/TerminalRenderer.cc b/TerminalRenderer.cc index ebca618..82b7461 100644 --- a/TerminalRenderer.cc +++ b/TerminalRenderer.cc @@ -11,6 +11,7 @@ #include "TerminalRenderer.h" #include "RegexGuard.h" +#include "TermWidth.h" #include "Buffer.h" #include "Editor.h" #include "Highlight.h" @@ -177,10 +178,7 @@ TerminalRenderer::Draw(Editor &ed) const std::size_t next_tab = tab_width - (rc % tab_width); rc += next_tab; } else { - int w = wcwidth(wch); - if (w < 0) - w = 1; - rc += static_cast(w); + rc += static_cast(kte::CellWidth(wch)); } si += static_cast(wch_len); } @@ -315,10 +313,8 @@ TerminalRenderer::Draw(Editor &ed) src_i += wch_len; continue; } else { - // normal char - disp_w = wcwidth(wch); - if (disp_w < 0) - disp_w = 1; // non-printable or similar + // normal char (controls are drawn as ^X or '?') + disp_w = kte::CellWidth(wch); if (render_col < coloffs) { render_col += disp_w; @@ -366,7 +362,12 @@ TerminalRenderer::Draw(Editor &ed) } attrset(a); - if (from_src) { + if (from_src && kte::IsCaretControl(wch)) { + addch('^'); + addch(wch == 0x7f ? '?' : static_cast(wch + 0x40)); + } else if (from_src && kte::IsC1Control(wch)) { + addch('?'); + } else if (from_src) { cchar_t cch; wchar_t warr[2] = {wch, L'\0'}; setcchar(&cch, warr, 0, 0, nullptr); @@ -405,21 +406,22 @@ TerminalRenderer::Draw(Editor &ed) &wch, &line_for_cursor[src_i_cur], line_for_cursor.size() - src_i_cur, &state); + // Same decoding and widths as the drawing loop above. + std::size_t len = res; if (res == (size_t) -1 || res == (size_t) -2) { - render_col_cur += 1; - src_i_cur += 1; + wch = static_cast(line_for_cursor[src_i_cur]); + len = 1; } else if (res == 0) { - src_i_cur += 1; - } else { - if (wch == L'\t') { - std::size_t next_tab = tabw - (render_col_cur % tabw); - render_col_cur += next_tab; - } else { - int dw = wcwidth(wch); - render_col_cur += (dw < 0) ? 1 : dw; - } - src_i_cur += res; + wch = L'\0'; + len = 1; } + if (wch == L'\t') { + std::size_t next_tab = tabw - (render_col_cur % tabw); + render_col_cur += next_tab; + } else { + render_col_cur += static_cast(kte::CellWidth(wch)); + } + src_i_cur += len; } rx_recomputed = render_col_cur; } diff --git a/tests/test_audit_regressions.cc b/tests/test_audit_regressions.cc index fc0e3d5..c106b38 100644 --- a/tests/test_audit_regressions.cc +++ b/tests/test_audit_regressions.cc @@ -818,3 +818,16 @@ TEST(Audit_SaveAs_ExistingFile_AsksFirst) ASSERT_TRUE(h.Exec(CommandId::Newline)); ASSERT_EQ(slurp(d.path / "other.txt"), std::string("precious\n")); } + + +// Control characters are drawn as ^X (two cells); horizontal scrolling must +// count them the same way. +TEST(Audit_HorizontalScroll_ControlCharsAreTwoCells) +{ + TestHarness h; + Buffer &b = h.Buf(); + b.insert_text(0, 0, std::string(50, '\r')); + b.SetCursor(0, 0); + ASSERT_TRUE(h.Exec(CommandId::MoveEnd)); + ASSERT_EQ(b.Coloffs(), (std::size_t) (100 - 80 + 1)); +}