From 5e0b09337f8d6b5f844cf68c7603fc2b2dbb6f8e Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Sat, 27 Jan 2024 20:00:16 -0600 Subject: [PATCH] lib: make ipc slightly less fragile Each process now gets an IPC handle that takes ownership of the ipc socket and tracks the queue and any associated callbacks. This gets passed around to make sure that we're always operating on the right socket and the right received data, rather than potentially crossing wires if some packets were still in the queue from a process that's since disappeared. This fixes an issue seen in the tty tests where we end up closing the socket out from underneath the next process because another process has been spawned before our current process is GC'd. Signed-off-by: Kyle Evans (cherry picked from commit c06977b113860059116a6646cde7e05a4e481ceb) --- lib/core/orch_ipc.c | 139 ++++++++++++++++++++++++------------------ lib/core/orch_lua.c | 35 ++++++----- lib/core/orch_spawn.c | 104 +++++++++++++++++++------------ lib/core/orch_tty.c | 6 +- lib/orch_lib.h | 24 +++++--- 5 files changed, 182 insertions(+), 126 deletions(-) diff --git a/lib/core/orch_ipc.c b/lib/core/orch_ipc.c index 82160da..aab12e3 100644 --- a/lib/core/orch_ipc.c +++ b/lib/core/orch_ipc.c @@ -20,70 +20,90 @@ struct orch_ipc_msgq { struct orch_ipc_msgq *next; }; -static struct orch_ipc_register { +struct orch_ipc_register { orch_ipc_handler *handler; void *cookie; -} orch_ipc_registration[IPC_LAST - 1]; +}; -static struct orch_ipc_msgq *head, *tail; -static int sockfd = -1; +struct orch_ipc { + struct orch_ipc_register *callbacks; + struct orch_ipc_msgq *head; + struct orch_ipc_msgq *tail; + int sockfd; +}; -static int orch_ipc_drain(void); -static int orch_ipc_pop(struct orch_ipc_msg **); +static int orch_ipc_drain(orch_ipc_t); +static int orch_ipc_pop(orch_ipc_t, struct orch_ipc_msg **); int -orch_ipc_close(void) +orch_ipc_close(orch_ipc_t ipc) { int error; + if (ipc == NULL) + return (0); + error = 0; - if (sockfd != -1) { - shutdown(sockfd, SHUT_WR); + if (ipc->sockfd != -1) { + shutdown(ipc->sockfd, SHUT_WR); /* - * orch_ipc_drain() should hit EOF then close the socket. + * orch_ipc_drain() should hit EOF then close the socket. This + * will just drain the socket, a follow-up orch_ipc_pop() will + * drain the read queue and invoke callbacks. */ - while (sockfd != -1 && error == 0) { - orch_ipc_wait(NULL); + while (ipc->sockfd != -1 && error == 0) { + orch_ipc_wait(ipc, NULL); - error = orch_ipc_drain(); + error = orch_ipc_drain(ipc); } + + close(ipc->sockfd); + ipc->sockfd = -1; } /* * We may have hit EOF at an inopportune time, just cope with it * and free the queue. */ - error = orch_ipc_pop(NULL); - assert(head == NULL); - tail = NULL; - for (size_t i = 0; i < IPC_LAST; i++) { - struct orch_ipc_register *reg = &orch_ipc_registration[i]; - - reg->handler = NULL; - reg->cookie = NULL; - } + error = orch_ipc_pop(ipc, NULL); + assert(ipc->head == NULL); + + free(ipc->callbacks); + free(ipc); return (error); } -void +orch_ipc_t orch_ipc_open(int fd) { + orch_ipc_t hdl; + + hdl = malloc(sizeof(*hdl)); + if (hdl == NULL) + return (NULL); + + hdl->callbacks = calloc(IPC_LAST - 1, sizeof(*hdl->callbacks)); + if (hdl->callbacks == NULL) { + free(hdl); + return (NULL); + } - assert(sockfd == -1); - sockfd = fd; + hdl->head = hdl->tail = NULL; + hdl->sockfd = fd; + return (hdl); } bool -orch_ipc_okay(void) +orch_ipc_okay(orch_ipc_t ipc) { - return (sockfd >= 0); + return (ipc->sockfd >= 0); } static int -orch_ipc_drain(void) +orch_ipc_drain(orch_ipc_t ipc) { struct orch_ipc_header hdr; struct orch_ipc_msg *msg; @@ -91,11 +111,11 @@ orch_ipc_drain(void) ssize_t readsz; size_t off, resid; - if (sockfd == -1) + if (!orch_ipc_okay(ipc)) return (0); for (;;) { - readsz = read(sockfd, &hdr, sizeof(hdr)); + readsz = read(ipc->sockfd, &hdr, sizeof(hdr)); if (readsz == -1) { if (errno == EAGAIN) break; @@ -133,7 +153,7 @@ orch_ipc_drain(void) resid = hdr.size - sizeof(hdr); while (resid != 0) { - readsz = read(sockfd, &msg->data[off], resid); + readsz = read(ipc->sockfd, &msg->data[off], resid); if (readsz == -1) { if (errno != EAGAIN) { free(msg); @@ -153,11 +173,11 @@ orch_ipc_drain(void) resid -= readsz; } - if (head == NULL) { - head = tail = msgq; + if (ipc->head == NULL) { + ipc->head = ipc->tail = msgq; } else { - tail->next = msgq; - tail = msgq; + ipc->tail->next = msgq; + ipc->tail = msgq; } msg = NULL; @@ -167,13 +187,14 @@ orch_ipc_drain(void) return (0); eof: - close(sockfd); - sockfd = -1; + close(ipc->sockfd); + ipc->sockfd = -1; + return (0); } static int -orch_ipc_pop(struct orch_ipc_msg **omsg) +orch_ipc_pop(orch_ipc_t ipc, struct orch_ipc_msg **omsg) { struct orch_ipc_register *reg; struct orch_ipc_msgq *msgq; @@ -181,10 +202,10 @@ orch_ipc_pop(struct orch_ipc_msg **omsg) int error; error = 0; - while (head != NULL) { + while (ipc->head != NULL) { /* Dequeue a msg */ - msgq = head; - head = head->next; + msgq = ipc->head; + ipc->head = ipc->head->next; /* Free the container */ msg = msgq->msg; @@ -193,11 +214,11 @@ orch_ipc_pop(struct orch_ipc_msg **omsg) msgq = NULL; /* Do we have a handler for it? */ - reg = &orch_ipc_registration[msg->hdr.tag - 1]; + reg = &ipc->callbacks[msg->hdr.tag - 1]; if (reg->handler != NULL) { int serr; - error = (*reg->handler)(msg, reg->cookie); + error = (*reg->handler)(ipc, msg, reg->cookie); if (error != 0) serr = errno; @@ -236,26 +257,26 @@ orch_ipc_pop(struct orch_ipc_msg **omsg) } int -orch_ipc_recv(struct orch_ipc_msg **omsg) +orch_ipc_recv(orch_ipc_t ipc, struct orch_ipc_msg **omsg) { struct orch_ipc_msg *rcvmsg; int error; - if (orch_ipc_drain() != 0) + if (orch_ipc_drain(ipc) != 0) return (-1); rcvmsg = NULL; - error = orch_ipc_pop(&rcvmsg); + error = orch_ipc_pop(ipc, &rcvmsg); if (error == 0) *omsg = rcvmsg; return (error); } int -orch_ipc_register(enum orch_ipc_tag tag, orch_ipc_handler *handler, - void *cookie) +orch_ipc_register(orch_ipc_t ipc, enum orch_ipc_tag tag, + orch_ipc_handler *handler, void *cookie) { - struct orch_ipc_register *reg = &orch_ipc_registration[tag - 1]; + struct orch_ipc_register *reg = &ipc->callbacks[tag - 1]; reg->handler = handler; reg->cookie = cookie; @@ -263,16 +284,16 @@ orch_ipc_register(enum orch_ipc_tag tag, orch_ipc_handler *handler, } int -orch_ipc_send(struct orch_ipc_msg *msg) +orch_ipc_send(orch_ipc_t ipc, struct orch_ipc_msg *msg) { ssize_t writesz; size_t off, resid; retry: - if (orch_ipc_drain() != 0) + if (orch_ipc_drain(ipc) != 0) return (-1); - writesz = write(sockfd, &msg->hdr, sizeof(msg->hdr)); + writesz = write(ipc->sockfd, &msg->hdr, sizeof(msg->hdr)); if (writesz == -1) { if (errno != EAGAIN) return (-1); @@ -285,7 +306,7 @@ orch_ipc_send(struct orch_ipc_msg *msg) off = 0; resid = msg->hdr.size - sizeof(msg->hdr); while (resid != 0) { - writesz = write(sockfd, &msg->data[off], resid); + writesz = write(ipc->sockfd, &msg->data[off], resid); if (writesz == -1) { if (errno != EAGAIN) return (-1); @@ -299,10 +320,8 @@ orch_ipc_send(struct orch_ipc_msg *msg) return (0); } -#include - int -orch_ipc_wait(bool *eof_seen) +orch_ipc_wait(orch_ipc_t ipc, bool *eof_seen) { fd_set rfd; int error; @@ -314,20 +333,20 @@ orch_ipc_wait(bool *eof_seen) * If we have any messages in the queue, don't bother polling; recv * will return something. */ - if (head != NULL) + if (ipc->head != NULL) return (0); FD_ZERO(&rfd); do { - if (sockfd == -1) { + if (ipc->sockfd == -1) { if (eof_seen != NULL) *eof_seen = true; return (0); } - FD_SET(sockfd, &rfd); + FD_SET(ipc->sockfd, &rfd); - error = select(sockfd + 1, &rfd, NULL, NULL, NULL); + error = select(ipc->sockfd + 1, &rfd, NULL, NULL, NULL); } while (error == -1 && errno == EINTR); return (error); diff --git a/lib/core/orch_lua.c b/lib/core/orch_lua.c index 6b7dd30..f243714 100644 --- a/lib/core/orch_lua.c +++ b/lib/core/orch_lua.c @@ -263,7 +263,8 @@ orchlua_time(lua_State *L) } static int -orchlua_child_error(struct orch_ipc_msg *msg, void *cookie) +orchlua_child_error(orch_ipc_t ipc __unused, struct orch_ipc_msg *msg, + void *cookie) { struct orch_process *proc = cookie; @@ -321,9 +322,7 @@ orchlua_spawn(lua_State *L) luaL_setmetatable(L, ORCHLUA_PROCESSHANDLE); - orch_ipc_register(IPC_ERROR, &orchlua_child_error, proc); - - if (orch_spawn(argc, argv, proc) != 0) { + if (orch_spawn(argc, argv, proc, &orchlua_child_error) != 0) { int serrno = errno; free(argv); @@ -445,9 +444,11 @@ orchlua_process_close(lua_State *L) self->pid = 0; } - orch_ipc_close(); + orch_ipc_close(self->ipc); + self->ipc = NULL; - close(self->termctl); + if (self->termctl != -1) + close(self->termctl); self->termctl = -1; if (failed) { @@ -716,8 +717,9 @@ orchlua_process_release(lua_State *L) self = luaL_checkudata(L, 1, ORCHLUA_PROCESSHANDLE); - error = orch_release(); - orch_ipc_close(); + error = orch_release(self->ipc); + orch_ipc_close(self->ipc); + self->ipc = NULL; if (error != 0) { error = errno; @@ -744,7 +746,8 @@ orchlua_process_released(lua_State *L) } static int -orchlua_process_term_set(struct orch_ipc_msg *msg, void *cookie) +orchlua_process_term_set(orch_ipc_t ipc __unused, struct orch_ipc_msg *msg, + void *cookie) { struct orch_term *term = cookie; struct termios *parent_termios = &term->term; @@ -775,7 +778,7 @@ orchlua_process_term(lua_State *L) retvals = 0; self = luaL_checkudata(L, 1, ORCHLUA_PROCESSHANDLE); - if (!orch_ipc_okay()) { + if (!orch_ipc_okay(self->ipc)) { luaL_pushfail(L); lua_pushstring(L, "process already released"); return (2); @@ -785,25 +788,27 @@ orchlua_process_term(lua_State *L) return (2); } + sterm.proc = self; sterm.initialized = false; - orch_ipc_register(IPC_TERMIOS_SET, orchlua_process_term_set, &sterm); + orch_ipc_register(self->ipc, IPC_TERMIOS_SET, orchlua_process_term_set, + &sterm); /* * The client is only responding to our messages up until we release, so * there shouldn't be anything in the queue. We'll just fire this off, * and wait for a response to become ready. */ - if ((error = orch_ipc_send(&msg)) != 0) { + if ((error = orch_ipc_send(self->ipc, &msg)) != 0) { error = errno; goto out; } - if (orch_ipc_wait(NULL) == -1) { + if (orch_ipc_wait(self->ipc, NULL) == -1) { error = errno; goto out; } - if (orch_ipc_recv(&cmsg) != 0) { + if (orch_ipc_recv(self->ipc, &cmsg) != 0) { error = errno; goto out; } @@ -825,7 +830,7 @@ orchlua_process_term(lua_State *L) out: /* Deallocate the slot */ - orch_ipc_register(IPC_TERMIOS_SET, NULL, NULL); + orch_ipc_register(self->ipc, IPC_TERMIOS_SET, NULL, NULL); if (error != 0 && retvals == 0) { luaL_pushfail(L); lua_pushstring(L, strerror(error)); diff --git a/lib/core/orch_spawn.c b/lib/core/orch_spawn.c index 715072f..3cd6520 100644 --- a/lib/core/orch_spawn.c +++ b/lib/core/orch_spawn.c @@ -6,6 +6,7 @@ #include #include +#include #include #include @@ -44,16 +45,17 @@ extern char **environ; static int orch_newpt(void); /* Child */ -static pid_t orch_newsess(void); -static void orch_usept(pid_t, int, struct termios *); -static void orch_child_error(const char *, ...) __printflike(1, 2); -static void orch_exec(int, const char *[], struct termios *t); +static pid_t orch_newsess(orch_ipc_t); +static void orch_usept(orch_ipc_t, pid_t, int, struct termios *); +static void orch_child_error(orch_ipc_t, const char *, ...) __printflike(2, 3); +static void orch_exec(orch_ipc_t, int, const char *[], struct termios *); /* Both */ -static int orch_wait(void); +static int orch_wait(orch_ipc_t); int -orch_spawn(int argc, const char *argv[], struct orch_process *p) +orch_spawn(int argc, const char *argv[], struct orch_process *p, + orch_ipc_handler *child_error_handler) { int cmdsock[2]; pid_t pid, sess; @@ -85,47 +87,70 @@ orch_spawn(int argc, const char *argv[], struct orch_process *p) err(1, "fork"); } else if (pid == 0) { struct termios t; + orch_ipc_t ipc; /* Child */ close(cmdsock[0]); - orch_ipc_open(cmdsock[1]); + ipc = orch_ipc_open(cmdsock[1]); + if (ipc == NULL) { + close(cmdsock[1]); + fprintf(stderr, "child out of memory\n"); + _exit(1); + } - sess = orch_newsess(); + sess = orch_newsess(ipc); - orch_usept(sess, p->termctl, &t); + orch_usept(ipc, sess, p->termctl, &t); close(p->termctl); p->termctl = -1; - orch_exec(argc, argv, &t); + orch_exec(ipc, argc, argv, &t); } p->released = false; p->pid = pid; - orch_ipc_open(cmdsock[0]); + p->ipc = orch_ipc_open(cmdsock[0]); /* Parent */ close(cmdsock[1]); + if (p->ipc == NULL) { + int status; + + close(p->termctl); + close(cmdsock[0]); + + kill(pid, SIGKILL); + while (waitpid(pid, &status, 0) != pid) { + continue; + } + + errno = ENOMEM; + return (-1); + } + + orch_ipc_register(p->ipc, IPC_ERROR, child_error_handler, p); + /* * Stalls until the tty is configured, completely side step races from * script writing to the tty before, e.g., echo is disabled. */ - return (orch_wait()); + return (orch_wait(p->ipc)); } static int -orch_wait(void) +orch_wait(orch_ipc_t ipc) { struct orch_ipc_msg *msg; bool stop = false; while (!stop) { - if (orch_ipc_wait(&stop) == -1) + if (orch_ipc_wait(ipc, &stop) == -1) return (-1); else if (stop) break; - if (orch_ipc_recv(&msg) != 0) + if (orch_ipc_recv(ipc, &msg) != 0) return (-1); if (msg == NULL) continue; @@ -140,18 +165,18 @@ orch_wait(void) } int -orch_release(void) +orch_release(orch_ipc_t ipc) { struct orch_ipc_msg msg; msg.hdr.size = sizeof(msg.hdr); msg.hdr.tag = IPC_RELEASE; - return (orch_ipc_send(&msg)); + return (orch_ipc_send(ipc, &msg)); } static void -orch_child_error(const char *fmt, ...) +orch_child_error(orch_ipc_t ipc, const char *fmt, ...) { struct orch_ipc_msg *errmsg; char *str, *msgstr; @@ -176,17 +201,18 @@ orch_child_error(const char *fmt, ...) free(str); str = NULL; - orch_ipc_send(errmsg); + orch_ipc_send(ipc, errmsg); out: free(errmsg); free(str); - orch_ipc_close(); + orch_ipc_close(ipc); _exit(1); } static int -orch_child_termios_inquiry(struct orch_ipc_msg *inmsg __unused, void *cookie) +orch_child_termios_inquiry(orch_ipc_t ipc, struct orch_ipc_msg *inmsg __unused, + void *cookie) { struct orch_ipc_msg *msg; struct termios *child_termios = cookie, *parent_termios; @@ -206,7 +232,7 @@ orch_child_termios_inquiry(struct orch_ipc_msg *inmsg __unused, void *cookie) parent_termios = (void *)(msg + 1); memcpy(parent_termios, child_termios, sizeof(*child_termios)); - error = orch_ipc_send(msg); + error = orch_ipc_send(ipc, msg); serr = errno; free(msg); @@ -216,7 +242,7 @@ orch_child_termios_inquiry(struct orch_ipc_msg *inmsg __unused, void *cookie) } static int -orch_child_termios_set(struct orch_ipc_msg *msg, void *cookie) +orch_child_termios_set(orch_ipc_t ipc, struct orch_ipc_msg *msg, void *cookie) { struct termios *updated_termios; struct termios *current_termios = cookie; @@ -239,13 +265,14 @@ orch_child_termios_set(struct orch_ipc_msg *msg, void *cookie) memcpy(current_termios, updated_termios, sizeof(*updated_termios)); if (tcsetattr(STDIN_FILENO, TCSANOW, current_termios) == -1) - orch_child_error("tcsetattr"); + orch_child_error(ipc, "tcsetattr"); - return (orch_ipc_send(&ack)); + return (orch_ipc_send(ipc, &ack)); } static void -orch_exec(int argc __unused, const char *argv[], struct termios *t) +orch_exec(orch_ipc_t ipc, int argc __unused, const char *argv[], + struct termios *t) { int error; @@ -256,11 +283,12 @@ orch_exec(int argc __unused, const char *argv[], struct termios *t) * - IPC_TERMIOS_INQUIRY: sent our terminal attributes back over. * - IPC_TERMIOS_SET: update our terminal attributes */ - orch_ipc_register(IPC_TERMIOS_INQUIRY, orch_child_termios_inquiry, t); - orch_ipc_register(IPC_TERMIOS_SET, orch_child_termios_set, t); + orch_ipc_register(ipc, IPC_TERMIOS_INQUIRY, orch_child_termios_inquiry, + t); + orch_ipc_register(ipc, IPC_TERMIOS_SET, orch_child_termios_set, t); /* Let the script commence. */ - if (orch_release() != 0) + if (orch_release(ipc) != 0) _exit(1); /* @@ -272,8 +300,8 @@ orch_exec(int argc __unused, const char *argv[], struct termios *t) * For now this is just a simple int, in the future it may grow a more * extensive protocol so that the script can, e.g., reconfigure the tty. */ - error = orch_wait(); - orch_ipc_close(); + error = orch_wait(ipc); + orch_ipc_close(ipc); if (error != 0) _exit(1); @@ -305,36 +333,36 @@ orch_newpt(void) } static pid_t -orch_newsess(void) +orch_newsess(orch_ipc_t ipc) { pid_t sess; sess = setsid(); if (sess == -1) - orch_child_error("setsid"); + orch_child_error(ipc, "setsid"); return (sess); } static void -orch_usept(pid_t sess, int termctl, struct termios *t) +orch_usept(orch_ipc_t ipc, pid_t sess, int termctl, struct termios *t) { const char *name; int target; name = ptsname(termctl); if (name == NULL) - orch_child_error("ptsname: %s", strerror(errno)); + orch_child_error(ipc, "ptsname: %s", strerror(errno)); target = open(name, O_RDWR); if (target == -1) - orch_child_error("open %s: %s", name, strerror(errno)); + orch_child_error(ipc, "open %s: %s", name, strerror(errno)); if (tcsetsid(target, sess) == -1) - orch_child_error("tcsetsid"); + orch_child_error(ipc, "tcsetsid"); if (tcgetattr(target, t) == -1) - orch_child_error("tcgetattr"); + orch_child_error(ipc, "tcgetattr"); /* XXX Accept mask, buffering? */ dup2(target, STDIN_FILENO); diff --git a/lib/core/orch_tty.c b/lib/core/orch_tty.c index 8b49b8d..0686df8 100644 --- a/lib/core/orch_tty.c +++ b/lib/core/orch_tty.c @@ -279,7 +279,7 @@ orchlua_term_update(lua_State *L) msg->hdr.size = msgsz; memcpy(msgterm, &self->term, sizeof(self->term)); - error = orch_ipc_send(msg); + error = orch_ipc_send(self->proc->ipc, msg); if (error != 0) error = errno; @@ -293,12 +293,12 @@ orchlua_term_update(lua_State *L) } /* Wait for ack */ - if (orch_ipc_wait(NULL) == -1) { + if (orch_ipc_wait(self->proc->ipc, NULL) == -1) { error = errno; goto err; } - if (orch_ipc_recv(&msg) != 0) { + if (orch_ipc_recv(self->proc->ipc, &msg) != 0) { error = errno; goto err; } else if (msg == NULL) { diff --git a/lib/orch_lib.h b/lib/orch_lib.h index 473f0e5..5742756 100644 --- a/lib/orch_lib.h +++ b/lib/orch_lib.h @@ -21,6 +21,8 @@ #define luaL_pushfail(L) lua_pushnil(L) #endif +typedef struct orch_ipc *orch_ipc_t; + enum orch_ipc_tag { IPC_RELEASE = 1, /* Bidrectional */ IPC_ERROR, /* Child -> Parent */ @@ -45,6 +47,7 @@ struct orch_term; struct orch_process { lua_State *L; struct orch_term *term; + orch_ipc_t ipc; int cmdsock; pid_t pid; int status; @@ -58,6 +61,7 @@ struct orch_process { struct orch_term { struct termios term; + struct orch_process *proc; bool initialized; }; @@ -78,18 +82,18 @@ struct orchlua_tty_mode { #define CNTRL_LITERAL 0x04 /* orch_ipc.c */ -typedef int (orch_ipc_handler)(struct orch_ipc_msg *, void *); -int orch_ipc_close(void); -void orch_ipc_open(int); -bool orch_ipc_okay(void); -int orch_ipc_recv(struct orch_ipc_msg **); -int orch_ipc_register(enum orch_ipc_tag, orch_ipc_handler *, void *); -int orch_ipc_send(struct orch_ipc_msg *); -int orch_ipc_wait(bool *); +typedef int (orch_ipc_handler)(orch_ipc_t, struct orch_ipc_msg *, void *); +int orch_ipc_close(orch_ipc_t); +orch_ipc_t orch_ipc_open(int); +bool orch_ipc_okay(orch_ipc_t); +int orch_ipc_recv(orch_ipc_t, struct orch_ipc_msg **); +int orch_ipc_register(orch_ipc_t, enum orch_ipc_tag, orch_ipc_handler *, void *); +int orch_ipc_send(orch_ipc_t, struct orch_ipc_msg *); +int orch_ipc_wait(orch_ipc_t, bool *); /* orch_spawn.c */ -int orch_release(void); -int orch_spawn(int, const char *[], struct orch_process *); +int orch_release(orch_ipc_t); +int orch_spawn(int, const char *[], struct orch_process *, orch_ipc_handler *); /* orch_tty.c */ int orchlua_setup_tty(lua_State *);