Skip to content

Commit 6c18b25

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 aea52d0 commit 6c18b25

File tree

1 file changed

+79
-84
lines changed

1 file changed

+79
-84
lines changed

‎libcontainer/nsenter/nsexec.c‎

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

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

341380
/* 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);
381+
xread(fd, &hdr, NLMSG_HDRLEN, "failed to read netlink header", -1, -1);
347382

348383
if (hdr.nlmsg_type == NLMSG_ERROR)
349384
bailx("failed to read netlink message");
@@ -357,11 +392,7 @@ static void nl_parse(int fd, struct nlconfig_t *config)
357392
if (!data)
358393
bail("failed to allocate %zu bytes of memory for nl_payload", size);
359394

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);
395+
xread(fd, data, size, "failed to read netlink payload", -1, -1);
365396

366397
/* Parse the netlink payload. */
367398
config->data = data;
@@ -641,16 +672,6 @@ void join_namespaces(char *nsspec)
641672
__close_namespaces(to_join, joined, ns_list, ns_len);
642673
}
643674

644-
static inline void sane_kill(pid_t pid, int signum)
645-
{
646-
if (pid <= 0)
647-
return;
648-
649-
int saved_errno = errno;
650-
kill(pid, signum);
651-
errno = saved_errno;
652-
}
653-
654675
void try_unshare(int flags, const char *msg)
655676
{
656677
write_log(DEBUG, "unshare %s", msg);
@@ -862,11 +883,8 @@ void nsexec(void)
862883
while (!stage1_complete) {
863884
enum sync_t s;
864885

865-
if (read(syncfd, &s, sizeof(s)) != sizeof(s)) {
866-
sane_kill(stage1_pid, SIGKILL);
867-
sane_kill(stage2_pid, SIGKILL);
868-
bail("failed to sync with stage-1: next state");
869-
}
886+
xread(syncfd, &s, sizeof(s),
887+
"failed to sync with stage-1: next state", stage1_pid, stage2_pid);
870888

871889
switch (s) {
872890
case SYNC_USERMAP_PLS:
@@ -890,27 +908,21 @@ void nsexec(void)
890908
update_gidmap(config.gidmappath, stage1_pid, config.gidmap, config.gidmap_len);
891909

892910
s = SYNC_USERMAP_ACK;
893-
if (write(syncfd, &s, sizeof(s)) != sizeof(s)) {
894-
sane_kill(stage1_pid, SIGKILL);
895-
bail("failed to sync with stage-1: write(SYNC_USERMAP_ACK)");
896-
}
911+
xwrite(syncfd, &s, sizeof(s),
912+
"failed to sync with stage-1: write(SYNC_USERMAP_ACK)", stage1_pid, -1);
897913
break;
898914
case SYNC_RECVPID_PLS:
899915
write_log(DEBUG, "stage-1 requested pid to be forwarded");
900916

901917
/* Get the stage-2 pid. */
902-
if (read(syncfd, &stage2_pid, sizeof(stage2_pid)) != sizeof(stage2_pid)) {
903-
sane_kill(stage1_pid, SIGKILL);
904-
bail("failed to sync with stage-1: read(stage2_pid)");
905-
}
918+
xread(syncfd, &stage2_pid, sizeof(stage2_pid),
919+
"failed to sync with stage-1: read(stage2_pid)", stage1_pid, -1);
906920

907921
/* Send ACK. */
908922
s = SYNC_RECVPID_ACK;
909-
if (write(syncfd, &s, sizeof(s)) != sizeof(s)) {
910-
sane_kill(stage1_pid, SIGKILL);
911-
sane_kill(stage2_pid, SIGKILL);
912-
bail("failed to sync with stage-1: write(SYNC_RECVPID_ACK)");
913-
}
923+
xwrite(syncfd, &s, sizeof(s),
924+
"failed to sync with stage-1: write(SYNC_RECVPID_ACK)",
925+
stage1_pid, stage2_pid);
914926

915927
/*
916928
* Send both the stage-1 and stage-2 pids back to runc.
@@ -924,20 +936,18 @@ void nsexec(void)
924936
len =
925937
dprintf(pipenum, "{\"stage1_pid\":%d,\"stage2_pid\":%d}\n", stage1_pid,
926938
stage2_pid);
927-
if (len < 0) {
928-
sane_kill(stage1_pid, SIGKILL);
929-
sane_kill(stage2_pid, SIGKILL);
930-
bail("failed to sync with runc: write(pid-JSON)");
931-
}
939+
if (len < 0)
940+
iobail(len, len,
941+
"failed to sync with runc: write(pid-JSON)",
942+
stage1_pid, stage2_pid);
932943
break;
933944
case SYNC_TIMEOFFSETS_PLS:
934945
write_log(DEBUG, "stage-1 requested timens offsets to be configured");
935946
update_timens_offsets(stage1_pid, config.timensoffset, config.timensoffset_len);
936947
s = SYNC_TIMEOFFSETS_ACK;
937-
if (write(syncfd, &s, sizeof(s)) != sizeof(s)) {
938-
sane_kill(stage1_pid, SIGKILL);
939-
bail("failed to sync with child: write(SYNC_TIMEOFFSETS_ACK)");
940-
}
948+
xwrite(syncfd, &s, sizeof(s),
949+
"failed to sync with child: write(SYNC_TIMEOFFSETS_ACK)",
950+
stage1_pid, -1);
941951
break;
942952
case SYNC_CHILD_FINISH:
943953
write_log(DEBUG, "stage-1 complete");
@@ -966,15 +976,10 @@ void nsexec(void)
966976

967977
write_log(DEBUG, "signalling stage-2 to run");
968978
s = SYNC_GRANDCHILD;
969-
if (write(syncfd, &s, sizeof(s)) != sizeof(s)) {
970-
sane_kill(stage2_pid, SIGKILL);
971-
bail("failed to sync with child: write(SYNC_GRANDCHILD)");
972-
}
979+
xwrite(syncfd, &s, sizeof(s),
980+
"failed to sync with child: write(SYNC_GRANDCHILD)", -1, stage2_pid);
973981

974-
if (read(syncfd, &s, sizeof(s)) != sizeof(s)) {
975-
sane_kill(stage2_pid, SIGKILL);
976-
bail("failed to sync with child: next state");
977-
}
982+
xread(syncfd, &s, sizeof(s), "failed to sync with child: next state", -1, stage2_pid);
978983

979984
switch (s) {
980985
case SYNC_CHILD_FINISH:
@@ -1067,13 +1072,13 @@ void nsexec(void)
10671072
*/
10681073
write_log(DEBUG, "request stage-0 to map user namespace");
10691074
s = SYNC_USERMAP_PLS;
1070-
if (write(syncfd, &s, sizeof(s)) != sizeof(s))
1071-
bail("failed to sync with parent: write(SYNC_USERMAP_PLS)");
1075+
xwrite(syncfd, &s, sizeof(s),
1076+
"failed to sync with parent: write(SYNC_USERMAP_PLS)", -1, -1);
10721077

10731078
/* ... wait for mapping ... */
10741079
write_log(DEBUG, "waiting stage-0 to complete the mapping of user namespace");
1075-
if (read(syncfd, &s, sizeof(s)) != sizeof(s))
1076-
bail("failed to sync with parent: read(SYNC_USERMAP_ACK)");
1080+
xread(syncfd, &s, sizeof(s),
1081+
"failed to sync with parent: read(SYNC_USERMAP_ACK)", -1, -1);
10771082
if (s != SYNC_USERMAP_ACK)
10781083
bailx("failed to sync with parent: SYNC_USERMAP_ACK: got %u", s);
10791084

@@ -1105,11 +1110,11 @@ void nsexec(void)
11051110
write_log(DEBUG, "request stage-0 to write timens offsets");
11061111

11071112
s = SYNC_TIMEOFFSETS_PLS;
1108-
if (write(syncfd, &s, sizeof(s)) != sizeof(s))
1109-
bail("failed to sync with parent: write(SYNC_TIMEOFFSETS_PLS)");
1113+
xwrite(syncfd, &s, sizeof(s),
1114+
"failed to sync with parent: write(SYNC_TIMEOFFSETS_PLS)", -1, -1);
11101115

1111-
if (read(syncfd, &s, sizeof(s)) != sizeof(s))
1112-
bail("failed to sync with parent: read(SYNC_TIMEOFFSETS_ACK)");
1116+
xread(syncfd, &s, sizeof(s),
1117+
"failed to sync with parent: read(SYNC_TIMEOFFSETS_ACK)", -1, -1);
11131118
if (s != SYNC_TIMEOFFSETS_ACK)
11141119
bailx("failed to sync with parent: SYNC_TIMEOFFSETS_ACK: got %u", s);
11151120
}
@@ -1131,31 +1136,23 @@ void nsexec(void)
11311136
/* Send the child to our parent, which knows what it's doing. */
11321137
write_log(DEBUG, "request stage-0 to forward stage-2 pid (%d)", stage2_pid);
11331138
s = SYNC_RECVPID_PLS;
1134-
if (write(syncfd, &s, sizeof(s)) != sizeof(s)) {
1135-
sane_kill(stage2_pid, SIGKILL);
1136-
bail("failed to sync with parent: write(SYNC_RECVPID_PLS)");
1137-
}
1138-
if (write(syncfd, &stage2_pid, sizeof(stage2_pid)) != sizeof(stage2_pid)) {
1139-
sane_kill(stage2_pid, SIGKILL);
1140-
bail("failed to sync with parent: write(stage2_pid)");
1141-
}
1139+
xwrite(syncfd, &s, sizeof(s),
1140+
"failed to sync with parent: write(SYNC_RECVPID_PLS)", -1, stage2_pid);
1141+
xwrite(syncfd, &stage2_pid, sizeof(stage2_pid),
1142+
"failed to sync with parent: write(stage2_pid)", -1, stage2_pid);
11421143

11431144
/* ... wait for parent to get the pid ... */
1144-
if (read(syncfd, &s, sizeof(s)) != sizeof(s)) {
1145-
sane_kill(stage2_pid, SIGKILL);
1146-
bail("failed to sync with parent: read(SYNC_RECVPID_ACK)");
1147-
}
1145+
xread(syncfd, &s, sizeof(s),
1146+
"failed to sync with parent: read(SYNC_RECVPID_ACK)", -1, stage2_pid);
11481147
if (s != SYNC_RECVPID_ACK) {
11491148
sane_kill(stage2_pid, SIGKILL);
11501149
bailx("failed to sync with parent: SYNC_RECVPID_ACK: got %u", s);
11511150
}
11521151

11531152
write_log(DEBUG, "signal completion to stage-0");
11541153
s = SYNC_CHILD_FINISH;
1155-
if (write(syncfd, &s, sizeof(s)) != sizeof(s)) {
1156-
sane_kill(stage2_pid, SIGKILL);
1157-
bail("failed to sync with parent: write(SYNC_CHILD_FINISH)");
1158-
}
1154+
xwrite(syncfd, &s, sizeof(s),
1155+
"failed to sync with parent: write(SYNC_CHILD_FINISH)", -1, stage2_pid);
11591156

11601157
/* Our work is done. [Stage 2: STAGE_INIT] is doing the rest of the work. */
11611158
write_log(DEBUG, "<~ nsexec stage-1");
@@ -1191,8 +1188,7 @@ void nsexec(void)
11911188
prctl(PR_SET_NAME, (unsigned long)"runc:[2:INIT]", 0, 0, 0);
11921189
write_log(DEBUG, "~> nsexec stage-2");
11931190

1194-
if (read(syncfd, &s, sizeof(s)) != sizeof(s))
1195-
bail("failed to sync with parent: read(SYNC_GRANDCHILD)");
1191+
xread(syncfd, &s, sizeof(s), "failed to sync with parent: read(SYNC_GRANDCHILD)", -1, -1);
11961192
if (s != SYNC_GRANDCHILD)
11971193
bailx("failed to sync with parent: SYNC_GRANDCHILD: got %u", s);
11981194

@@ -1212,8 +1208,7 @@ void nsexec(void)
12121208

12131209
write_log(DEBUG, "signal completion to stage-0");
12141210
s = SYNC_CHILD_FINISH;
1215-
if (write(syncfd, &s, sizeof(s)) != sizeof(s))
1216-
bail("failed to sync with parent: write(SYNC_CHILD_FINISH)");
1211+
xwrite(syncfd, &s, sizeof(s), "failed to sync with parent: write(SYNC_CHILD_FINISH)", -1, -1);
12171212

12181213
/* Close sync pipes. */
12191214
if (close(sync_grandchild_pipe[0]) < 0)

0 commit comments

Comments
 (0)