From 46156624b77fd995e6fc45df097aa94a6b8be5c2 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Sat, 11 Oct 2025 19:07:25 -0700 Subject: [PATCH] libct/nsenter: better read/write errors Introduce and use iobail, xread, and xwrite wrappers so that we can properly check read/write return value and call either bail or bailx on error, with proper diagnostics (distinguishing failed read/write from a short read/write). This prevents the "Success" prefix in errors like: failed to sync with stage-1: next state: Success Signed-off-by: Kir Kolyshkin (cherry picked from commit 6c18b25cdcf170f7a2020e1725926f51e55752f8) Signed-off-by: Aleksa Sarai --- libcontainer/nsenter/nsexec.c | 163 ++++++++++++++++------------------ 1 file changed, 79 insertions(+), 84 deletions(-) diff --git a/libcontainer/nsenter/nsexec.c b/libcontainer/nsenter/nsexec.c index 8283ebb4b..59b785109 100644 --- a/libcontainer/nsenter/nsexec.c +++ b/libcontainer/nsenter/nsexec.c @@ -332,18 +332,53 @@ static uint8_t readint8(char *buf) return *(uint8_t *) buf; } +static inline void sane_kill(pid_t pid, int signum) +{ + if (pid <= 0) + return; + + int saved_errno = errno; + kill(pid, signum); + errno = saved_errno; +} + +__attribute__((noreturn)) +static void iobail(int got, int want, const char *errmsg, int pid1, int pid2) +{ + sane_kill(pid1, SIGKILL); + sane_kill(pid2, SIGKILL); + if (got < 0) + bail("%s", errmsg); + /* Short read or write. */ + bailx("%s (got %d of %d bytes)", errmsg, got, want); +} + +static void xread(int fd, void *buf, size_t nbytes, const char *errmsg, int pid1, int pid2) +{ + ssize_t len; + + len = read(fd, buf, nbytes); + if (len != nbytes) + iobail(len, nbytes, errmsg, pid1, pid2); +} + +static void xwrite(int fd, void *buf, size_t nbytes, const char *errmsg, int pid1, int pid2) +{ + ssize_t len; + + len = write(fd, buf, nbytes); + if (len != nbytes) + iobail(len, nbytes, errmsg, pid1, pid2); +} + static void nl_parse(int fd, struct nlconfig_t *config) { - size_t len, size; + size_t size; struct nlmsghdr hdr; char *data, *current; /* Retrieve the netlink header. */ - len = read(fd, &hdr, NLMSG_HDRLEN); - if (len < 0) - bail("failed to read netlink header"); - if (len != NLMSG_HDRLEN) - bailx("invalid netlink header length %zu", len); + xread(fd, &hdr, NLMSG_HDRLEN, "failed to read netlink header", -1, -1); if (hdr.nlmsg_type == NLMSG_ERROR) bailx("failed to read netlink message"); @@ -357,11 +392,7 @@ static void nl_parse(int fd, struct nlconfig_t *config) if (!data) bail("failed to allocate %zu bytes of memory for nl_payload", size); - len = read(fd, data, size); - if (len < 0) - bail("failed to read netlink payload"); - if (len != size) - bailx("failed to read netlink payload, %zu != %zu", len, size); + xread(fd, data, size, "failed to read netlink payload", -1, -1); /* Parse the netlink payload. */ config->data = data; @@ -641,16 +672,6 @@ void join_namespaces(char *nsspec) __close_namespaces(to_join, joined, ns_list, ns_len); } -static inline void sane_kill(pid_t pid, int signum) -{ - if (pid <= 0) - return; - - int saved_errno = errno; - kill(pid, signum); - errno = saved_errno; -} - void try_unshare(int flags, const char *msg) { write_log(DEBUG, "unshare %s", msg); @@ -862,11 +883,8 @@ void nsexec(void) while (!stage1_complete) { enum sync_t s; - if (read(syncfd, &s, sizeof(s)) != sizeof(s)) { - sane_kill(stage1_pid, SIGKILL); - sane_kill(stage2_pid, SIGKILL); - bail("failed to sync with stage-1: next state"); - } + xread(syncfd, &s, sizeof(s), + "failed to sync with stage-1: next state", stage1_pid, stage2_pid); switch (s) { case SYNC_USERMAP_PLS: @@ -890,27 +908,21 @@ void nsexec(void) update_gidmap(config.gidmappath, stage1_pid, config.gidmap, config.gidmap_len); s = SYNC_USERMAP_ACK; - if (write(syncfd, &s, sizeof(s)) != sizeof(s)) { - sane_kill(stage1_pid, SIGKILL); - bail("failed to sync with stage-1: write(SYNC_USERMAP_ACK)"); - } + xwrite(syncfd, &s, sizeof(s), + "failed to sync with stage-1: write(SYNC_USERMAP_ACK)", stage1_pid, -1); break; case SYNC_RECVPID_PLS: write_log(DEBUG, "stage-1 requested pid to be forwarded"); /* Get the stage-2 pid. */ - if (read(syncfd, &stage2_pid, sizeof(stage2_pid)) != sizeof(stage2_pid)) { - sane_kill(stage1_pid, SIGKILL); - bail("failed to sync with stage-1: read(stage2_pid)"); - } + xread(syncfd, &stage2_pid, sizeof(stage2_pid), + "failed to sync with stage-1: read(stage2_pid)", stage1_pid, -1); /* Send ACK. */ s = SYNC_RECVPID_ACK; - if (write(syncfd, &s, sizeof(s)) != sizeof(s)) { - sane_kill(stage1_pid, SIGKILL); - sane_kill(stage2_pid, SIGKILL); - bail("failed to sync with stage-1: write(SYNC_RECVPID_ACK)"); - } + xwrite(syncfd, &s, sizeof(s), + "failed to sync with stage-1: write(SYNC_RECVPID_ACK)", + stage1_pid, stage2_pid); /* * Send both the stage-1 and stage-2 pids back to runc. @@ -924,20 +936,18 @@ void nsexec(void) len = dprintf(pipenum, "{\"stage1_pid\":%d,\"stage2_pid\":%d}\n", stage1_pid, stage2_pid); - if (len < 0) { - sane_kill(stage1_pid, SIGKILL); - sane_kill(stage2_pid, SIGKILL); - bail("failed to sync with runc: write(pid-JSON)"); - } + if (len < 0) + iobail(len, len, + "failed to sync with runc: write(pid-JSON)", + stage1_pid, stage2_pid); break; case SYNC_TIMEOFFSETS_PLS: write_log(DEBUG, "stage-1 requested timens offsets to be configured"); update_timens_offsets(stage1_pid, config.timensoffset, config.timensoffset_len); s = SYNC_TIMEOFFSETS_ACK; - if (write(syncfd, &s, sizeof(s)) != sizeof(s)) { - sane_kill(stage1_pid, SIGKILL); - bail("failed to sync with child: write(SYNC_TIMEOFFSETS_ACK)"); - } + xwrite(syncfd, &s, sizeof(s), + "failed to sync with child: write(SYNC_TIMEOFFSETS_ACK)", + stage1_pid, -1); break; case SYNC_CHILD_FINISH: write_log(DEBUG, "stage-1 complete"); @@ -966,15 +976,10 @@ void nsexec(void) write_log(DEBUG, "signalling stage-2 to run"); s = SYNC_GRANDCHILD; - if (write(syncfd, &s, sizeof(s)) != sizeof(s)) { - sane_kill(stage2_pid, SIGKILL); - bail("failed to sync with child: write(SYNC_GRANDCHILD)"); - } + xwrite(syncfd, &s, sizeof(s), + "failed to sync with child: write(SYNC_GRANDCHILD)", -1, stage2_pid); - if (read(syncfd, &s, sizeof(s)) != sizeof(s)) { - sane_kill(stage2_pid, SIGKILL); - bail("failed to sync with child: next state"); - } + xread(syncfd, &s, sizeof(s), "failed to sync with child: next state", -1, stage2_pid); switch (s) { case SYNC_CHILD_FINISH: @@ -1067,13 +1072,13 @@ void nsexec(void) */ write_log(DEBUG, "request stage-0 to map user namespace"); s = SYNC_USERMAP_PLS; - if (write(syncfd, &s, sizeof(s)) != sizeof(s)) - bail("failed to sync with parent: write(SYNC_USERMAP_PLS)"); + xwrite(syncfd, &s, sizeof(s), + "failed to sync with parent: write(SYNC_USERMAP_PLS)", -1, -1); /* ... wait for mapping ... */ write_log(DEBUG, "waiting stage-0 to complete the mapping of user namespace"); - if (read(syncfd, &s, sizeof(s)) != sizeof(s)) - bail("failed to sync with parent: read(SYNC_USERMAP_ACK)"); + xread(syncfd, &s, sizeof(s), + "failed to sync with parent: read(SYNC_USERMAP_ACK)", -1, -1); if (s != SYNC_USERMAP_ACK) bailx("failed to sync with parent: SYNC_USERMAP_ACK: got %u", s); @@ -1105,11 +1110,11 @@ void nsexec(void) write_log(DEBUG, "request stage-0 to write timens offsets"); s = SYNC_TIMEOFFSETS_PLS; - if (write(syncfd, &s, sizeof(s)) != sizeof(s)) - bail("failed to sync with parent: write(SYNC_TIMEOFFSETS_PLS)"); + xwrite(syncfd, &s, sizeof(s), + "failed to sync with parent: write(SYNC_TIMEOFFSETS_PLS)", -1, -1); - if (read(syncfd, &s, sizeof(s)) != sizeof(s)) - bail("failed to sync with parent: read(SYNC_TIMEOFFSETS_ACK)"); + xread(syncfd, &s, sizeof(s), + "failed to sync with parent: read(SYNC_TIMEOFFSETS_ACK)", -1, -1); if (s != SYNC_TIMEOFFSETS_ACK) bailx("failed to sync with parent: SYNC_TIMEOFFSETS_ACK: got %u", s); } @@ -1131,20 +1136,14 @@ void nsexec(void) /* Send the child to our parent, which knows what it's doing. */ write_log(DEBUG, "request stage-0 to forward stage-2 pid (%d)", stage2_pid); s = SYNC_RECVPID_PLS; - if (write(syncfd, &s, sizeof(s)) != sizeof(s)) { - sane_kill(stage2_pid, SIGKILL); - bail("failed to sync with parent: write(SYNC_RECVPID_PLS)"); - } - if (write(syncfd, &stage2_pid, sizeof(stage2_pid)) != sizeof(stage2_pid)) { - sane_kill(stage2_pid, SIGKILL); - bail("failed to sync with parent: write(stage2_pid)"); - } + xwrite(syncfd, &s, sizeof(s), + "failed to sync with parent: write(SYNC_RECVPID_PLS)", -1, stage2_pid); + xwrite(syncfd, &stage2_pid, sizeof(stage2_pid), + "failed to sync with parent: write(stage2_pid)", -1, stage2_pid); /* ... wait for parent to get the pid ... */ - if (read(syncfd, &s, sizeof(s)) != sizeof(s)) { - sane_kill(stage2_pid, SIGKILL); - bail("failed to sync with parent: read(SYNC_RECVPID_ACK)"); - } + xread(syncfd, &s, sizeof(s), + "failed to sync with parent: read(SYNC_RECVPID_ACK)", -1, stage2_pid); if (s != SYNC_RECVPID_ACK) { sane_kill(stage2_pid, SIGKILL); bailx("failed to sync with parent: SYNC_RECVPID_ACK: got %u", s); @@ -1152,10 +1151,8 @@ void nsexec(void) write_log(DEBUG, "signal completion to stage-0"); s = SYNC_CHILD_FINISH; - if (write(syncfd, &s, sizeof(s)) != sizeof(s)) { - sane_kill(stage2_pid, SIGKILL); - bail("failed to sync with parent: write(SYNC_CHILD_FINISH)"); - } + xwrite(syncfd, &s, sizeof(s), + "failed to sync with parent: write(SYNC_CHILD_FINISH)", -1, stage2_pid); /* Our work is done. [Stage 2: STAGE_INIT] is doing the rest of the work. */ write_log(DEBUG, "<~ nsexec stage-1"); @@ -1191,8 +1188,7 @@ void nsexec(void) prctl(PR_SET_NAME, (unsigned long)"runc:[2:INIT]", 0, 0, 0); write_log(DEBUG, "~> nsexec stage-2"); - if (read(syncfd, &s, sizeof(s)) != sizeof(s)) - bail("failed to sync with parent: read(SYNC_GRANDCHILD)"); + xread(syncfd, &s, sizeof(s), "failed to sync with parent: read(SYNC_GRANDCHILD)", -1, -1); if (s != SYNC_GRANDCHILD) bailx("failed to sync with parent: SYNC_GRANDCHILD: got %u", s); @@ -1212,8 +1208,7 @@ void nsexec(void) write_log(DEBUG, "signal completion to stage-0"); s = SYNC_CHILD_FINISH; - if (write(syncfd, &s, sizeof(s)) != sizeof(s)) - bail("failed to sync with parent: write(SYNC_CHILD_FINISH)"); + xwrite(syncfd, &s, sizeof(s), "failed to sync with parent: write(SYNC_CHILD_FINISH)", -1, -1); /* Close sync pipes. */ if (close(sync_grandchild_pipe[0]) < 0)