On Mon, Apr 11, 2022 at 12:08:06PM +0200, Karel Zak wrote:
> On Fri, Jan 14, 2022 at 12:28:11AM +0100, наб wrote:
> > -static int write_to_child(struct ul_pty *pty, char *buf, size_t bufsz)
> > +static int schedule_child_write(struct ul_pty *pty, char *buf, size_t bufsz, int final)
> >  {
> > -	return write_all(pty->master, buf, bufsz);
> > +	struct ul_pty_child_buffer *stash = calloc(1, sizeof(*stash));
> 
> It means that for each activity on the file descriptor it will
> allocate a new buffer (in BUFSIZ). It seems pretty expensive.
> 
> Cannot we reuse the buffers? 

Added pty->free_buffers where we put free-to-use (fully-written-out buffers)
instead of free()ing them; my testing indicates, that for interactive use
we allocate a single buffer and re-use 100% of the time.

> >  /*
> > @@ -311,16 +329,13 @@ static int write_to_child(struct ul_pty
> >   * maintains master+slave tty stuff within the session. Use pipe to write to
> >   * pty and assume non-interactive (tee-like) behavior is NOT well supported.
> >   */
> > -void ul_pty_write_eof_to_child(struct ul_pty *pty)
> > +static void drain(struct ul_pty *pty)
> 
> drain_child_buffers() :-)

Renamed; updated scissor-patch below.

Best,
наб

-- >8 --
Date: Fri, 14 Jan 2022 00:28:12 +0100
Subject: [PATCH v2] Put master PTY into non-blocking mode and buffer its
 output to avoid deadlock

If we filled the script->child buffer before the child had a chance
to read any input, we'd sleep forever in write_all(pty->master),
and the child would sleep forever in write(1<pty->slave>)

By putting the master PTY in non-blocking mode, we can
poll(pty->master, POLLOUT) and keep supplying more data
as the child reads from the buffer

Fixes Debian bug #1003095
---
 include/pty-session.h |   7 +++
 lib/pty-session.c     | 126 ++++++++++++++++++++++++++++++++++--------
 2 files changed, 110 insertions(+), 23 deletions(-)

diff --git a/include/pty-session.h b/include/pty-session.h
index 09eff43fd..7176d6ba7 100644
--- a/include/pty-session.h
+++ b/include/pty-session.h
@@ -81,6 +81,13 @@ struct ul_pty {
 
 	struct timeval	next_callback_time;
 
+	struct ul_pty_child_buffer {
+		struct ul_pty_child_buffer *next;
+		char buf[BUFSIZ];
+		size_t size, cursor;
+		unsigned int final_input:1;	/* drain child before writing */
+	} *child_buffer_head, *child_buffer_tail, *free_buffers;
+
 	unsigned int isterm:1,		/* is stdin terminal? */
 		     slave_echo:1;	/* keep ECHO on pty slave */
 };
diff --git a/lib/pty-session.c b/lib/pty-session.c
index 6f038e1c5..880f08d27 100644
--- a/lib/pty-session.c
+++ b/lib/pty-session.c
@@ -69,6 +69,15 @@ struct ul_pty *ul_new_pty(int is_stdin_tty)
 
 void ul_free_pty(struct ul_pty *pty)
 {
+	struct ul_pty_child_buffer *hd;
+	while((hd = pty->child_buffer_head)) {
+		pty->child_buffer_head = hd->next;
+		free(hd);
+	}
+	while((hd = pty->free_buffers)) {
+		pty->free_buffers = hd->next;
+		free(hd);
+	}
 	free(pty);
 }
 
@@ -182,7 +191,7 @@ int ul_pty_setup(struct ul_pty *pty)
 		cfmakeraw(&attrs);
 		tcsetattr(STDIN_FILENO, TCSANOW, &attrs);
 	} else {
-	        DBG(SETUP, ul_debugobj(pty, "create for non-terminal"));
+		DBG(SETUP, ul_debugobj(pty, "create for non-terminal"));
 
 		rc = openpty(&pty->master, &pty->slave, NULL, NULL, NULL);
 		if (rc)
@@ -198,6 +207,8 @@ int ul_pty_setup(struct ul_pty *pty)
 		tcsetattr(pty->slave, TCSANOW, &attrs);
 	}
 
