Skip to content

Commit 189c82c

Browse files
authored
IH-533: Remove usage of forkexecd daemon to execute processes (#5995)
Forkexecd was written to avoid some issues with Ocaml and multi-threading. Instead use C code to launch processes and avoid these issues. Interface remains unchanged from Ocaml side but implementation rely entirely on C code. vfork() is used to avoid performance memory issue. Reap of the processes are done directly. Code automatically reap child processes to avoid zombies. One small helper is used to better separate Ocaml and C code and handling syslog redirection. This allows to better debug in case of issues. Syslog handling is done in a separate process allowing to restart the toolstack and keep launched programs running; note that even with forkexecd daemon one process was used for this purpose. Code tries to keep compatibility with forkexecd, in particular: - SIGPIPE is ignored in the parent; - /dev/null is open with O_WRONLY even for stdin; - file descriptors are limited to 1024. We use close_range (if available) to reduce system calls to close file descriptors. Cgroup is set to avoid systemd closing processes on toolstack restart. There's a fuzzer program to check file remapping algorithm; for this reason the algorithm is in a separate file. To turn internal debug on, you need to set FORKEXECD_DEBUG_LOGS C preprocessor macro to 1.
2 parents 6d776fe + 43729fa commit 189c82c

20 files changed

+2065
-40
lines changed

Makefile

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ DUNE_IU_PACKAGES1+=xapi-client xapi-schema xapi-consts xapi-cli-protocol xapi-da
153153
DUNE_IU_PACKAGES1+=xen-api-client xen-api-client-lwt rrdd-plugin rrd-transport
154154
DUNE_IU_PACKAGES1+=gzip http-lib pciutil sexpr stunnel uuid xml-light2 zstd xapi-compression safe-resources
155155
DUNE_IU_PACKAGES1+=message-switch message-switch-cli message-switch-core message-switch-lwt
156-
DUNE_IU_PACKAGES1+=message-switch-unix xapi-idl forkexec xapi-forkexecd xapi-storage xapi-storage-script xapi-storage-cli
156+
DUNE_IU_PACKAGES1+=message-switch-unix xapi-idl xapi-forkexecd xapi-storage xapi-storage-script xapi-storage-cli
157157
DUNE_IU_PACKAGES1+=xapi-nbd varstored-guard xapi-log xapi-open-uri xapi-tracing xapi-tracing-export xapi-expiry-alerts cohttp-posix
158158
DUNE_IU_PACKAGES1+=xapi-rrd xapi-inventory clock xapi-sdk
159159
DUNE_IU_PACKAGES1+=xapi-stdext-date xapi-stdext-encodings xapi-stdext-pervasives xapi-stdext-std xapi-stdext-threads xapi-stdext-unix xapi-stdext-zerocheck xapi-tools
@@ -173,7 +173,7 @@ DUNE_IU_PACKAGES3=-j $(JOBS) --destdir=$(DESTDIR) --prefix=$(OPTDIR) --libdir=$(
173173
install-dune3:
174174
dune install $(DUNE_IU_PACKAGES3)
175175

176-
DUNE_IU_PACKAGES4=-j $(JOBS) --destdir=$(DESTDIR) --prefix=$(PREFIX) --libdir=$(LIBDIR) --libexecdir=/usr/libexec --mandir=$(MANDIR) vhd-tool
176+
DUNE_IU_PACKAGES4=-j $(JOBS) --destdir=$(DESTDIR) --prefix=$(PREFIX) --libdir=$(LIBDIR) --libexecdir=/usr/libexec --mandir=$(MANDIR) vhd-tool forkexec
177177

178178
install-dune4:
179179
dune install $(DUNE_IU_PACKAGES4)

ocaml/forkexecd/dune

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
(data_only_dirs helper)
2+
3+
(rule
4+
(deps (source_tree helper))
5+
(targets vfork_helper)
6+
(package forkexec)
7+
(action
8+
(no-infer
9+
(progn
10+
(chdir helper (run make))
11+
(copy helper/vfork_helper vfork_helper)
12+
)
13+
)
14+
)
15+
)
16+
17+
(install
18+
(package forkexec)
19+
(section libexec_root)
20+
(files (vfork_helper as xapi/vfork_helper))
21+
)

ocaml/forkexecd/helper/Makefile

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
## Set some macro but not override environment ones
2+
CFLAGS ?= -O2 -g -Wall -Werror
3+
LDFLAGS ?=
4+
5+
all:: vfork_helper
6+
7+
clean::
8+
rm -f vfork_helper *.o
9+
10+
%.o: %.c
11+
$(CC) $(CFLAGS) -MMD -MP -MF $@.d -c -o $@ $<
12+
13+
vfork_helper: vfork_helper.o close_from.o syslog.o
14+
$(CC) $(CFLAGS) $(LDFLAGS) -o $@ $^ -pthread
15+
16+
-include $(wildcard *.o.d)
17+
18+
## Fuzzer uses AFL (American Fuzzy Lop).
19+
##
20+
## Use "make fuzz" to build and launch the fuzzer
21+
##
22+
## Use "make show" to look at the first failures (if found).
23+
24+
fuzz::
25+
afl-gcc $(CFLAGS) -Wall -Werror -o algo_fuzzer algo_fuzzer.c
26+
rm -rf testcase_dir
27+
mkdir testcase_dir
28+
echo maomaoamaoaoao > testcase_dir/test1
29+
rm -rf findings_dir/
30+
afl-fuzz -i testcase_dir -o findings_dir -D -- ./algo_fuzzer
31+
32+
show::
33+
cat "$$(ls -1 findings_dir/default/crashes/id* | head -1)" | ./algo_fuzzer

ocaml/forkexecd/helper/algo_fuzzer.c

Lines changed: 246 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,246 @@
1+
2+
/*
3+
* Copyright (C) Cloud Software Group, Inc.
4+
*
5+
* This program is free software; you can redistribute it and/or modify
6+
* it under the terms of the GNU Lesser General Public License as published
7+
* by the Free Software Foundation; version 2.1 only. with the special
8+
* exception on linking described in file LICENSE.
9+
*
10+
* This program is distributed in the hope that it will be useful,
11+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
12+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
13+
* GNU Lesser General Public License for more details.
14+
*/
15+
16+
#undef NDEBUG
17+
#define DEBUG 1
18+
19+
#if DEBUG
20+
#define log(fmt, ...) printf(fmt "\n", ##__VA_ARGS__)
21+
#else
22+
#define log(fmt, ...) do {} while(0)
23+
#endif
24+
25+
// include as first file to make sure header is self contained
26+
#include "redirect_algo.h"
27+
28+
#include <stdio.h>
29+
#include <stdlib.h>
30+
#include <errno.h>
31+
#include <string.h>
32+
#include <stdint.h>
33+
#include <stdbool.h>
34+
#include <assert.h>
35+
36+
static int fake_close(int fd);
37+
38+
typedef struct {
39+
bool open;
40+
bool cloexec;
41+
char *name;
42+
} fd;
43+
44+
#define NUM_FDS 4096
45+
static fd fds[NUM_FDS];
46+
47+
static bool
48+
fake_close_fds_from(int fd_from)
49+
{
50+
for (int fd = fd_from; fd < NUM_FDS; ++fd)
51+
fake_close(fd);
52+
53+
return true;
54+
}
55+
56+
#define O_WRONLY 1
57+
static int
58+
fake_open(const char *fn, int dummy)
59+
{
60+
for (int i = 0; i < NUM_FDS; ++i)
61+
if (!fds[i].open) {
62+
assert(fds[i].name == NULL);
63+
fds[i].name = strdup(fn);
64+
fds[i].open = true;
65+
fds[i].cloexec = false;
66+
return i;
67+
}
68+
assert(0);
69+
return -1;
70+
}
71+
72+
static int
73+
fake_close(int fd)
74+
{
75+
assert(fd >= 0);
76+
assert(fd < NUM_FDS);
77+
if (!fds[fd].open) {
78+
errno = EBADF;
79+
return -1;
80+
}
81+
fds[fd].open = false;
82+
free(fds[fd].name);
83+
fds[fd].name = NULL;
84+
return 0;
85+
}
86+
87+
static int
88+
fake_dup2(int from, int to)
89+
{
90+
assert(from >= 0 && from < NUM_FDS);
91+
assert(to >= 0 && to < NUM_FDS);
92+
assert(fds[from].open);
93+
assert(from != to);
94+
free(fds[to].name);
95+
fds[to].open = true;
96+
fds[to].name = strdup(fds[from].name);
97+
fds[to].cloexec = false;
98+
return 0;
99+
}
100+
101+
static int
102+
fake_fcntl(int fd)
103+
{
104+
assert(fd >= 0 && fd < NUM_FDS);
105+
assert(fds[fd].open);
106+
fds[fd].cloexec = false;
107+
return 0;
108+
}
109+
110+
int main(int argc, char **argv)
111+
{
112+
// Input where a given FD goes??
113+
// No, not enough, can be duplicated.
114+
// Numbers >4096 in 2 bytes not file descriptor,
115+
// (-1 for standard, skip for normal).
116+
// We should add some random fds.
117+
enum { MAX_FILE_BUF = 2048 };
118+
uint16_t file_buf[MAX_FILE_BUF];
119+
size_t read = fread(file_buf, 2, MAX_FILE_BUF, stdin);
120+
if (read < 3)
121+
return 0;
122+
123+
static const char standard_names[][8] = {
124+
"stdin", "stdout", "stderr"
125+
};
126+
int num_mappings = 0;
127+
uint16_t *num = file_buf;
128+
mapping mappings[MAX_FILE_BUF];
129+
int i = 0;
130+
for (i = 0; i < 3; ++i) {
131+
mapping *m = &mappings[num_mappings++];
132+
m->uuid = standard_names[i];
133+
uint16_t n = *num++;
134+
m->current_fd = n < NUM_FDS ? n : -1;
135+
m->wanted_fd = i;
136+
}
137+
for (; i < read; ++i) {
138+
uint16_t n = *num++;
139+
if (n >= NUM_FDS)
140+
continue;
141+
142+
mapping *m = &mappings[num_mappings++];
143+
m->current_fd = n;
144+
m->wanted_fd = -1;
145+
char buf[64];
146+
sprintf(buf, "file%d", i);
147+
m->uuid = strdup(buf);
148+
}
149+
if (num_mappings > MAX_TOTAL_MAPPINGS)
150+
return 0;
151+
152+
for (unsigned n = 0; n < num_mappings; ++n) {
153+
mapping *m = &mappings[n];
154+
int fd = m->current_fd;
155+
if (fd < 0)
156+
continue;
157+
fake_close(fd);
158+
fds[fd].open = true;
159+
fds[fd].name = strdup(m->uuid);
160+
fds[fd].cloexec = true;
161+
}
162+
163+
// Check in the final file mapping all valid mappings
164+
// have an open file descriptor.
165+
// There should be no duplicate numbers in current_fd.
166+
// current_fd must be in a range.
167+
// Only if wanted_fd >= 0 current_fd can be -1.
168+
// There should be a correspondance between input and output names.
169+
// If current_fd was -1 it will still be -1.
170+
// If wanted_fd >= 0 current_fd should be the same.
171+
172+
fd_operation operations[MAX_OPERATIONS];
173+
int num_operations =
174+
redirect_mappings(mappings, num_mappings, operations);
175+
assert(num_operations > 0);
176+
assert(num_operations <= MAX_OPERATIONS);
177+
178+
for (int i = 0; i < num_operations; ++i) {
179+
const fd_operation* op = &operations[i];
180+
log("op %d %d %d", op->fd_from, op->fd_to, op->operation);
181+
switch (op->operation) {
182+
case FD_OP_DUP:
183+
if (op->fd_from == op->fd_to)
184+
fake_fcntl(op->fd_from);
185+
else
186+
fake_dup2(op->fd_from, op->fd_to);
187+
break;
188+
case FD_OP_MOVE:
189+
assert(op->fd_from != op->fd_to);
190+
fake_dup2(op->fd_from, op->fd_to);
191+
fake_close(op->fd_from);
192+
break;
193+
case FD_OP_DEVNULL:
194+
// first close old, then create new one
195+
fake_close(op->fd_to);
196+
// TODO ideally we want read only for input for Ocaml did the same...
197+
assert(fake_open("/dev/null", O_WRONLY) == op->fd_to);
198+
break;
199+
case FD_OP_CLOSE_FROM:
200+
fake_close_fds_from(op->fd_from);
201+
break;
202+
default:
203+
assert(0);
204+
}
205+
}
206+
207+
// check files opened
208+
for (int fd = 0; fd < NUM_FDS; ++fd)
209+
assert(fds[fd].open == (fd < num_mappings));
210+
211+
for (int fd = 0; fd < num_mappings; ++fd) {
212+
assert(fds[fd].cloexec == false);
213+
log("file %d %s", fd, fds[fd].name);
214+
}
215+
216+
// Check in the final file mapping all valid mappings
217+
// has an open file descriptor.
218+
bool already_found[NUM_FDS] = { false, };
219+
for (unsigned n = 0; n < num_mappings; ++n) {
220+
const int fd = mappings[n].current_fd;
221+
const int wanted = mappings[n].wanted_fd;
222+
if (fd >= 0) {
223+
assert(fd < NUM_FDS);
224+
assert(fds[fd].open);
225+
226+
// There should be no duplicate numbers in current_fd.
227+
assert(!already_found[fd]);
228+
already_found[fd] = true;
229+
} else {
230+
// Only if wanted_fd >= 0 current_fd can be -1.
231+
assert(mappings[n].wanted_fd >= 0);
232+
assert(fd == -1);
233+
}
234+
235+
// If wanted_fd >= 0 current_fd should be the same.
236+
if (wanted >= 0)
237+
assert(wanted == fd || fd == -1);
238+
239+
// current_fd must be in a range.
240+
assert(fd >= -1);
241+
assert(fd < num_mappings);
242+
}
243+
244+
// There should be a correspondance between input and output names.
245+
// If current_fd was -1 it will still be -1.
246+
}

0 commit comments

Comments
 (0)