Skip to content

Commit 600e983

Browse files
kolyshkinkolyshkin
committed
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 <[email protected]>
1 parent 3a88f69 commit 600e983

File tree

1 file changed

+81
-87
lines changed

1 file changed

+81
-87
lines changed

‎libcontainer/nsenter/nsexec.c‎

Lines changed: 81 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -332,18 +332,55 @@ static uint8_t readint8(char *buf)
332332
return *(uint8_t *) buf;
333333
}
334334

335+
static inline int sane_kill(pid_t pid, int signum)
336+
{
337+
if (pid > 0) {
338+
int ret, saved_errno;
339+
340+
saved_errno = errno;
341+
ret = kill(pid, signum);
342+
errno = saved_errno;
343+
return ret;
344+
} else
345+
return 0;
346+
}
347+
348+
static void __attribute__((noreturn))iobail(int got, int want, const char *errmsg, int pid1, int pid2)
349+
{
350+
sane_kill(pid1, SIGKILL);
351+
sane_kill(pid2, SIGKILL);
352+
if (got < 0)
353+
bail("%s", errmsg);
354+
/* Short read or write. */
355+
bailx("%s (got %d of %d bytes)", errmsg, got, want);
356+
}
357+
358+
static void xread(int fd, void *buf, size_t nbytes, const char *errmsg, int pid1, int pid2)
359+
{
360+
ssize_t len;
361+
362+
len = read(fd, buf, nbytes);
363+
if (len != nbytes)
364+
iobail(len, nbytes, errmsg, pid1, pid2);
365+
}
366+
367+
static void xwrite(int fd, void *buf, size_t nbytes, const char *errmsg, int pid1, int pid2)
368+
{
369+
ssize_t len;
370+
371+
len = write(fd, buf, nbytes);
372+
if (len != nbytes)
373+
iobail(len, nbytes, errmsg, pid1, pid2);
374+
}
375+
335376
static void nl_parse(int fd, struct nlconfig_t *config)
336377
{
337-
size_t len, size;
378+
size_t size;
338379
struct nlmsghdr hdr;
339380
char *data, *current;
340381

341382
/* Retrieve the netlink header. */
342-
len = read(fd, &hdr, NLMSG_HDRLEN);
343-
if (len < 0)
344-
bail("failed to read netlink header");
345-
if (len != NLMSG_HDRLEN)
346-
bailx("invalid netlink header length %zu", len);
383+
xread(fd, &hdr, NLMSG_HDRLEN, "failed to read netlink header", -1, -1);
347384

348385
if (hdr.nlmsg_type == NLMSG_ERROR)
349386
bailx("failed to read netlink message");
@@ -357,11 +394,7 @@ static void nl_parse(int fd, struct nlconfig_t *config)
357394
if (!data)
358395
bail("failed to allocate %zu bytes of memory for nl_payload", size);
359396

360-
len = read(fd, data, size);
361-
if (len < 0)
362-
bail("failed to read netlink payload");
363-
if (len != size)
364-
bailx("failed to read netlink payload, %zu != %zu", len, size);
397+
xread(fd, data, size, "failed to read netlink payload", -1, -1);
365398

366399
/* Parse the netlink payload. */
367400
config->data = data;
@@ -641,19 +674,6 @@ void join_namespaces(char *nsspec)
641674
__close_namespaces(to_join, joined, ns_list, ns_len);
642675
}
643676