+	fcntl(pty->master, F_SETFL, O_NONBLOCK);
+
 	sigfillset(&ourset);
 	if (sigprocmask(SIG_BLOCK, &ourset, NULL)) {
 		rc = -errno;
@@ -290,9 +301,27 @@ static int write_output(char *obuf, ssize_t bytes)
 	return 0;
 }
 
-static int write_to_child(struct ul_pty *pty, char *buf, size_t bufsz)
+static int schedule_child_write(struct ul_pty *pty, char *buf, size_t bufsz, int final)
 {
-	return write_all(pty->master, buf, bufsz);
+	struct ul_pty_child_buffer *stash;
+	if (pty->free_buffers) {
+		stash = pty->free_buffers;
+		pty->free_buffers = stash->next;
+		memset(stash, 0, sizeof(*stash));
+	} else
+		stash = calloc(1, sizeof(*stash));
+	if (!stash)
+		return -1;
+
+	memcpy(stash->buf, buf, bufsz);
+	stash->size = bufsz;
+	stash->final_input = final ? 1 : 0;
+
+	if (pty->child_buffer_head)
+		pty->child_buffer_tail = pty->child_buffer_tail->next = stash;
+	else
+		pty->child_buffer_head = pty->child_buffer_tail = stash;
+	return 0;
 }
 
 /*
@@ -311,16 +340,13 @@ static int write_to_child(struct ul_pty *pty, char *buf, size_t bufsz)
  * maintains master+slave tty stuff within the session. Use pipe to write to
  * pty and assume non-interactive (tee-like) behavior is NOT well supported.
  */
