devs/cproof
1
2
Fork 1
Code Issues 43 Pull Requests 6 Actions Packages Projects 1 Releases Wiki Activity

Harden pad reclaim: trigger on dead space instead of absolute pad height #134

New Issue
Open
opened 2026-06-08 09:35:17 +00:00 by jabber.developer2 · 0 comments
jabber.developer2 commented 2026-06-08 09:35:17 +00:00
Collaborator
Copy Link

Context

fix/history-scroll-pad-redraw-storm fixes the history-scroll regression by raising PAD_THRESHOLD from 3000 to 12000 — above the buffer line cap (PAD_SIZE - PAD_SIZE/10 = 9000) so the reclaim win_redraw() never fires during normal scrolling. That is the correct quick fix and restores parity with the pre-merge behaviour, but the trigger is still an absolute pad-height threshold, which leaves one residual edge.

Residual vulnerability (variant A, shipped)

_win_ensure_pad_capacity() re-enters win_redraw() whenever cur_height >= PAD_THRESHOLD. After a reclaim redraw the cursor resets to buffer->lines, so a per-message redraw storm reappears whenever the live buffer renders ≥ PAD_THRESHOLD lines. That is reachable with a single message ≥ ~12000 rendered lines (a huge paste, a narrow terminal, or a crafted long incoming message — a mild DoS vector), because trimming cannot drop a single oversized entry below the cap. This edge already existed pre-merge at ~10000 via the buffer.c overflow branch; raising the threshold mitigates but does not remove it. The constant is also a magic number decoupled from PAD_SIZE / MESSAGES_TO_RETRIEVE.

Proposed change (variant B, dead-space trigger)

Trigger the reclaim by how much dead space the pad holds (getcury - buffer->lines), not by absolute height. Only the append path accumulates dead space (trimmed-oldest leaves a gap); the scroll/prepend path keeps getcury == buffer->lines after each redraw, so the trigger is structurally inert during scrolling regardless of buffer size — no per-message storm even on a giant single message (after a redraw dead == 0), and oversized messages can actually be displayed (the pad grows to fit) instead of being clipped at the cap.

Sketch (same 4 edit sites as A, ~2 extra lines; buffer->lines is already reachable in window.c):

static const int PAD_DEAD_SPACE_LIMIT = 2000;

if (lines_needed >= cur_height - 1) {
    int dead = window ? lines_needed - window->layout->buffer->lines : 0;
    if (window && dead > PAD_DEAD_SPACE_LIMIT && !_in_redraw) {
        win_redraw(window);
    } else {
        wresize(win, lines_needed + 100, cur_width);
    }
}

Why this is a separate, tested follow-up (not folded into the quick fix)

B introduces a reset trigger that did not exist before the merge. It is provably storm-free in analysis, but it has untested runtime dynamics that warrant real validation:

  • dead spikes by the trimmed amount when trimming fires mid-batch during scroll-up at the cap; DELTA must exceed a typical batch so it does not cause a few extra redraws per page.
  • Behaviour on narrow terminals with long messages (large batch line counts).
  • Signed comparison / transient ordering: getcury advances before buffer_prepend updates buffer->lines; the _in_redraw guard must still wrap win_redraw().

Acceptance criteria

  • Scroll a >9000-line history to the top: smooth, lossless, no *page_start adjusted to 0 firing mid-history.
  • Paste / receive a single ≥15000-line message: no per-message redraw storm; the message is displayed (not clipped); UI stays responsive.
  • Long append-only session in one window (no scrolling/switching): pad memory stays bounded (~buffer->lines + DELTA).
  • Narrow terminal (e.g. 40 cols) with long messages: at most a couple of redraws per fetch, never per message.

Shared hardening (applies to both A and B)

  • Check the wresize() return value; large pads can fail to allocate and silently drop content.
  • win_redraw(wins_get_current()) in buffer.c redraws the current window, not necessarily the one whose buffer overflowed (latent, pre-existing; mostly dead with the dynamic pad).
