diff options
| author | Berke Güzel <wenekar1@gmail.com> | 2026-10-15 11:31:50 +0300 |
|---|---|---|
| committer | Berke Güzel <wenekar1@gmail.com> | 2026-10-15 11:31:50 +0300 |
| commit | acd338ea33de36d223cc6d301712eed4a9501e6c (patch) | |
| tree | d93c46a97e2ed4e8a86e3402de5f847cdc144ec8 | |
| parent | f97c3a585e2efc1d0a4bc10e6ac04b96ee6428ea (diff) | |
Fix snapshot storage and allocation failure correctness
Account for cluster records discarded by snapshot scrolling, propagate snapshot allocation failures with an empty retryable view, and allocate resized screens before mutating visible state. Add bounded-memory and allocation-failure regressions, and update the saved diagnostic and audit report.
| -rw-r--r-- | docs/correctness-audit.md | 20 | ||||
| -rw-r--r-- | src/screen.c | 18 | ||||
| -rw-r--r-- | src/snapshot.c | 59 | ||||
| -rw-r--r-- | tests/correctness_probe.c | 13 | ||||
| -rw-r--r-- | tests/meson.build | 12 | ||||
| -rw-r--r-- | tests/resize_failure_test.c | 51 | ||||
| -rw-r--r-- | tests/snapshot_failure_test.c | 91 | ||||
| -rw-r--r-- | tests/snapshot_scroll_arena_test.c | 29 |
8 files changed, 271 insertions, 22 deletions
diff --git a/docs/correctness-audit.md b/docs/correctness-audit.md index bb85e24..4c9a14c 100644 --- a/docs/correctness-audit.md +++ b/docs/correctness-audit.md @@ -24,7 +24,7 @@ ASAN_OPTIONS=detect_leaks=0 build-audit/correctness-probe `tests/correctness_probe.c` prints the observed behavior. It intentionally includes `src/snapshot.c` to inspect its private arena counters and links against the static library. It is a diagnostic, outside the default suite; -exit status zero alone does not mean the findings have been fixed. +it returns a nonzero status if any of the three findings still reproduces. Leak detection was disabled because LeakSanitizer could not run in the audit sandbox. Address and undefined-behavior checks remained enabled. To repeat @@ -91,3 +91,21 @@ remain on row 3, and history should remain empty. - Inject allocation failures and verify each API's documented failure state. - Compare incremental snapshots with fresh captures, including grapheme text. - Assert bounded storage during prolonged scrolling with a fixed viewport. + +## Fixes + +- Snapshot scroll accounting now counts discarded cluster records as dead, + allowing compaction to reclaim them. +- Snapshot allocation failures now report false, empty the snapshot, and + force a complete capture on retry. +- Height shrink allocates the replacement screen before moving rows or + changing the cursor, preserving state if allocation fails. + +Dedicated regression tests cover sustained scrolling, snapshot allocation +failure and recovery, and failed height shrink on both normal and alternate +screens. The output above records the original failures for reference. + +Validation after the fixes: all 43 unit test entries passed in an +unsanitized debug build and all 43 passed with address and undefined-behavior +sanitizers. The standalone probe also passed in both builds. Leak detection +remained disabled in the sandbox. diff --git a/src/screen.c b/src/screen.c index bd65564..1712faf 100644 --- a/src/screen.c +++ b/src/screen.c @@ -1236,6 +1236,14 @@ static bool term_resize_internal(struct term *t, int cols, int rows) int old_cols = active_scr->cols; int old_rows = active_scr->rows; + struct screen new_scr; + if (!resize_target(t, &new_scr, cols, rows, active_scr)) { + term_gstore_unpin(t->gstore); + return false; + } + + /* Allocate the replacement before scrolling rows into history. A + * failed height shrink must leave the active screen and cursor alone. */ if (rows < old_rows && t->cursor_y >= rows) { int shift = t->cursor_y - rows + 1; for (int i = 0; i < shift; i++) @@ -1243,12 +1251,10 @@ static bool term_resize_internal(struct term *t, int cols, int rows) t->cursor_y -= shift; t->saved.y = t->saved.y >= shift ? t->saved.y - shift : 0; } - - struct screen new_scr; - if (!resize_target(t, &new_scr, cols, rows, active_scr)) { - term_gstore_unpin(t->gstore); - return false; - } + /* Scrolling can advance the screen's history mapping when there is + * no history buffer, so capture its final value after the scroll. */ + new_scr.top_id = active_scr->top_id; + new_scr.top_seg = active_scr->top_seg; int pull = 0; if (rows > old_rows && !t->alternate) diff --git a/src/snapshot.c b/src/snapshot.c index 29ed98c..6578c9a 100644 --- a/src/snapshot.c +++ b/src/snapshot.c @@ -73,6 +73,20 @@ static bool reserve(struct term_snapshot *s, int cols, int rows) return true; } +/* The public contract makes every failed update an empty, retryable snapshot. */ +static void invalidate(struct term_snapshot *s) +{ + s->view = (struct term_view){ .palette = s->palette }; + s->source = 0; + s->arena_len = 0; + s->arena_dead = 0; + if (s->row_cap > 0) { + memset(s->keys, 0, (size_t)s->row_cap * sizeof(*s->keys)); + memset(s->row_clusters, 0, (size_t)s->row_cap * sizeof(*s->row_clusters)); + memset(s->selection, 0, (size_t)s->row_cap * sizeof(*s->selection)); + } +} + /* Attribute contributions of the low and high byte of cell flags. */ static uint16_t lo_attrs[256], hi_attrs[256]; static uint8_t hi_underline[256]; @@ -100,7 +114,8 @@ void term_snapshot_tables_init(void) } } -static uint32_t arena_add(struct term_snapshot *s, const struct gstore *g, uint32_t handle) +static uint32_t arena_add(struct term_snapshot *s, const struct gstore *g, uint32_t handle, + bool *ok) { uint32_t cps[GRAPHEME_MAX_LEN]; int n = term_gstore_expand(g, handle, cps, GRAPHEME_MAX_LEN); @@ -110,8 +125,10 @@ static uint32_t arena_add(struct term_snapshot *s, const struct gstore *g, uint3 uint32_t cap = term_grow_cap32(s->arena_cap ? s->arena_cap * 2 : 256, s->arena_len + (uint32_t)n + 1); uint32_t *a = realloc(s->arena, cap * sizeof(*a)); - if (!a) - return cps[0]; + if (!a) { + *ok = false; + return 0; + } s->arena = a; s->arena_cap = cap; } @@ -125,7 +142,7 @@ static uint32_t arena_add(struct term_snapshot *s, const struct gstore *g, uint3 /* Convert one stored row; returns the number of arena words it added. */ static uint32_t convert_row(struct term_snapshot *s, const struct gstore *g, struct term_cell *dst, - const struct cell *src, int cols) + const struct cell *src, int cols, bool *ok) { uint32_t before = s->arena_len; for (int x = 0; x < cols; x++) { @@ -138,8 +155,11 @@ static uint32_t convert_row(struct term_snapshot *s, const struct gstore *g, uint32_t text = c->c; if (TERM_EXPECT(text < 0x20, 0)) text = text ? ' ' : 0; - else if (TERM_EXPECT(grapheme_is_handle(text), 0)) - text = arena_add(s, g, text); + else if (TERM_EXPECT(grapheme_is_handle(text), 0)) { + text = arena_add(s, g, text, ok); + if (!*ok) + return 0; + } dst[x] = (struct term_cell){ .text = text, .fg = c->fg, @@ -158,8 +178,7 @@ static bool term_snapshot_update_inner(struct term_snapshot *s, struct term *t) return false; int cols = t->cols, rows = t->rows; if (!reserve(s, cols, rows)) { - s->view.size = (struct term_vector2i){ 0 }; - s->source = 0; + invalidate(s); return false; } @@ -175,13 +194,17 @@ static bool term_snapshot_update_inner(struct term_snapshot *s, struct term *t) bool changed = all; term_hist_prepare_rows(&t->hist, cols); - if (!term_hist_view_sync(&t->view, &t->hist, cols)) + if (!term_hist_view_sync(&t->view, &t->hist, cols)) { + invalidate(s); return false; + } term_hist_view_refine(&t->view, &t->hist, cols, rows); uint64_t stack_keys[256]; uint64_t *new_keys = rows <= 256 ? stack_keys : malloc((size_t)rows * sizeof(uint64_t)); - if (!new_keys) + if (!new_keys) { + invalidate(s); return false; + } for (int y = 0; y < rows; y++) new_keys[y] = term_screen_view_row_key(t, y); @@ -230,6 +253,8 @@ static bool term_snapshot_update_inner(struct term_snapshot *s, struct term *t) if (scrolled > 0) { int k = scrolled; + for (int y = 0; y < k; y++) + s->arena_dead += s->row_clusters[y]; memmove(s->cells, s->cells + (size_t)k * cols, (size_t)(rows - k) * cols * sizeof(struct term_cell)); memmove(s->row_clusters, s->row_clusters + k, @@ -247,7 +272,15 @@ static bool term_snapshot_update_inner(struct term_snapshot *s, struct term *t) if (!all) s->arena_dead += s->row_clusters[y]; const struct cell *src = term_screen_view_row(t, y); - s->row_clusters[y] = convert_row(s, t->gstore, s->cells + (size_t)y * cols, src, cols); + bool converted = true; + s->row_clusters[y] = convert_row(s, t->gstore, s->cells + (size_t)y * cols, + src, cols, &converted); + if (!converted) { + if (new_keys != stack_keys) + free(new_keys); + invalidate(s); + return false; + } s->keys[y] = key; changed = true; } @@ -256,8 +289,10 @@ static bool term_snapshot_update_inner(struct term_snapshot *s, struct term *t) int stack[2][256]; int *c0 = rows <= 256 ? stack[0] : malloc((size_t)rows * 2 * sizeof(int)); - if (!c0) + if (!c0) { + invalidate(s); return false; + } int *c1 = rows <= 256 ? stack[1] : c0 + rows; term_selection_view_spans(t, c0, c1); for (int y = 0; y < rows; y++) { diff --git a/tests/correctness_probe.c b/tests/correctness_probe.c index 0b55211..a7c15df 100644 --- a/tests/correctness_probe.c +++ b/tests/correctness_probe.c @@ -26,6 +26,7 @@ void *__wrap_realloc(void *ptr, size_t size) int main(void) { + bool failed = false; struct term_options options = { .size = { 8, 3 }, .scrollback = 5 }; struct term *t = term_new(&options); struct term_snapshot *s = term_snapshot_new(); @@ -35,11 +36,14 @@ int main(void) fail_realloc = 1; bool ok = term_snapshot_update(s, t); uint32_t cps[16]; - int n = term_snapshot_cluster(s, term_snapshot_row(s, 0)[0].text, cps, 16); - printf("snapshot arena allocation failure: update=%d cluster_length=%d (expected false, or length 2)\n", ok, n); + const struct term_cell *row = term_snapshot_row(s, 0); + int n = row ? term_snapshot_cluster(s, row[0].text, cps, 16) : 0; + printf("snapshot arena allocation failure: update=%d cluster_length=%d (expected false and empty)\n", ok, n); + failed |= ok || term_snapshot_view(s)->size.x != 0 || term_snapshot_view(s)->size.y != 0; assert(term_snapshot_update(s, t)); n = term_snapshot_cluster(s, term_snapshot_row(s, 0)[0].text, cps, 16); printf("retry after allocation recovers: cluster_length=%d (expected 2)\n", n); + failed |= n != 2 || cps[0] != 'e' || cps[1] != 0x301; term_snapshot_free(s); term_free(t); @@ -55,6 +59,7 @@ int main(void) printf("after %d lines: arena_words=%u dead_words=%u history_rows=%d\n", i, s->arena_len, s->arena_dead, term_history_rows(t)); } + failed |= s->arena_len >= 8192 || term_history_rows(t) != 5; term_snapshot_free(s); term_free(t); @@ -74,7 +79,9 @@ int main(void) printf("after failed height shrink: resize=%d height=%d top=%c cursor_y=%d history=%d\n", ok, term_size(t).y, (char)term_snapshot_row(s, 0)[0].text, term_cursor(t).pos.y, term_history_rows(t)); + failed |= ok || term_size(t).y != 4 || term_snapshot_row(s, 0)[0].text != 'A' || + term_cursor(t).pos.y != 3 || term_history_rows(t) != 0; term_snapshot_free(s); term_free(t); - return 0; + return failed ? 1 : 0; } diff --git a/tests/meson.build b/tests/meson.build index 4cf559c..69f3c49 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -24,6 +24,7 @@ term_tests = [ ['gstore-test', files('gstore_test.c'), [], []], ['hist-test', files('hist_test.c'), [], []], ['history-text-test', files('history_text_test.c'), [thread_dep], []], + ['snapshot-scroll-arena-test', files('snapshot_scroll_arena_test.c'), [], []], ['view-test', files('view_test.c'), [], []], ['resize-property-test', files('resize_property_test.c'), [], []], ['term-resize-failure-test', files('resize_failure_test.c'), [dl_dep], ['extlib']], @@ -105,6 +106,17 @@ test('term-header-test', bash, args: [files('header_test.sh'), meson.project_source_root()], suite: 'unit', ) +if cc.has_link_argument('-Wl,--wrap=realloc') and cc.has_link_argument('-Wl,--wrap=malloc') + snapshot_failure_test = executable('snapshot-failure-test', 'snapshot_failure_test.c', + c_args: test_c_args, + link_args: ['-Wl,--wrap=realloc', '-Wl,--wrap=malloc'], + dependencies: [term_internal_dep], + ) + test('snapshot-failure-test', snapshot_failure_test, + env: test_env, + suite: 'unit', + ) +endif test('term-cross-test', bash, args: [files('cross_test.sh'), meson.project_source_root()], suite: 'portability', diff --git a/tests/resize_failure_test.c b/tests/resize_failure_test.c index 5d7e63c..fbfa38d 100644 --- a/tests/resize_failure_test.c +++ b/tests/resize_failure_test.c @@ -146,6 +146,57 @@ int main(void) assert(tt_cols(term) == 10); term_free(term); + /* A failed same-width height shrink must not scroll rows into history or + * move the cursor. The same resize must succeed when allocation recovers. */ + term = tt_new(8, 4); + assert(term); + const char four_rows[] = "A\r\nB\r\nC\r\nD"; + tt_write(term, four_rows, sizeof four_rows - 1); + assert(tt_cursor_y(term) == 3); + assert(tt_history(term) == 0); + + fail_next_calloc = true; + resized = tt_resize(term, 8, 2); + assert(!fail_next_calloc); + assert(!resized); + assert(tt_cols(term) == 8 && tt_rows(term) == 4); + assert(tt_cell(term, 0, 0)->text == 'A'); + assert(tt_cell(term, 0, 1)->text == 'B'); + assert(tt_cell(term, 0, 2)->text == 'C'); + assert(tt_cell(term, 0, 3)->text == 'D'); + assert(tt_cursor_y(term) == 3); + assert(tt_history(term) == 0); + + resized = tt_resize(term, 8, 2); + assert(resized); + assert(tt_cols(term) == 8 && tt_rows(term) == 2); + assert(tt_cell(term, 0, 0)->text == 'C'); + assert(tt_cell(term, 0, 1)->text == 'D'); + assert(tt_cursor_y(term) == 1); + assert(tt_history(term) == 2); + term_free(term); + + /* The alternate screen uses the same allocation-before-scroll path. */ + term = tt_new(8, 4); + assert(term); + tt_write(term, "\033[?1049h", 8); + tt_write(term, four_rows, sizeof four_rows - 1); + assert(term_modes(term).alternate); + assert(tt_cursor_y(term) == 3); + fail_next_calloc = true; + resized = tt_resize(term, 8, 2); + assert(!fail_next_calloc); + assert(!resized); + assert(tt_rows(term) == 4 && tt_cursor_y(term) == 3); + assert(tt_cell(term, 0, 0)->text == 'A'); + assert(tt_cell(term, 0, 3)->text == 'D'); + resized = tt_resize(term, 8, 2); + assert(resized); + assert(tt_rows(term) == 2 && tt_cursor_y(term) == 1); + assert(tt_cell(term, 0, 0)->text == 'C'); + assert(tt_cell(term, 0, 1)->text == 'D'); + term_free(term); + puts("lazy screen allocation failures preserve observable terminal state"); return 0; #endif diff --git a/tests/snapshot_failure_test.c b/tests/snapshot_failure_test.c new file mode 100644 index 0000000..4133bfd --- /dev/null +++ b/tests/snapshot_failure_test.c @@ -0,0 +1,91 @@ +#include "term/term.h" + +#include <assert.h> +#include <stdbool.h> +#include <stddef.h> +#include <stdlib.h> + +static bool fail_next_realloc; +static int malloc_failure_countdown = -1; +static bool malloc_was_failed; +void *__real_malloc(size_t); +void *__wrap_malloc(size_t size) +{ + if (malloc_failure_countdown >= 0) { + if (malloc_failure_countdown-- == 0) { + malloc_failure_countdown = -1; + malloc_was_failed = true; + return NULL; + } + } + return __real_malloc(size); +} + +void *__real_realloc(void *, size_t); +void *__wrap_realloc(void *p, size_t size) +{ + if (fail_next_realloc) { + fail_next_realloc = false; + return NULL; + } + return __real_realloc(p, size); +} + +int main(void) +{ + struct term_options options = { .size = { 8, 3 }, .scrollback = 5 }; + struct term *term = term_new(&options); + struct term_snapshot *snapshot = term_snapshot_new(); + assert(term && snapshot && term_snapshot_update(snapshot, term)); + + const char accented[] = "e\xcc\x81"; + term_feed(term, accented, sizeof(accented) - 1, 0); + fail_next_realloc = true; + assert(!term_snapshot_update(snapshot, term)); + assert(!fail_next_realloc); + assert(term_snapshot_view(snapshot)->size.x == 0); + assert(term_snapshot_view(snapshot)->size.y == 0); + assert(term_snapshot_row(snapshot, 0) == NULL); + + assert(term_snapshot_update(snapshot, term)); + const struct term_cell *row = term_snapshot_row(snapshot, 0); + assert(row); + uint32_t cps[4]; + assert(term_snapshot_cluster(snapshot, row[0].text, cps, 4) == 2); + assert(cps[0] == 'e' && cps[1] == 0x301); + + term_snapshot_free(snapshot); + term_free(term); + + /* More than 256 rows moves both temporary arrays off the stack. */ + options.size.y = 300; + term = term_new(&options); + snapshot = term_snapshot_new(); + assert(term && snapshot && term_snapshot_update(snapshot, term)); + + /* Fail new_keys allocation, then make sure retry fills all rows. */ + malloc_was_failed = false; + malloc_failure_countdown = 0; + assert(!term_snapshot_update(snapshot, term)); + assert(malloc_was_failed); + assert(term_snapshot_view(snapshot)->size.y == 0); + assert(term_snapshot_row(snapshot, 0) == NULL); + assert(term_snapshot_update(snapshot, term)); + assert(term_snapshot_view(snapshot)->size.y == 300); + assert(term_snapshot_row(snapshot, 299)); + + /* The first malloc is new_keys; fail the following selection-span array. */ + malloc_was_failed = false; + malloc_failure_countdown = 1; + assert(!term_snapshot_update(snapshot, term)); + assert(malloc_was_failed); + assert(term_snapshot_view(snapshot)->size.y == 0); + assert(term_snapshot_row(snapshot, 0) == NULL); + assert(term_snapshot_update(snapshot, term)); + assert(term_snapshot_view(snapshot)->size.y == 300); + assert(term_snapshot_row(snapshot, 299)); + + term_snapshot_free(snapshot); + term_free(term); + return 0; +} diff --git a/tests/snapshot_scroll_arena_test.c b/tests/snapshot_scroll_arena_test.c new file mode 100644 index 0000000..7da6507 --- /dev/null +++ b/tests/snapshot_scroll_arena_test.c @@ -0,0 +1,29 @@ +/* Include snapshot.c to inspect its private arena accounting. The static + * library supplies the terminal implementation; this translation unit's + * snapshot symbols satisfy the test's references before the archive is read. + */ +#include <assert.h> +#include "../src/snapshot.c" + +int main(void) +{ + struct term_options options = { .size = { 8, 24 }, .scrollback = 5 }; + struct term *t = term_new(&options); + struct term_snapshot *s = term_snapshot_new(); + assert(t && s && term_snapshot_update(s, t)); + + const char line[] = "e\xcc\x81\r\n"; + for (int i = 0; i < 10000; i++) { + term_feed(t, line, sizeof(line) - 1, 0); + assert(term_snapshot_update(s, t)); + } + + /* A fixed viewport holds fewer than 24 * 8 cluster records at once. + * Dead records are compacted before they can dominate this arena. */ + assert(term_history_rows(t) == 5); + assert(s->arena_len < 8192); + + term_snapshot_free(s); + term_free(t); + return 0; +} |