-void ul_pty_write_eof_to_child(struct ul_pty *pty)
+static void drain_child_buffers(struct ul_pty *pty)
 {
 	unsigned int tries = 0;
-	struct pollfd fds[] = {
-	           { .fd = pty->slave, .events = POLLIN }
-	};
-	char c = DEF_EOF;
+	struct pollfd fd = { .fd = pty->slave, .events = POLLIN };
 
 	DBG(IO, ul_debugobj(pty, " waiting for empty slave"));
-	while (poll(fds, 1, 10) == 1 && tries < 8) {
+	while (poll(&fd, 1, 10) == 1 && tries < 8) {
 		DBG(IO, ul_debugobj(pty, "   slave is not empty"));
 		xusleep(250000);
 		tries++;
@@ -329,7 +355,53 @@ void ul_pty_write_eof_to_child(struct ul_pty *pty)
 		DBG(IO, ul_debugobj(pty, "   slave is empty now"));
 
 	DBG(IO, ul_debugobj(pty, " sending EOF to master"));
-	write_to_child(pty, &c, sizeof(char));
+}
+
+static int flush_child_buffers(struct ul_pty *pty, int *anything)
+{
+	int ret = 0, any = 0;
+	while (pty->child_buffer_head) {
+		struct ul_pty_child_buffer *hd = pty->child_buffer_head;
+
+		if(hd->final_input)
+			drain_child_buffers(pty);
+
+		DBG(IO, ul_debugobj(hd, " stdin --> master trying %zu bytes", hd->size - hd->cursor));
+		ssize_t ret = write(pty->master, hd->buf + hd->cursor, hd->size - hd->cursor);
+		if (ret == -1) {
+			DBG(IO, ul_debugobj(hd, "   EAGAIN"));
+			if (!(errno == EINTR || errno == EAGAIN))
+				ret = -errno;
+			goto out;
+		}
+		DBG(IO, ul_debugobj(hd, "   wrote %zd", ret));
+		any = 1;
+		hd->cursor += ret;
+		if (hd->cursor == hd->size) {
+			pty->child_buffer_head = hd->next;
+			if(!hd->next)
+				pty->child_buffer_tail = NULL;
+
+			hd->next = pty->free_buffers;
+			pty->free_buffers = hd;
+		}
+	}
+
+out:
+	/* without sync write_output() will write both input &
+	 * shell output that looks like double echoing */
+	if (any)
+		fdatasync(pty->master);
+
+	if (anything)
+		*anything = any;
+	return ret;
+}
+
+void ul_pty_write_eof_to_child(struct ul_pty *pty)
+{
+	char c = DEF_EOF;
+	schedule_child_write(pty, &c, sizeof(char), 1);
 }
 
 static int mainloop_callback(struct ul_pty *pty)
@@ -362,7 +434,7 @@ static int handle_io(struct ul_pty *pty, int fd, int *eof)
 	/* read from active FD */
 	bytes = read(fd, buf, sizeof(buf));
 	sigprocmask(SIG_BLOCK, &set, NULL);
-	if (bytes < 0) {
+	if (bytes == -1) {
 		if (errno == EAGAIN || errno == EINTR)
 			return 0;
 		return -errno;
@@ -375,15 +447,11 @@ static int handle_io(struct ul_pty *pty, int fd, int *eof)
 
 	/* from stdin (user) to command */
 	if (fd == STDIN_FILENO) {
-		DBG(IO, ul_debugobj(pty, " stdin --> master %zd bytes", bytes));
+		DBG(IO, ul_debugobj(pty, " stdin --> master %zd bytes queued", bytes));
 
-		if (write_to_child(pty, buf, bytes))
+		if (schedule_child_write(pty, buf, bytes, 0))
 			return -errno;
 
-		/* without sync write_output() will write both input &
-		 * shell output that looks like double echoing */
-		fdatasync(pty->master);
-
 	/* from command (master) to stdout */
 	} else if (fd == pty->master) {
 		DBG(IO, ul_debugobj(pty, " master --> stdout %zd bytes", bytes));
@@ -567,6 +635,11 @@ int ul_pty_proxy_master(struct ul_pty *pty)
 		} else
 			timeout = pty->poll_timeout;
 
+		if (pty->child_buffer_head)
+			pfd[POLLFD_MASTER].events |= POLLOUT;
+		else
+			pfd[POLLFD_MASTER].events &= ~POLLOUT;
+
 		/* wait for input, signal or timeout */
 		DBG(IO, ul_debugobj(pty, "calling poll() [timeout=%dms]", timeout));
 		ret = poll(pfd, ARRAY_SIZE(pfd), timeout);
@@ -600,23 +673,30 @@ int ul_pty_proxy_master(struct ul_pty *pty)
 			if (pfd[i].revents == 0)
 				continue;
 
-			DBG(IO, ul_debugobj(pty, " active pfd[%s].fd=%d %s %s %s %s",
+			DBG(IO, ul_debugobj(pty, " active pfd[%s].fd=%d %s %s %s %s %s",
 						i == POLLFD_STDIN  ? "stdin" :
 						i == POLLFD_MASTER ? "master" :
 						i == POLLFD_SIGNAL ? "signal" : "???",
 						pfd[i].fd,
 						pfd[i].revents & POLLIN  ? "POLLIN" : "",
+						pfd[i].revents & POLLOUT ? "POLLOUT" : "",
 						pfd[i].revents & POLLHUP ? "POLLHUP" : "",
 						pfd[i].revents & POLLERR ? "POLLERR" : "",
 						pfd[i].revents & POLLNVAL ? "POLLNVAL" : ""));
 
 			if (i == POLLFD_SIGNAL)
 				rc = handle_signal(pty, pfd[i].fd);
-			else if (pfd[i].revents & POLLIN)
-				rc = handle_io(pty, pfd[i].fd, &eof); /* data */
+			else {
+				if (pfd[i].revents & POLLIN)
+					rc = handle_io(pty, pfd[i].fd, &eof); /* data */
+				if (pfd[i].revents & POLLOUT) /* i == POLLFD_MASTER */
+					rc = flush_child_buffers(pty, NULL);
+			}
 
 			if (rc) {
 				ul_pty_write_eof_to_child(pty);
+				for (int anything = 1; anything;)
+					flush_child_buffers(pty, &anything);
 				break;
 			}
 
@@ -631,11 +711,11 @@ int ul_pty_proxy_master(struct ul_pty *pty)
 			 */
 			if ((pfd[i].revents & POLLHUP) || (pfd[i].revents & POLLNVAL) || eof) {
 				DBG(IO, ul_debugobj(pty, " ignore FD"));
-				pfd[i].fd = -1;
 				if (i == POLLFD_STDIN) {
+					pfd[i].fd = -1;
 					ul_pty_write_eof_to_child(pty);
-					DBG(IO, ul_debugobj(pty, "  ignore STDIN"));
-				}
+				} else /* i == POLLFD_MASTER */
+					pfd[i].revents &= ~POLLIN;
 			}
 		}
 		if (rc)
-- 
2.30.2