Hi Jeremy,
I glanced the patch and it looks good.
Do you perhaps also have a way to easily reproduce this deadlock condition?
On Thu, Jun 27, 2024 at 06:51:28PM -0700, Jeremy Bobbin wrote:
> because underlying programs can request info from the tty, there was a
> potential recursion in ttyread:
> 1. ttyread
> 2. tty program wants window size
> 3. ttywrite the window size
>
> this assumes the ttyfd is both ready for read & write.
>
> if, for example, the tty wants st to report the cursor position & the
> write pipe is already cloggled, then this could cause a deadlock.
>
> this was initialy addressed with 2 selects:
>
> one in x.c which scanned for { display events & tty for reading }.
> another in st.c which scanned for { read & write }-ability of the tty.
>
> this patch, instead, buffers what we write to the TTY, and then
> conditionally selects over tty readability, tty writability & display
> events.
>
> in addition to simplifying the ttywrite function, this will make fewer
> calls to select & should make fewer calls to write.
> ---
> st.c | 92 ++++++++++++++++++++++++++++--------------------------------
> st.h | 3 ++
> x.c | 9 +++++-
> 3 files changed, 54 insertions(+), 50 deletions(-)
>
> diff --git a/st.c b/st.c
> index 57c6e96..030c294 100644
> --- a/st.c
> +++ b/st.c
> @@ -152,6 +152,12 @@ typedef struct {
> int narg; /* nb of args */
> } STREscape;
>
> +typedef struct {
> + char *buf; /* allocated raw string */
> + size_t siz; /* allocation size */
> + size_t len; /* raw string length */
> +} TBuffer;
> +
> static void execsh(char *, char **);
> static void stty(char **);
> static void sigchld(int);
> @@ -223,6 +229,7 @@ static Term term;
> static Selection sel;
> static CSIEscape csiescseq;
> static STREscape strescseq;
> +static TBuffer tbuf;
> static int iofd = 1;
> static int cmdfd;
> static pid_t pid;
> @@ -752,6 +759,35 @@ stty(char **args)
> perror("Couldn't call stty");
> }
>
> +void
> +tbufnew(size_t siz) {
> + tbuf.buf = (char *)xmalloc(siz);
> + tbuf.siz = siz;
> + tbuf.len = 0;
> +}
> +
> +size_t
> +tbuflen(void) {
> + return tbuf.len;
> +}
> +
> +size_t
> +tbufwrite(void) {
> + size_t lim = 256;
> + ssize_t r;
> +
> + /*
> + * Migrate the world to Plan 9.
> + */
> + if ((r = write(cmdfd, &tbuf.buf[0], MIN(tbuf.len, lim))) < 0)
> + die("write error on tty: %s\n", strerror(errno));
> +
> + tbuf.len -= r;
> + memmove(tbuf.buf, &tbuf.buf[r], tbuf.len);
> +
> + return r;
> +}
> +
> int
> ttynew(const char *line, char *cmd, const char *out, char **args)
> {
> @@ -868,61 +904,19 @@ ttywrite(const char *s, size_t n, int may_echo)
> }
> }
>
> +
> void
> ttywriteraw(const char *s, size_t n)
> {
> - fd_set wfd, rfd;
> - ssize_t r;
> - size_t lim = 256;
> -
> - /*
> - * Remember that we are using a pty, which might be a modem line.
> - * Writing too much will clog the line. That's why we are doing this
> - * dance.
> - * FIXME: Migrate the world to Plan 9.
> - */
> - while (n > 0) {
> - FD_ZERO(&wfd);
> - FD_ZERO(&rfd);
> - FD_SET(cmdfd, &wfd);
> - FD_SET(cmdfd, &rfd);
> -
> - /* Check if we can write. */
> - if (pselect(cmdfd+1, &rfd, &wfd, NULL, NULL, NULL) < 0) {
> - if (errno == EINTR)
> - continue;
> - die("select failed: %s\n", strerror(errno));
> - }
> - if (FD_ISSET(cmdfd, &wfd)) {
> - /*
> - * Only write the bytes written by ttywrite() or the
> - * default of 256. This seems to be a reasonable value
> - * for a serial line. Bigger values might clog the I/O.
> - */
> - if ((r = write(cmdfd, s, (n < lim)? n : lim)) < 0)
> - goto write_error;
> - if (r < n) {
> - /*
> - * We weren't able to write out everything.
> - * This means the buffer is getting full
> - * again. Empty it.
> - */
> - if (n < lim)
> - lim = ttyread();
> - n -= r;
> - s += r;
> - } else {
> - /* All bytes have been written. */
> - break;
> - }
> - }
> - if (FD_ISSET(cmdfd, &rfd))
> - lim = ttyread();
> + if (tbuf.siz - tbuf.len < n) {
> + tbuf.siz *= 2;
> + tbuf.buf = xrealloc(tbuf.buf, tbuf.siz);
> }
> + memcpy(&tbuf.buf[tbuf.len], s, n);
> + tbuf.len += n;
> +
> return;
>
> -write_error:
> - die("write error on tty: %s\n", strerror(errno));
> }
>
> void
> diff --git a/st.h b/st.h
> index fd3b0d8..2b810b8 100644
> --- a/st.h
> +++ b/st.h
> @@ -87,6 +87,9 @@ void sendbreak(const Arg *);
> void toggleprinter(const Arg *);
>
> int tattrset(int);
> +void tbufnew(size_t);
> +size_t tbuflen(void);
> +size_t tbufwrite(void);
> void tnew(int, int);
> void tresize(int, int);
> void tsetdirtattr(int);
> diff --git a/x.c b/x.c
> index bd23686..f1ac1a9 100644
> --- a/x.c
> +++ b/x.c
> @@ -1922,7 +1922,7 @@ run(void)
> {
> XEvent ev;
> int w = win.w, h = win.h;
> - fd_set rfd;
> + fd_set rfd, wfd;
> int xfd = XConnectionNumber(xw.dpy), ttyfd, xev, drawing;
> struct timespec seltv, *tv, now, lastblink, trigger;
> double timeout;
> @@ -1948,8 +1948,11 @@ run(void)
>
> for (timeout = -1, drawing = 0, lastblink = (struct timespec){0};;) {
> FD_ZERO(&rfd);
> + FD_ZERO(&wfd);
> FD_SET(ttyfd, &rfd);
> FD_SET(xfd, &rfd);
> + if (tbuflen() > 0)
> + FD_SET(ttyfd, &wfd);
>
> if (XPending(xw.dpy))
> timeout = 0; /* existing events might not set xfd */
> @@ -1968,6 +1971,9 @@ run(void)
> if (FD_ISSET(ttyfd, &rfd))
> ttyread();
>
> + if (FD_ISSET(ttyfd, &wfd))
> + tbufwrite();
> +
> xev = 0;
> while (XPending(xw.dpy)) {
> xev = 1;
> @@ -2095,6 +2101,7 @@ run:
> XSetLocaleModifiers("");
> cols = MAX(cols, 1);
> rows = MAX(rows, 1);
> + tbufnew(256);
> tnew(cols, rows);
> xinit(cols, rows);
> xsetenv();
> --
> 2.45.2
>
>
--
Kind regards,
Hiltjo
