starred/herdr
1
0
Fork 0
mirror of https://github.com/herdrdev/herdr.git synced 2026-11-06 00:06:18 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: stop server crash when a cursor move carries a link onto a full page

Browse Source 
refs #5296
This commit is contained in:
Ogulcan Celik
2026-10-15 17:48:51 +03:00
parent 916fcf8de0
commit 406456e7dc
3 changed files with 169 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files
vendor/libghostty-vt.patches.md
Vendored
+38
View File
@@ -228,3 +228,41 @@ just test-one kitty_file_image_survives
(cd vendor/libghostty-vt && zig build test-lib-vt -Dtest-filter='experimental PNG')
just check
```
## 0008 keep cursor hyperlink across page replacement
status: active
patch: `vendor/patches/libghostty-vt/0008-cursor-hyperlink-page-replace.patch`
herdr issue: https://github.com/herdrdev/herdr/issues/5296
upstream discussion: not opened
upstream pr: not opened
vendored base: `44f2a44df7e8c4a0c6df3f7d872ef3d7ead88e51`
local files:
- `vendor/libghostty-vt/src/terminal/Screen.zig`
reason: Moving the cursor onto another page while a hyperlink and a
non-default style are active can grow that page. Growth replaces the page,
re-adds the same heap hyperlink, and frees it. The outer move then clears
the pointer without clearing the id that replacement just installed, so
`endHyperlink` dereferences null and the server dies. The same sequence is
still present on upstream main (`cursorChangePin` around line 1606 and
`cursorPageReplaced` around line 706 at 22fa134a0). This is not the one-row
scrollback prune fixed by 65ff54c67.
remove when: the vendored source keeps the cursor hyperlink alive across a
page replacement during a pin change, and the Screen test below passes
without this patch.
verification:
```sh
(cd vendor/libghostty-vt && zig build test-lib-vt -Dtest-filter='cursorAbsolute across a full page keeps the hyperlink')
just maintenance-test
```
vendor/libghostty-vt/src/terminal/Screen.zig
Vendored
+51 -3
View File
@@ -1530,6 +1530,10 @@ inline fn cursorChangePin(self: *Screen, new: Pin) void {
// Update our pin to the new page
self.cursor.page_pin.* = new;
// Remember the heap link before style migration. Growing the
// destination page replaces it and frees this object.
const pre_style_link = self.cursor.hyperlink;
// On the new page, we need to migrate our style
if (old_style_) |old_style| {
self.cursor.style = old_style;
@@ -1546,10 +1550,14 @@ inline fn cursorChangePin(self: *Screen, new: Pin) void {
// On the new page, we need to migrate our hyperlink
if (self.cursor.hyperlink) |link| {
// startHyperlink will try to free old hyperlinks, so set this
// to null. We free it ourselves later since we're doing some
// ref-counting shenanigans in this function.
// Page replacement during style migration already inserted this
// link on the new page and freed the pre-move object.
if (link != pre_style_link) return;
// Clear both fields so startHyperlink doesn't try to end a link
// through a null pointer. We free the old link ourselves below.
self.cursor.hyperlink = null;
self.cursor.hyperlink_id = 0;
// Re-add
self.startHyperlink(link.uri, switch (link.id) {
@@ -5021,6 +5029,46 @@ test "Screen: cursorAbsolute to page with insufficient capacity" {
new_page.assertIntegrity();
}
test "Screen: cursorAbsolute across a full page keeps the hyperlink" {
const testing = std.testing;
const alloc = testing.allocator;
const io = testing.io;
var s = try init(io, alloc, .{ .cols = 10, .rows = 3, .max_scrollback_bytes = 1 });
defer s.deinit();
const start_page = s.pages.pages.last.?.page();
const rem = start_page.capacity.rows;
start_page.pauseIntegrityChecks(true);
for (0..rem) |_| try s.cursorDownOrScroll();
start_page.pauseIntegrityChecks(false);
try testing.expect(start_page != s.cursor.page_pin.node.page());
{
start_page.pauseIntegrityChecks(true);
defer start_page.pauseIntegrityChecks(false);
var n: u24 = 1;
while (start_page.styles.add(
start_page.memory,
.{ .bg_color = .{ .rgb = @bitCast(n) } },
)) |_| n += 1 else |_| {}
}
try s.setAttribute(.{ .bold = {} });
try s.startHyperlink("https://example.com/5296", "keep");
// The destination style set is full, so this pin change grows the page
// while the cursor still holds the pre-move hyperlink.
s.cursorAbsolute(1, 1);
try testing.expect(s.cursor.hyperlink_id != 0);
try testing.expectEqualStrings("https://example.com/5296", s.cursor.hyperlink.?.uri);
try testing.expectEqualStrings("keep", s.cursor.hyperlink.?.id.explicit);
s.endHyperlink();
try testing.expect(s.cursor.hyperlink == null);
try testing.expect(s.cursor.hyperlink_id == 0);
}
test "Screen: scrolling" {
const testing = std.testing;
const alloc = testing.allocator;
vendor/patches/libghostty-vt/0008-cursor-hyperlink-page-replace.patch
Vendored
+80
View File
@@ -0,0 +1,80 @@
diff --git a/vendor/libghostty-vt/src/terminal/Screen.zig b/vendor/libghostty-vt/src/terminal/Screen.zig
index d0d2b929..b395c28c 100644
--- a/vendor/libghostty-vt/src/terminal/Screen.zig
+++ b/vendor/libghostty-vt/src/terminal/Screen.zig
@@ -1530,6 +1530,10 @@ inline fn cursorChangePin(self: *Screen, new: Pin) void {
// Update our pin to the new page
self.cursor.page_pin.* = new;
+ // Remember the heap link before style migration. Growing the
+ // destination page replaces it and frees this object.
+ const pre_style_link = self.cursor.hyperlink;
+
// On the new page, we need to migrate our style
if (old_style_) |old_style| {
self.cursor.style = old_style;
@@ -1546,10 +1550,14 @@ inline fn cursorChangePin(self: *Screen, new: Pin) void {
// On the new page, we need to migrate our hyperlink
if (self.cursor.hyperlink) |link| {
- // startHyperlink will try to free old hyperlinks, so set this
- // to null. We free it ourselves later since we're doing some
- // ref-counting shenanigans in this function.
+ // Page replacement during style migration already inserted this
+ // link on the new page and freed the pre-move object.
+ if (link != pre_style_link) return;
+
+ // Clear both fields so startHyperlink doesn't try to end a link
+ // through a null pointer. We free the old link ourselves below.
self.cursor.hyperlink = null;
+ self.cursor.hyperlink_id = 0;
// Re-add
self.startHyperlink(link.uri, switch (link.id) {
@@ -5021,6 +5029,46 @@ test "Screen: cursorAbsolute to page with insufficient capacity" {
new_page.assertIntegrity();
}
+test "Screen: cursorAbsolute across a full page keeps the hyperlink" {
+ const testing = std.testing;
+ const alloc = testing.allocator;
+ const io = testing.io;
+
+ var s = try init(io, alloc, .{ .cols = 10, .rows = 3, .max_scrollback_bytes = 1 });
+ defer s.deinit();
+
+ const start_page = s.pages.pages.last.?.page();
+ const rem = start_page.capacity.rows;
+ start_page.pauseIntegrityChecks(true);
+ for (0..rem) |_| try s.cursorDownOrScroll();
+ start_page.pauseIntegrityChecks(false);
+ try testing.expect(start_page != s.cursor.page_pin.node.page());
+
+ {
+ start_page.pauseIntegrityChecks(true);
+ defer start_page.pauseIntegrityChecks(false);
+ var n: u24 = 1;
+ while (start_page.styles.add(
+ start_page.memory,
+ .{ .bg_color = .{ .rgb = @bitCast(n) } },
+ )) |_| n += 1 else |_| {}
+ }
+
+ try s.setAttribute(.{ .bold = {} });
+ try s.startHyperlink("https://example.com/5296", "keep");
+
+ // The destination style set is full, so this pin change grows the page
+ // while the cursor still holds the pre-move hyperlink.
+ s.cursorAbsolute(1, 1);
+
+ try testing.expect(s.cursor.hyperlink_id != 0);
+ try testing.expectEqualStrings("https://example.com/5296", s.cursor.hyperlink.?.uri);
+ try testing.expectEqualStrings("keep", s.cursor.hyperlink.?.id.explicit);
+ s.endHyperlink();
+ try testing.expect(s.cursor.hyperlink == null);
+ try testing.expect(s.cursor.hyperlink_id == 0);
+}
+
test "Screen: scrolling" {
const testing = std.testing;
const alloc = testing.allocator;