644-
static inline int sane_kill(pid_t pid, int signum)
645-
{
646-
if (pid > 0) {
647-
int ret, saved_errno;
648-
649-
saved_errno = errno;
650-
ret = kill(pid, signum);
651-
errno = saved_errno;
652-
return ret;
653-
} else
654-
return 0;
655-
}
656-
657677
void try_unshare(int flags, const char *msg)
658678
{
659679
write_log(DEBUG, "unshare %s", msg);
@@ -865,11 +885,8 @@ void nsexec(void)
865885
while (!stage1_complete) {
866886
enum sync_t s;
867887

868-
if (read(syncfd, &s, sizeof(s)) != sizeof(s)) {
869-
sane_kill(stage1_pid, SIGKILL);
870-
sane_kill(stage2_pid, SIGKILL);
871-
bail("failed to sync with stage-1: next state");
872-
}
888+
xread(syncfd, &s, sizeof(s),
889+
"failed to sync with stage-1: next state", stage1_pid, stage2_pid);
873890

874891
switch (s) {
875892
case SYNC_USERMAP_PLS:
@@ -893,27 +910,21 @@ void nsexec(void)
893910
update_gidmap(config.gidmappath, stage1_pid, config.gidmap, config.gidmap_len);
894911

895912
s = SYNC_USERMAP_ACK;
896-
if (write(syncfd, &s, sizeof(s)) != sizeof(s)) {
897-
sane_kill(stage1_pid, SIGKILL);
898-
bail("failed to sync with stage-1: write(SYNC_USERMAP_ACK)");
899-
}
913+
xwrite(syncfd, &s, sizeof(s),
914+
"failed to sync with stage-1: write(SYNC_USERMAP_ACK)", stage1_pid, -1);
900915
break;
901916
case SYNC_RECVPID_PLS:
902917
write_log(DEBUG, "stage-1 requested pid to be forwarded");
903918

904919
/* Get the stage-2 pid. */
905-
if (read(syncfd, &stage2_pid, sizeof(stage2_pid)) != sizeof(stage2_pid)) {
906-
sane_kill(stage1_pid, SIGKILL);
907-
bail("failed to sync with stage-1: read(stage2_pid)");
908-
}
920+
xread(syncfd, &stage2_pid, sizeof(stage2_pid),
921+
"failed to sync with stage-1: read(stage2_pid)", stage1_pid, -1);
909922

910923
/* Send ACK. */
911924
s = SYNC_RECVPID_ACK;
912-
if (write(syncfd, &s, sizeof(s)) != sizeof(s)) {
913-
sane_kill(stage1_pid, SIGKILL);
914-
sane_kill(stage2_pid, SIGKILL);
915-
bail("failed to sync with stage-1: write(SYNC_RECVPID_ACK)");
916-
}
925+
xwrite(syncfd, &s, sizeof(s),
926+
"failed to sync with stage-1: write(SYNC_RECVPID_ACK)",
927+
stage1_pid, stage2_pid);
917928

918929
/*
919930
* Send both the stage-1 and stage-2 pids back to runc.
@@ -927,20 +938,18 @@ void nsexec(void)
927938
len =
928939
dprintf(pipenum, "{\"stage1_pid\":%d,\"stage2_pid\":%d}\n", stage1_pid,
929940
stage2_pid);
930-
if (len < 0) {
931-
sane_kill(stage1_pid, SIGKILL);
932-
sane_kill(stage2_pid, SIGKILL);
933-
bail("failed to sync with runc: write(pid-JSON)");
934-
}
941+
if (len < 0)
942+
iobail(len, len,
943+
"failed to sync with runc: write(pid-JSON)",
944+
stage1_pid, stage2_pid);
935945
break;
936946
case SYNC_TIMEOFFSETS_PLS:
937947
write_log(DEBUG, "stage-1 requested timens offsets to be configured");
938948
update_timens_offsets(stage1_pid, config.timensoffset, config.timensoffset_len);
939949
s = SYNC_TIMEOFFSETS_ACK;
940-
if (write(syncfd, &s, sizeof(s)) != sizeof(s)) {
941-
sane_kill(stage1_pid, SIGKILL);
942-
bail("failed to sync with child: write(SYNC_TIMEOFFSETS_ACK)");
943-
}
950+
xwrite(syncfd, &s, sizeof(s),
951+
"failed to sync with child: write(SYNC_TIMEOFFSETS_ACK)",
952+
stage1_pid, -1);
944953
break;
945954
case SYNC_CHILD_FINISH:
946955
write_log(DEBUG, "stage-1 complete");
@@ -969,15 +978,10 @@ void nsexec(void)
969978

970979
write_log(DEBUG, "signalling stage-2 to run");
971980
s = SYNC_GRANDCHILD;
972-
if (write(syncfd, &s, sizeof(s)) != sizeof(s)) {
973-
sane_kill(stage2_pid, SIGKILL);
974-
bail("failed to sync with child: write(SYNC_GRANDCHILD)");
975-
}
981+
xwrite(syncfd, &s, sizeof(s),
982+
"failed to sync with child: write(SYNC_GRANDCHILD)", -1, stage2_pid);
976983

977-
if (read(syncfd, &s, sizeof(s)) != sizeof(s)) {
978-
sane_kill(stage2_pid, SIGKILL);
979-
bail("failed to sync with child: next state");
980-
}
984+
xread(syncfd, &s, sizeof(s), "failed to sync with child: next state", -1, stage2_pid);
981985

982986
switch (s) {
983987
case SYNC_CHILD_FINISH:
@@ -1070,13 +1074,13 @@ void nsexec(void)
10701074
*/
10711075
write_log(DEBUG, "request stage-0 to map user namespace");
10721076
s = SYNC_USERMAP_PLS;
1073-
if (write(syncfd, &s, sizeof(s)) != sizeof(s))
1074-
bail("failed to sync with parent: write(SYNC_USERMAP_PLS)");
1077+
xwrite(syncfd, &s, sizeof(s),
1078+
"failed to sync with parent: write(SYNC_USERMAP_PLS)", -1, -1);
10751079

10761080
/* ... wait for mapping ... */
10771081
write_log(DEBUG, "waiting stage-0 to complete the mapping of user namespace");
1078-
if (read(syncfd, &s, sizeof(s)) != sizeof(s))
1079-
bail("failed to sync with parent: read(SYNC_USERMAP_ACK)");
1082+
xread(syncfd, &s, sizeof(s),
1083+
"failed to sync with parent: read(SYNC_USERMAP_ACK)", -1, -1);
10801084
if (s != SYNC_USERMAP_ACK)
10811085
bailx("failed to sync with parent: SYNC_USERMAP_ACK: got %u", s);
10821086

@@ -1108,11 +1112,11 @@ void nsexec(void)
11081112
write_log(DEBUG, "request stage-0 to write timens offsets");
11091113

11101114
s = SYNC_TIMEOFFSETS_PLS;
1111-
if (write(syncfd, &s, sizeof(s)) != sizeof(s))
1112-
bail("failed to sync with parent: write(SYNC_TIMEOFFSETS_PLS)");
1115+
xwrite(syncfd, &s, sizeof(s),
1116+
"failed to sync with parent: write(SYNC_TIMEOFFSETS_PLS)", -1, -1);
11131117

1114-
if (read(syncfd, &s, sizeof(s)) != sizeof(s))
1115-
bail("failed to sync with parent: read(SYNC_TIMEOFFSETS_ACK)");
1118+
xread(syncfd, &s, sizeof(s),
1119+
"failed to sync with parent: read(SYNC_TIMEOFFSETS_ACK)", -1, -1);
11161120
if (s != SYNC_TIMEOFFSETS_ACK)
11171121
bailx("failed to sync with parent: SYNC_TIMEOFFSETS_ACK: got %u", s);
11181122
}
@@ -1134,31 +1138,23 @@ void nsexec(void)
11341138
/* Send the child to our parent, which knows what it's doing. */
11351139
write_log(DEBUG, "request stage-0 to forward stage-2 pid (%d)", stage2_pid);
11361140
s = SYNC_RECVPID_PLS;
1137-
if (write(syncfd, &s, sizeof(s)) != sizeof(s)) {
1138-
sane_kill(stage2_pid, SIGKILL);
1139-
bail("failed to sync with parent: write(SYNC_RECVPID_PLS)");
1140-
}
1141-
if (write(syncfd, &stage2_pid, sizeof(stage2_pid)) != sizeof(stage2_pid)) {
1142-
sane_kill(stage2_pid, SIGKILL);
1143-
bail("failed to sync with parent: write(stage2_pid)");
1144-
}
1141+
xwrite(syncfd, &s, sizeof(s),
1142+
"failed to sync with parent: write(SYNC_RECVPID_PLS)", -1, stage2_pid);
1143+
xwrite(syncfd, &stage2_pid, sizeof(stage2_pid),
1144+
"failed to sync with parent: write(stage2_pid)", -1, stage2_pid);
11451145

11461146
/* ... wait for parent to get the pid ... */
1147-
if (read(syncfd, &s, sizeof(s)) != sizeof(s)) {
1148-
sane_kill(stage2_pid, SIGKILL);
1149-
bail("failed to sync with parent: read(SYNC_RECVPID_ACK)");
1150-
}
1147+
xread(syncfd, &s, sizeof(s),
1148+
"failed to sync with parent: read(SYNC_RECVPID_ACK)", -1, stage2_pid);
11511149
if (s != SYNC_RECVPID_ACK) {
11521150
sane_kill(stage2_pid, SIGKILL);
11531151
bailx("failed to sync with parent: SYNC_RECVPID_ACK: got %u", s);
11541152
}
11551153

11561154
write_log(DEBUG, "signal completion to stage-0");
11571155
s = SYNC_CHILD_FINISH;
1158-
if (write(syncfd, &s, sizeof(s)) != sizeof(s)) {
1159-
sane_kill(stage2_pid, SIGKILL);
1160-
bail("failed to sync with parent: write(SYNC_CHILD_FINISH)");
1161-
}
1156+
xwrite(syncfd, &s, sizeof(s),
1157+
"failed to sync with parent: write(SYNC_CHILD_FINISH)", -1, stage2_pid);
11621158

11631159
/* Our work is done. [Stage 2: STAGE_INIT] is doing the rest of the work. */
11641160
write_log(DEBUG, "<~ nsexec stage-1");
@@ -1194,8 +1190,7 @@ void nsexec(void)
11941190
prctl(PR_SET_NAME, (unsigned long)"runc:[2:INIT]", 0, 0, 0);
11951191
write_log(DEBUG, "~> nsexec stage-2");
11961192

1197-
if (read(syncfd, &s, sizeof(s)) != sizeof(s))
1198-
bail("failed to sync with parent: read(SYNC_GRANDCHILD)");
1193+
xread(syncfd, &s, sizeof(s), "failed to sync with parent: read(SYNC_GRANDCHILD)", -1, -1);
11991194
if (s != SYNC_GRANDCHILD)
12001195
bailx("failed to sync with parent: SYNC_GRANDCHILD: got %u", s);
12011196

@@ -1215,8 +1210,7 @@ void nsexec(void)
12151210

12161211
write_log(DEBUG, "signal completion to stage-0");
12171212
s = SYNC_CHILD_FINISH;
1218-
if (write(syncfd, &s, sizeof(s)) != sizeof(s))
1219-
bail("failed to sync with parent: write(SYNC_CHILD_FINISH)");
1213+
xwrite(syncfd, &s, sizeof(s), "failed to sync with parent: write(SYNC_CHILD_FINISH)", -1, -1);
12201214

12211215
/* Close sync pipes. */
12221216
if (close(sync_grandchild_pipe[0]) < 0)

0 commit comments

Comments
 (0)