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
Attachment:
signature.asc
Description: PGP signature
