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 5005b72f0492de2e311a5fafd3ba138a915d973b
Author: [email protected] <[email protected]>
AuthorDate: Wed Mar 18 22:24:02 2026 -0600
fix: address memory leaks, race conditions, and protocol issues from code review
Multiple fixes to tymux/termsrv protocol handling, session lifecycle, and error
paths discovered in comprehensive code review:
- Memory leak: validate msg type before malloc to avoid leak on invalid type
- Client race: use MSG_WAITALL in accept handler to prevent EAGAIN on ATTACH
- Stale socket: probe with real connect() instead of stat() to detect crashed daemons
- Protocol: NUL-terminate TITLE payload to prevent buffer overread
- Efficiency: remove redundant flush after resize (already flushed by callback)
- Robustness: use MSG_NOSIGNAL on attach message send; detect partial sendmsg
- API cleanup: remove unused ts param from _shadow_pty_new
- Environment: export TYMUX_SESSION before exec for nested session detection
Changes:
- termsrv.h: move TERMSRV_MAX_MSG_PAYLOAD to shared header for reuse in tymuxd
- termsrv.c: validate type before malloc, add MSG_NOSIGNAL, check exact sendmsg bytes
- tymux_session.c: free payload on recv error, remove double flush on resize
- tymuxd.c: use MSG_WAITALL for ATTACH handshake, drain payload with bounds check
- termsession.c: replace stat() probing with real connect() in _ensure_daemon,
NUL-terminate TITLE payload, remove unused parameter from _shadow_pty_new
- tymux.c: setenv TYMUX_SESSION before exec in both new and attach paths
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
---
src/bin/termsession.c | 65 ++++++++++++++++++++++++++++++++++++++-----------
src/bin/termsrv.c | 15 ++++++------
src/bin/termsrv.h | 6 +++++
src/bin/tymux.c | 6 +++++
src/bin/tymux_session.c | 5 +++-
src/bin/tymuxd.c | 31 ++++++++++++++++-------
6 files changed, 96 insertions(+), 32 deletions(-)
diff --git a/src/bin/termsession.c b/src/bin/termsession.c
index 6544739c..81438614 100644
--- a/src/bin/termsession.c
+++ b/src/bin/termsession.c
@@ -18,7 +18,6 @@
#include <fcntl.h>
#include <stdatomic.h>
#include <time.h>
-#include <sys/stat.h>
/* ── write_cb: forward input bytes to daemon via TSRV_MSG_INPUT ─────── */
@@ -38,7 +37,7 @@ _session_write_cb(const char *buf, int len, void *data)
/* ── shadow Termpty ─────────────────────────────────────────────────── */
static Termpty *
-_shadow_pty_new(TermSession *ts, int cols, int rows, Config *config)
+_shadow_pty_new(int cols, int rows, Config *config)
{
Termpty *ty;
@@ -230,7 +229,12 @@ _cb_session_data(void *data, Ecore_Fd_Handler *handler EINA_UNUSED)
case TSRV_MSG_TITLE:
if (payload && size > 0)
- eina_stringshare_replace(&ts->shadow_pty->prop.title, payload);
+ {
+ /* Force NUL-termination in case a malformed message omitted it,
+ * preventing eina_stringshare_replace from reading past the buffer. */
+ ((char *)payload)[size - 1] = '\0';
+ eina_stringshare_replace(&ts->shadow_pty->prop.title, payload);
+ }
if (ts->cb_title.func)
ts->cb_title.func(ts->cb_title.data);
break;
@@ -265,14 +269,31 @@ _cb_session_data(void *data, Ecore_Fd_Handler *handler EINA_UNUSED)
static int
_ensure_daemon(const char *session_name, const char *sock_path)
{
- struct timespec ts_sleep;
- int i;
- pid_t child;
- struct stat st;
+ struct timespec ts_sleep;
+ int i;
+ pid_t child;
- /* If socket already exists, nothing to do */
- if (stat(sock_path, &st) == 0)
- return 0;
+ /* Probe with a real connect so we distinguish a live daemon from a stale
+ * socket file left by a crashed one. stat() succeeds for both, but
+ * connect() returns ECONNREFUSED for the stale case. */
+ {
+ int probe = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0);
+ if (probe >= 0)
+ {
+ struct sockaddr_un sa;
+ memset(&sa, 0, sizeof(sa));
+ sa.sun_family = AF_UNIX;
+ strncpy(sa.sun_path, sock_path, sizeof(sa.sun_path) - 1);
+ if (connect(probe, (struct sockaddr *)&sa, sizeof(sa)) == 0)
+ {
+ close(probe);
+ return 0; /* live daemon already accepting */
+ }
+ if (errno == ECONNREFUSED)
+ unlink(sock_path); /* stale socket from a previous crash */
+ close(probe);
+ }
+ }
child = fork();
if (child < 0)
@@ -313,15 +334,31 @@ _ensure_daemon(const char *session_name, const char *sock_path)
/* Parent: reap the short-lived first child immediately */
waitpid(child, NULL, 0);
- /* Poll for socket with 10 ms intervals, 3 s total */
+ /* Poll for the daemon to start accepting, using connect probes.
+ * This correctly distinguishes a live socket from a stale one. */
ts_sleep.tv_sec = 0;
ts_sleep.tv_nsec = 10 * 1000 * 1000; /* 10 ms */
for (i = 0; i < 300; i++)
{
+ int probe;
+ struct sockaddr_un sa;
+
nanosleep(&ts_sleep, NULL);
- if (stat(sock_path, &st) == 0)
- return 0;
+
+ probe = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0);
+ if (probe < 0) continue;
+
+ memset(&sa, 0, sizeof(sa));
+ sa.sun_family = AF_UNIX;
+ strncpy(sa.sun_path, sock_path, sizeof(sa.sun_path) - 1);
+
+ if (connect(probe, (struct sockaddr *)&sa, sizeof(sa)) == 0)
+ {
+ close(probe);
+ return 0;
+ }
+ close(probe);
}
ERR("termsession: daemon did not create socket within 3 s: %s", sock_path);
@@ -427,7 +464,7 @@ termsession_attach(const char *session_name,
/* Use requested cols/rows for the shadow pty; _sync_shadow will
* adjust to whatever the server reports in the header. */
- ty = _shadow_pty_new(NULL, cols, rows, config);
+ ty = _shadow_pty_new(cols, rows, config);
if (!ty)
{
ERR("termsession: failed to allocate shadow pty");
diff --git a/src/bin/termsrv.c b/src/bin/termsrv.c
index 69adb7d9..6e113d05 100644
--- a/src/bin/termsrv.c
+++ b/src/bin/termsrv.c
@@ -12,10 +12,6 @@
#include "termsrv.h"
-/* Sanity cap on incoming message payload size. No tymux message should
- * ever approach this; it guards against a malformed hdr.size of ~4 GB. */
-#define TERMSRV_MAX_MSG_PAYLOAD (1u << 20) /* 1 MiB */
-
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
@@ -74,8 +70,9 @@ termsrv_msg_send(int fd, TermSrvMsgType type,
};
int iovcnt = (size && payload) ? 2 : 1;
struct msghdr msg = { .msg_iov = iov, .msg_iovlen = (size_t)iovcnt };
+ ssize_t expected = (ssize_t)(sizeof(hdr) + ((payload && size) ? size : 0));
ssize_t n = sendmsg(fd, &msg, MSG_NOSIGNAL);
- return (n < 0) ? -1 : 0;
+ return (n == expected) ? 0 : -1;
}
/*
@@ -154,6 +151,10 @@ termsrv_msg_recv(int fd, void **payload_out, uint32_t *size_out)
return -1;
}
+ /* Reject unknown message types before allocating — valid range is
+ * ATTACH(1)..EXIT(9). Checking type first prevents a malloc that
+ * would otherwise leak on the error path. */
+ if (hdr.type == 0 || hdr.type > 9) return -1;
if (hdr.size > TERMSRV_MAX_MSG_PAYLOAD) return -1;
if (hdr.size > 0)
@@ -164,8 +165,6 @@ termsrv_msg_recv(int fd, void **payload_out, uint32_t *size_out)
{ free(buf); return -1; }
*payload_out = buf;
}
- /* Reject unknown message types; valid range is ATTACH(1)..EXIT(9). */
- if (hdr.type == 0 || hdr.type > 9) return -1;
*size_out = hdr.size;
return (int)hdr.type;
@@ -197,7 +196,7 @@ termsrv_send_attached(int sock_fd, int shm_fd,
cm->cmsg_len = CMSG_LEN(sizeof(int));
memcpy(CMSG_DATA(cm), &shm_fd, sizeof(int));
- return (sendmsg(sock_fd, &msg, 0) < 0) ? -1 : 0;
+ return (sendmsg(sock_fd, &msg, MSG_NOSIGNAL) < 0) ? -1 : 0;
}
int
diff --git a/src/bin/termsrv.h b/src/bin/termsrv.h
index 7ba18a55..c6b7de16 100644
--- a/src/bin/termsrv.h
+++ b/src/bin/termsrv.h
@@ -85,6 +85,12 @@ typedef struct {
int32_t exit_code;
} TermSrvMsgExit;
+/* ── payload size limit ─────────────────────────────────────────── */
+
+/* Sanity cap on incoming message payload size. No tymux message should
+ * ever approach this; it guards against a malformed hdr.size of ~4 GB. */
+#define TERMSRV_MAX_MSG_PAYLOAD (1u << 20) /* 1 MiB */
+
/* ── session name ───────────────────────────────────────────────── */
/* Maximum length of a session name (excluding null terminator).
diff --git a/src/bin/tymux.c b/src/bin/tymux.c
index 440d14dc..44933765 100644
--- a/src/bin/tymux.c
+++ b/src/bin/tymux.c
@@ -228,6 +228,9 @@ cmd_new(int argc, char **argv)
printf("Session '%s' created.\n", name);
+ /* Advertise the session name so nested tymux invocations can warn. */
+ setenv("TYMUX_SESSION", name, 1);
+
/* Replace this process with terminology, connecting to the new session. */
execlp("terminology", "terminology", "--session", name, (char *)NULL);
perror("tymux: exec terminology");
@@ -266,6 +269,9 @@ cmd_attach(int argc, char **argv)
return 1;
}
+ /* Advertise the session name so nested tymux invocations can warn. */
+ setenv("TYMUX_SESSION", name, 1);
+
execlp("terminology", "terminology", "--session", name, (char *)NULL);
perror("tymux: exec terminology");
return 1;
diff --git a/src/bin/tymux_session.c b/src/bin/tymux_session.c
index 82c8b57b..28935210 100644
--- a/src/bin/tymux_session.c
+++ b/src/bin/tymux_session.c
@@ -201,6 +201,7 @@ _cb_client_data(void *data, Ecore_Fd_Handler *fd_handler EINA_UNUSED)
type = termsrv_msg_recv(client->fd, &payload, &size);
if (type < 0)
{
+ free(payload);
_client_remove(sess, client);
return ECORE_CALLBACK_CANCEL;
}
@@ -221,8 +222,10 @@ _cb_client_data(void *data, Ecore_Fd_Handler *fd_handler EINA_UNUSED)
if (cols > 0 && rows > 0 &&
cols <= TERMSRV_MAX_COLS && rows <= TERMSRV_MAX_ROWS)
{
+ /* termpty_resize triggers _pty_change_cb which flushes
+ * to shm and sends NOTIFY to all clients — no explicit
+ * _flush_screen_to_shm call needed here. */
termpty_resize(sess->pty, (int)cols, (int)rows);
- _flush_screen_to_shm(sess);
}
}
break;
diff --git a/src/bin/tymuxd.c b/src/bin/tymuxd.c
index 97c25928..89b7cab5 100644
--- a/src/bin/tymuxd.c
+++ b/src/bin/tymuxd.c
@@ -32,10 +32,9 @@ static Eina_Bool
_cb_client_connect(void *data EINA_UNUSED,
Ecore_Fd_Handler *fd_handler EINA_UNUSED)
{
- int cfd;
- void *payload = NULL;
- uint32_t size = 0;
- int type;
+ int cfd;
+ TermSrvMsgHdr hdr;
+ ssize_t n;
cfd = accept(_listen_fd, NULL, NULL);
if (cfd < 0)
@@ -45,16 +44,30 @@ _cb_client_connect(void *data EINA_UNUSED,
return ECORE_CALLBACK_RENEW;
}
- type = termsrv_msg_recv(cfd, &payload, &size);
- if (type != TSRV_MSG_ATTACH)
+ /* Read the ATTACH header with MSG_WAITALL so we do not race against the
+ * kernel delivering the bytes. The newly-accepted fd may not have data
+ * ready yet when accept() returns, so MSG_DONTWAIT (used by the normal
+ * termsrv_msg_recv path) would return EAGAIN → type 0 → drop the client
+ * silently. */
+ n = recv(cfd, &hdr, sizeof(hdr), MSG_WAITALL);
+ if (n != (ssize_t)sizeof(hdr) || hdr.type != TSRV_MSG_ATTACH)
{
- ERR("expected ATTACH from new client (got %d), closing fd %d", type, cfd);
- free(payload);
+ ERR("expected ATTACH from new client (hdr.type=%u n=%zd), closing fd %d",
+ (n == (ssize_t)sizeof(hdr)) ? hdr.type : 0u, n, cfd);
close(cfd);
return ECORE_CALLBACK_RENEW;
}
- free(payload);
+ /* Drain any payload the client sent with the ATTACH (session name). */
+ if (hdr.size > 0 && hdr.size <= TERMSRV_MAX_MSG_PAYLOAD)
+ {
+ void *tmp = malloc(hdr.size);
+ if (tmp)
+ {
+ recv(cfd, tmp, hdr.size, MSG_WAITALL);
+ free(tmp);
+ }
+ }
termd_session_client_add(_session, cfd);
return ECORE_CALLBACK_RENEW;
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