## Context `fix/history-scroll-pad-redraw-storm` fixes the history-scroll regression by raising `PAD_THRESHOLD` from 3000 to 12000 — above the buffer line cap (`PAD_SIZE - PAD_SIZE/10 = 9000`) so the reclaim `win_redraw()` never fires during normal scrolling. That is the correct quick fix and restores parity with the pre-merge behaviour, but the trigger is still an **absolute pad-height threshold**, which leaves one residual edge. ## Residual vulnerability (variant A, shipped) `_win_ensure_pad_capacity()` re-enters `win_redraw()` whenever `cur_height >= PAD_THRESHOLD`. After a reclaim redraw the cursor resets to `buffer->lines`, so a per-message redraw storm reappears whenever the **live** buffer renders ≥ `PAD_THRESHOLD` lines. That is reachable with a single message ≥ ~12000 rendered lines (a huge paste, a narrow terminal, or a crafted long incoming message — a mild DoS vector), because trimming cannot drop a single oversized entry below the cap. This edge already existed pre-merge at ~10000 via the `buffer.c` overflow branch; raising the threshold mitigates but does not remove it. The constant is also a magic number decoupled from `PAD_SIZE` / `MESSAGES_TO_RETRIEVE`. ## Proposed change (variant B, dead-space trigger) Trigger the reclaim by how much **dead** space the pad holds (`getcury - buffer->lines`), not by absolute height. Only the append path accumulates dead space (trimmed-oldest leaves a gap); the scroll/prepend path keeps `getcury == buffer->lines` after each redraw, so the trigger is structurally inert during scrolling **regardless of buffer size** — no per-message storm even on a giant single message (after a redraw `dead == 0`), and oversized messages can actually be displayed (the pad grows to fit) instead of being clipped at the cap. Sketch (same 4 edit sites as A, ~2 extra lines; `buffer->lines` is already reachable in `window.c`): ```c static const int PAD_DEAD_SPACE_LIMIT = 2000; if (lines_needed >= cur_height - 1) { int dead = window ? lines_needed - window->layout->buffer->lines : 0; if (window && dead > PAD_DEAD_SPACE_LIMIT && !_in_redraw) { win_redraw(window); } else { wresize(win, lines_needed + 100, cur_width); } } ``` ## Why this is a separate, tested follow-up (not folded into the quick fix) B introduces a reset trigger that did not exist before the merge. It is provably storm-free in analysis, but it has untested runtime dynamics that warrant real validation: - `dead` spikes by the trimmed amount when trimming fires mid-batch during scroll-up at the cap; `DELTA` must exceed a typical batch so it does not cause a few extra redraws per page. - Behaviour on narrow terminals with long messages (large batch line counts). - Signed comparison / transient ordering: `getcury` advances before `buffer_prepend` updates `buffer->lines`; the `_in_redraw` guard must still wrap `win_redraw()`. ## Acceptance criteria - Scroll a >9000-line history to the top: smooth, lossless, no `*page_start adjusted to 0` firing mid-history. - Paste / receive a single ≥15000-line message: no per-message redraw storm; the message is displayed (not clipped); UI stays responsive. - Long append-only session in one window (no scrolling/switching): pad memory stays bounded (~`buffer->lines + DELTA`). - Narrow terminal (e.g. 40 cols) with long messages: at most a couple of redraws per fetch, never per message. ## Shared hardening (applies to both A and B) - Check the `wresize()` return value; large pads can fail to allocate and silently drop content. - `win_redraw(wins_get_current())` in `buffer.c` redraws the *current* window, not necessarily the one whose buffer overflowed (latent, pre-existing; mostly dead with the dynamic pad).
jabber.developer added this to the Plans (2025-2026) project 2026-06-24 16:52:11 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
Compat/Breaking
Breaking change that won't be backward compatible
Kind/Bug
Something is not working
Kind/Documentation
Documentation changes
Kind/Enhancement
Improve existing functionality
Kind/Feature
New functionality
Kind/Security
This is security issue
Kind/Testing
Issue or pull request related to testing
Priority
Critical
1
The priority is critical
Priority
High
2
The priority is high
Priority
Low
4
The priority is low
Priority
Medium
3
The priority is medium
Reviewed
Confirmed
1
Issue has been confirmed
Reviewed
Duplicate
2
This issue or pull request already exists
Reviewed
Invalid
3
Invalid issue
Reviewed
Won't Fix
3
This issue won't be fixed
Status
Abandoned
3
Somebody has started to work on this but abandoned work
Status
Blocked
1
Something is blocking this issue or pull request
Status
Need More Info
2
Feedback is required to reproduce issue or to continue work
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project Plans (2025-2026)
Assignees
Clear assignees
admin jabber.developer jabber.developer2
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: devs/cproof#134
No description provided.
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?