This is an automated email from the git hooks/post-receive script.
git pushed a commit to branch tymux
in repository terminology.
View the commit online.
commit 75959e14c7698c80f72c2cc2f90a345ae7614058
Author: [email protected] <[email protected]>
AuthorDate: Sun Mar 22 13:35:04 2026 -0600
fix: address review issues in tymux backlog implementation
Fix struct alignment (explicit pad + static assert), make num_lines
atomic with acquire/release semantics, fix buffer fetch into wrong
array slot, update proxy message type range, clamp buffer requests
to avoid evicted IDs, use two-stage mmap for correct buffer sizing,
guard write_seq increment, use uint32_t consistently for buffer IDs,
and improve forward compatibility of recv payload checks.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
---
src/bin/termd_tymux.c | 50 +++++++++++------
src/bin/termsrv.c | 10 ++--
src/bin/termsrv.h | 7 ++-
src/bin/termtymux.c | 152 ++++++++++++++++++++++++++++++++++++--------------
src/bin/tymux.c | 2 +-
5 files changed, 153 insertions(+), 68 deletions(-)
diff --git a/src/bin/termd_tymux.c b/src/bin/termd_tymux.c
index 857d6ebb..d83c4e88 100644
--- a/src/bin/termd_tymux.c
+++ b/src/bin/termd_tymux.c
@@ -72,8 +72,8 @@ struct _TermDTymux
} *backlog_bufs; /* dynamic array of buffers */
int backlog_buf_count; /* number of buffers in chain */
int backlog_buf_alloc; /* allocated array slots */
- int oldest_buf_id; /* global id of oldest buffer */
- int next_buf_id; /* next global id to assign */
+ uint32_t oldest_buf_id; /* global id of oldest buffer */
+ uint32_t next_buf_id; /* next global id to assign */
size_t last_flushed_backpos; /* backpos at last flush */
};
@@ -156,7 +156,7 @@ _backlog_buf_new(TermDTymux *sess, int cols)
hdr->magic = TERMSRV_BACKLOG_MAGIC;
hdr->version = TERMSRV_BACKLOG_VERSION;
hdr->cols = (uint32_t)cols;
- hdr->num_lines = 0;
+ atomic_store_explicit(&hdr->num_lines, 0, memory_order_relaxed);
hdr->max_lines = TERMSRV_BACKLOG_MAX_LINES;
atomic_store_explicit(&hdr->write_seq, 0, memory_order_relaxed);
@@ -191,6 +191,7 @@ _flush_backlog_to_shm(TermDTymux *sess)
size_t backsize = ty->backsize;
size_t new_lines;
size_t i;
+ int lines_written = 0;
if (cur_backpos == old_backpos)
return;
@@ -226,18 +227,22 @@ _flush_backlog_to_shm(TermDTymux *sess)
/* get or create a head buffer with matching cols */
hdr = sess->backlog_bufs[sess->backlog_buf_count - 1].hdr;
- if (hdr->num_lines >= hdr->max_lines || (int)hdr->cols != ty->w)
- {
- if (_backlog_buf_new(sess, ty->w) < 0)
- break;
- hdr = sess->backlog_bufs[sess->backlog_buf_count - 1].hdr;
- }
+ {
+ uint32_t cur_num = atomic_load_explicit(&hdr->num_lines,
+ memory_order_relaxed);
+ if (cur_num >= hdr->max_lines || (int)hdr->cols != ty->w)
+ {
+ if (_backlog_buf_new(sess, ty->w) < 0)
+ break;
+ hdr = sess->backlog_bufs[sess->backlog_buf_count - 1].hdr;
+ }
+ }
ts = BACKLOG_ROW_GET(ty, (int)i);
if (!ts || !ts->cells)
continue;
- line_idx = hdr->num_lines;
+ line_idx = atomic_load_explicit(&hdr->num_lines, memory_order_relaxed);
dst = TERMSRV_BACKLOG_LINE(hdr, line_idx);
copy_cols = (ts->w < hdr->cols) ? (uint32_t)ts->w : hdr->cols;
@@ -247,7 +252,9 @@ _flush_backlog_to_shm(TermDTymux *sess)
(size_t)(hdr->cols - copy_cols) * sizeof(Termcell));
hdr->line_widths[line_idx] = copy_cols;
- hdr->num_lines++;
+ atomic_store_explicit(&hdr->num_lines, line_idx + 1,
+ memory_order_relaxed);
+ lines_written++;
}
/* evict oldest buffers while total lines exceed configured scrollback,
@@ -256,18 +263,24 @@ _flush_backlog_to_shm(TermDTymux *sess)
int total_lines = 0;
int b;
for (b = 0; b < sess->backlog_buf_count; b++)
- total_lines += (int)sess->backlog_bufs[b].hdr->num_lines;
+ total_lines += (int)atomic_load_explicit(
+ &sess->backlog_bufs[b].hdr->num_lines,
+ memory_order_relaxed);
while (total_lines > sess->config->scrollback &&
sess->backlog_buf_count > 1)
{
- total_lines -= (int)sess->backlog_bufs[0].hdr->num_lines;
+ total_lines -= (int)atomic_load_explicit(
+ &sess->backlog_bufs[0].hdr->num_lines,
+ memory_order_relaxed);
_backlog_buf_evict_oldest(sess);
}
}
- /* bump head buffer write_seq with release semantics */
- if (sess->backlog_buf_count > 0)
+ /* bump head buffer write_seq with release semantics, but only when at
+ * least one line was actually appended — a no-op flush must not advance
+ * the counter and cause the client to re-read an unchanged buffer */
+ if (lines_written > 0 && sess->backlog_buf_count > 0)
{
TermBacklogBufHeader *head =
sess->backlog_bufs[sess->backlog_buf_count - 1].hdr;
@@ -345,7 +358,7 @@ _pty_change_cb(void *data)
pay.write_seq = atomic_load_explicit(&sess->shm->write_seq,
memory_order_acquire);
pay.backlog_buf_count = (uint32_t)sess->backlog_buf_count;
- pay.oldest_buf_id = (uint32_t)sess->oldest_buf_id;
+ pay.oldest_buf_id = sess->oldest_buf_id;
if (sess->backlog_buf_count > 0)
{
TermBacklogBufHeader *head =
@@ -456,7 +469,8 @@ _cb_client_data(void *data, Ecore_Fd_Handler *fd_handler EINA_UNUSED)
sess->backlog_bufs[idx].fd,
req->buf_id,
h->cols,
- h->num_lines,
+ atomic_load_explicit(&h->num_lines,
+ memory_order_relaxed),
h->max_lines);
}
}
@@ -661,7 +675,7 @@ termd_tymux_client_add(TermDTymux *sess, int cfd)
(uint32_t)sess->pty->h,
(uint32_t)sess->backlog_buf_count,
TERMSRV_BACKLOG_MAX_LINES,
- (uint32_t)sess->oldest_buf_id) < 0)
+ sess->oldest_buf_id) < 0)
{
ERR("termsrv_send_attached failed for fd %d", cfd);
close(cfd);
diff --git a/src/bin/termsrv.c b/src/bin/termsrv.c
index 60a24cc5..6f299690 100644
--- a/src/bin/termsrv.c
+++ b/src/bin/termsrv.c
@@ -210,7 +210,8 @@ termsrv_send_attached(int sock_fd, int shm_fd, int backlog_fd,
if (nfds == 2)
memcpy((char *)CMSG_DATA(cm) + sizeof(int), &backlog_fd, sizeof(int));
- return (sendmsg(sock_fd, &msg, MSG_NOSIGNAL) < 0) ? -1 : 0;
+ ssize_t n = sendmsg(sock_fd, &msg, MSG_NOSIGNAL);
+ return (n == (ssize_t)(sizeof(hdr) + sizeof(pl))) ? 0 : -1;
}
int
@@ -242,7 +243,7 @@ termsrv_recv_attached(int sock_fd, int *shm_fd_out, int *backlog_fd_out,
if (n < (ssize_t)(sizeof(hdr) + sizeof(pl))) return -1;
if (msg.msg_flags & MSG_CTRUNC) return -1;
if (hdr.type != TSRV_MSG_ATTACHED) return -1;
- if (hdr.size != sizeof(TermSrvMsgAttached)) return -1;
+ if (hdr.size < sizeof(TermSrvMsgAttached)) return -1;
struct cmsghdr *cm = CMSG_FIRSTHDR(&msg);
if (!cm || cm->cmsg_type != SCM_RIGHTS ||
@@ -292,7 +293,8 @@ termsrv_send_backlog_buf(int sock_fd, int buf_fd,
cm->cmsg_len = CMSG_LEN(sizeof(int));
memcpy(CMSG_DATA(cm), &buf_fd, sizeof(int));
- return (sendmsg(sock_fd, &msg, MSG_NOSIGNAL) < 0) ? -1 : 0;
+ ssize_t n = sendmsg(sock_fd, &msg, MSG_NOSIGNAL);
+ return (n == (ssize_t)(sizeof(hdr) + sizeof(pl))) ? 0 : -1;
}
int
@@ -320,7 +322,7 @@ termsrv_recv_backlog_buf(int sock_fd, int *buf_fd_out,
if (n < (ssize_t)(sizeof(hdr) + sizeof(pl))) return -1;
if (msg.msg_flags & MSG_CTRUNC) return -1;
if (hdr.type != TSRV_MSG_BACKLOG_BUF) return -1;
- if (hdr.size != sizeof(TermSrvMsgBacklogBuf)) return -1;
+ if (hdr.size < sizeof(TermSrvMsgBacklogBuf)) return -1;
struct cmsghdr *cm = CMSG_FIRSTHDR(&msg);
if (!cm || cm->cmsg_type != SCM_RIGHTS ||
diff --git a/src/bin/termsrv.h b/src/bin/termsrv.h
index 97921815..035db6c7 100644
--- a/src/bin/termsrv.h
+++ b/src/bin/termsrv.h
@@ -44,13 +44,16 @@ typedef struct {
uint32_t magic; /* TERMSRV_BACKLOG_MAGIC */
uint32_t version; /* TERMSRV_BACKLOG_VERSION */
uint32_t cols; /* terminal width when buf created */
- uint32_t num_lines; /* lines currently stored */
+ _Atomic uint32_t num_lines; /* lines currently stored */
uint32_t max_lines; /* capacity (TERMSRV_BACKLOG_MAX_LINES) */
- uint8_t _pad[3];
+ uint8_t _pad[4];
_Atomic uint64_t write_seq; /* head buffer only: incremented on append */
uint32_t line_widths[TERMSRV_BACKLOG_MAX_LINES];
} TermBacklogBufHeader;
+_Static_assert(offsetof(TermBacklogBufHeader, write_seq) % 8 == 0,
+ "write_seq must be 8-byte aligned for atomic access");
+
#define TERMSRV_BACKLOG_HEADER_SIZE \
((sizeof(TermBacklogBufHeader) + 4095u) & ~4095u) /* round up to page */
diff --git a/src/bin/termtymux.c b/src/bin/termtymux.c
index 8861d1c5..72203836 100644
--- a/src/bin/termtymux.c
+++ b/src/bin/termtymux.c
@@ -103,8 +103,14 @@ _backlog_bufs_evict_old(TermTymux *ts, uint32_t new_oldest_id)
}
/*
- * Request and mmap a backlog buffer from the daemon, then push it into
- * ts->backlog_bufs. Returns EINA_TRUE on success.
+ * Request and mmap a backlog buffer from the daemon.
+ *
+ * If slot >= 0, the fetched buffer fills the existing NULL entry at
+ * ts->backlog_bufs[slot] (called from _backlog_get_cb for a known-position
+ * unmapped buffer). If slot < 0, the buffer is appended at the tail
+ * (called from the NOTIFY handler for new head buffers).
+ *
+ * Returns EINA_TRUE on success.
*
* NOTE: this temporarily clears O_NONBLOCK on the socket (via
* termtymux_request_backlog_buf) and must only be called from the main
@@ -112,7 +118,7 @@ _backlog_bufs_evict_old(TermTymux *ts, uint32_t new_oldest_id)
* non-blocking recv).
*/
static Eina_Bool
-_backlog_buf_fetch(TermTymux *ts, uint32_t buf_id)
+_backlog_buf_fetch(TermTymux *ts, uint32_t buf_id, int slot)
{
int fd = -1;
uint32_t cols = 0, num_lines = 0, max_lines = 0;
@@ -150,10 +156,22 @@ _backlog_buf_fetch(TermTymux *ts, uint32_t buf_id)
return EINA_FALSE;
}
- if (!_backlog_buf_push(ts, hdr, map_size, buf_id))
+ if (slot >= 0)
{
- munmap(ptr, map_size);
- return EINA_FALSE;
+ /* Fill the pre-existing NULL slot directly — no array growth needed */
+ TermBacklogBuf *entry = &ts->backlog_bufs[slot];
+ entry->hdr = hdr;
+ entry->size = map_size;
+ /* entry->buf_id is already set */
+ }
+ else
+ {
+ /* Append new head buffer */
+ if (!_backlog_buf_push(ts, hdr, map_size, buf_id))
+ {
+ munmap(ptr, map_size);
+ return EINA_FALSE;
+ }
}
return EINA_TRUE;
@@ -180,12 +198,20 @@ _backlog_update_backsize(TermTymux *ts)
if (b->hdr)
{
- /* head buffer: num_lines is live (re-read atomically) */
- total += b->hdr->num_lines;
+ /* head buffer: num_lines is live — acquire ensures we see all
+ * cell writes that happened before the daemon incremented it */
+ total += atomic_load_explicit(&b->hdr->num_lines,
+ memory_order_acquire);
}
else
{
- /* unmapped historical buffer: assume it was filled */
+ /* Unmapped historical buffer: assume fully filled.
+ * BUG-7: a buffer created due to a terminal resize may be only
+ * partially filled, making this an overcount. The NOTIFY
+ * protocol does not carry per-buffer num_lines, so we have no
+ * way to know the true count until the buffer is fetched. The
+ * overcount is cosmetic — the scrollbar range is slightly too
+ * large — and corrects itself once the buffer is fetched. */
total += TERMSRV_BACKLOG_MAX_LINES;
}
}
@@ -233,7 +259,8 @@ _backlog_get_cb(void *data, int y, ssize_t *wret)
* The head (newest, last in array) may be partially filled;
* older buffers are assumed full if not yet mmap'd. */
if (b->hdr)
- n_lines = b->hdr->num_lines; /* live read from mmap */
+ n_lines = atomic_load_explicit(&b->hdr->num_lines,
+ memory_order_acquire); /* live read */
else if (i == ts->backlog_buf_count - 1)
n_lines = 0; /* head buf should always be mmap'd */
else
@@ -254,21 +281,26 @@ _backlog_get_cb(void *data, int y, ssize_t *wret)
/* Ensure buffer is mmap'd */
if (!b->hdr)
{
- if (!_backlog_buf_fetch(ts, b->buf_id))
+ /* Pass slot=i so _backlog_buf_fetch fills this existing entry
+ * directly rather than appending a duplicate at the tail. */
+ if (!_backlog_buf_fetch(ts, b->buf_id, i))
return NULL;
- /* _backlog_buf_fetch appended to end; our 'b' pointer is now
- * stale only if the realloc moved the array. Re-look-up. */
- b = &ts->backlog_bufs[i];
+ b = &ts->backlog_bufs[i]; /* re-look-up in case array was reallocated */
}
hdr = b->hdr;
- if (!hdr || hdr->num_lines == 0)
+ if (!hdr)
return NULL;
-
- /* line_idx: 0 = oldest, num_lines-1 = newest */
- /* local_offset=1 → newest = num_lines-1 */
- if (local_offset == 0 || local_offset > hdr->num_lines)
- return NULL;
- line_idx = hdr->num_lines - local_offset;
+ {
+ uint32_t nl = atomic_load_explicit(&hdr->num_lines,
+ memory_order_acquire);
+ if (nl == 0)
+ return NULL;
+ /* line_idx: 0 = oldest, nl-1 = newest */
+ /* local_offset=1 → newest = nl-1 */
+ if (local_offset == 0 || local_offset > nl)
+ return NULL;
+ line_idx = nl - local_offset;
+ }
*wret = (ssize_t)hdr->line_widths[line_idx];
return TERMSRV_BACKLOG_LINE(hdr, line_idx);
@@ -505,6 +537,11 @@ _cb_tymux_data(void *data, Ecore_Fd_Handler *handler EINA_UNUSED)
ts->oldest_buf_id = new_oldest;
}
+ /* Clamp next_new_id so we never request buffer ids that the
+ * daemon has already evicted (can happen if we lag behind) */
+ if (next_new_id < new_oldest)
+ next_new_id = new_oldest;
+
/* Fetch new head buffer(s) produced since the last NOTIFY.
* new_head = oldest + count - 1; we want every id from
* next_new_id through new_head. */
@@ -528,7 +565,7 @@ _cb_tymux_data(void *data, Ecore_Fd_Handler *handler EINA_UNUSED)
}
}
if (!already)
- _backlog_buf_fetch(ts, id);
+ _backlog_buf_fetch(ts, id, -1);
}
}
@@ -830,40 +867,69 @@ termtymux_attach(const char *tymux_name,
ts->last_backlog_buf_count = backlog_buf_count;
/* mmap the head backlog buffer received in ATTACHED.
- * Map at maximum size so we don't need to do a two-stage map just to
- * read the cols field. Excess virtual pages have no physical cost. */
+ * Use a two-stage map: first map just the header page to read the actual
+ * cols width, then re-map at the correct total size. Mapping at
+ * TERMSRV_BACKLOG_TOTAL_SIZE(TERMSRV_MAX_COLS) when the file is smaller
+ * (because the session was started with fewer columns) would cause SIGBUS
+ * on any access past the file's actual end. */
if (backlog_fd >= 0 && backlog_buf_count > 0)
{
- size_t map_size = TERMSRV_BACKLOG_TOTAL_SIZE(TERMSRV_MAX_COLS);
- void *ptr;
+ TermBacklogBufHeader *hdr_tmp;
+ void *hdr_page;
+ uint32_t head_cols = 0;
+ uint32_t head_id = oldest_buf_id + backlog_buf_count - 1;
- ptr = mmap(NULL, map_size, PROT_READ, MAP_SHARED, backlog_fd, 0);
- close(backlog_fd);
- backlog_fd = -1;
-
- if (ptr != MAP_FAILED)
+ hdr_page = mmap(NULL, TERMSRV_BACKLOG_HEADER_SIZE,
+ PROT_READ, MAP_SHARED, backlog_fd, 0);
+ if (hdr_page != MAP_FAILED)
{
- TermBacklogBufHeader *hdr = ptr;
- uint32_t head_id;
+ hdr_tmp = hdr_page;
+ if (hdr_tmp->magic == TERMSRV_BACKLOG_MAGIC &&
+ hdr_tmp->version == TERMSRV_BACKLOG_VERSION &&
+ hdr_tmp->cols > 0 &&
+ hdr_tmp->cols <= TERMSRV_MAX_COLS)
+ head_cols = hdr_tmp->cols;
+ munmap(hdr_page, TERMSRV_BACKLOG_HEADER_SIZE);
+ }
+ else
+ WRN("termtymux: mmap head backlog buf header: %s", strerror(errno));
- if (hdr->magic == TERMSRV_BACKLOG_MAGIC &&
- hdr->version == TERMSRV_BACKLOG_VERSION)
+ if (head_cols > 0)
+ {
+ size_t map_size = TERMSRV_BACKLOG_TOTAL_SIZE(head_cols);
+ void *ptr;
+
+ ptr = mmap(NULL, map_size, PROT_READ, MAP_SHARED, backlog_fd, 0);
+ close(backlog_fd);
+ backlog_fd = -1;
+
+ if (ptr != MAP_FAILED)
{
- head_id = oldest_buf_id + backlog_buf_count - 1;
- if (!_backlog_buf_push(ts, hdr, map_size, head_id))
+ TermBacklogBufHeader *hdr = ptr;
+
+ if (hdr->magic == TERMSRV_BACKLOG_MAGIC &&
+ hdr->version == TERMSRV_BACKLOG_VERSION)
{
- WRN("termtymux: failed to push head backlog buf");
+ if (!_backlog_buf_push(ts, hdr, map_size, head_id))
+ {
+ WRN("termtymux: failed to push head backlog buf");
+ munmap(ptr, map_size);
+ }
+ }
+ else
+ {
+ WRN("termtymux: head backlog buf magic/version mismatch");
munmap(ptr, map_size);
}
}
else
- {
- WRN("termtymux: head backlog buf magic/version mismatch");
- munmap(ptr, map_size);
- }
+ WRN("termtymux: mmap head backlog buf: %s", strerror(errno));
}
else
- WRN("termtymux: mmap head backlog buf: %s", strerror(errno));
+ {
+ close(backlog_fd);
+ backlog_fd = -1;
+ }
}
else if (backlog_fd >= 0)
{
diff --git a/src/bin/tymux.c b/src/bin/tymux.c
index 0040b137..966dbe7a 100644
--- a/src/bin/tymux.c
+++ b/src/bin/tymux.c
@@ -276,7 +276,7 @@ _proto_recv(int fd, void **payload_out, uint32_t *size_out)
return -1;
}
- if (hdr.type == 0 || hdr.type > 9) return -1;
+ if (hdr.type == 0 || hdr.type > 11) return -1;
if (hdr.size > (1u << 20)) return -1;
if (hdr.size > 0)
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
