Skip to content

Commit a3bbcb5

Browse files
DemiMarieDemiMarie
committed
Prefer close() to shutdown()
There is no need for stdin_fd and stdout_fd to be distinct file descriptors, so long as the code is careful to not close a file descriptor that is still in use. This extra care allows for close() to be used in preference to shutdown(), unless the same socket is being used for both stdin and stdout. close() has the advantage that it does not interfere with other users of the same file description, which allows the "was this inherited from another process?" hack to be eliminated. Instead, use the file descriptor number to determine whether blocking mode needs to be restored - it only needs to be restored for inherited file descriptors. To avoid lots of error-prone boilerplate, use a single function to close stdin_fd or stdout_fd, and wrap it with macros that select the right one to use. These macros also pass the correct argument for shutdown(2) and set the just-closed FD to -1, eliminating a nasty potential source of bugs. The function automatically chooses between close(2) and shutdown(2) based on whether the two file descriptor arguments it is passed are identical, ensuring that shutdown(2) is only used if needed. Furthermore, the function knows (based on the file descriptor number) if the file descriptor was inherited from another process, and therefore whether it needs to be restored to blocking mode. This also avoids making a file descriptor blocking while still in use. Since the code now knows if stdin and stdout use the same file descriptor, it can avoid setting the blocking flag on a file descriptor that is still in use, which could cause a deadlock. This works in every case except where both stdin and stdout are passed from a parent process and share a file description. git blame points to commit 0802df7 ("Factor out process_child_io"), but I suspect the actual bug dates back as far as the introduction of socket-based services in 76697e3 ("Working socket-based qrexec").
1 parent bc71817 commit a3bbcb5

File tree

1 file changed

+34
-71
lines changed

1 file changed

+34
-71
lines changed

‎libqrexec/process_io.c

+34-71
Original file line numberDiff line numberDiff line change
@@ -40,45 +40,24 @@ static _Noreturn void handle_vchan_error(const char *op)
4040
/*
4141
* Closing the file descriptors:
4242
*
43-
* - Use shutdown(), except if this is a FD inherited from parent (detected by
44-
* FD number)
45-
* - Restore blocking status, unless this is a duplicated stdio socket for
46-
* stdout/stderr (in which case we can't restore it just for one FD, but we
47-
* don't care, because we created it)
43+
* - If this is a single socket used for both stdin and stdout, call shutdown()
44+
* to indicate that no more data will be sent or received.
45+
* - Otherwise, restore the blocking status of the FD.
4846
*/
49-
50-
static void close_stdin(int fd, bool restore_block) {
47+
static void close_stdio(int fd, int other_fd, int direction) {
5148
if (fd == -1)
5249
return;
53-
54-
if (restore_block)
55-
set_block(fd);
56-
57-
if (fd == 1) {
58-
close(fd);
59-
} else if (shutdown(fd, SHUT_WR) == -1) {
60-
if (errno == ENOTSOCK)
61-
close(fd);
62-
else
63-
PERROR("shutdown close_stdin");
64-
}
65-
}
66-
67-
static void close_stdout(int fd, bool restore_block) {
68-
if (fd == -1)
50+
if (fd == other_fd) {
51+
/* Do not close the FD, as it is still in use. */
52+
/* ENOTCONN can happen with TCP and is harmless */
53+
if (shutdown(fd, direction) == -1 && errno != ENOTSOCK && errno != ENOTCONN)
54+
PERROR("shutdown");
6955
return;
56+
}
7057

71-
if (restore_block)
58+
if (fd < 3)
7259
set_block(fd);
73-
74-
if (fd == 0) {
75-
close(fd);
76-
} else if (shutdown(fd, SHUT_RD) == -1) {
77-
if (errno == ENOTSOCK)
78-
close(fd);
79-
else if (errno != ENOTCONN) /* can happen with TCP, harmless */
80-
PERROR("shutdown close_stdout");
81-
}
60+
close(fd);
8261
}
8362

8463
static void close_stderr(int fd) {
@@ -116,8 +95,6 @@ int process_io(const struct process_io_request *req) {
11695
pid_t local_status = -1;
11796
pid_t remote_status = -1;
11897
int stdout_msg_type = is_service ? MSG_DATA_STDOUT : MSG_DATA_STDIN;
119-
bool use_stdio_socket = false;
120-
12198
int ret;
12299
struct pollfd fds[FD_NUM];
123100
sigset_t pollmask;
@@ -133,11 +110,20 @@ int process_io(const struct process_io_request *req) {
133110
set_nonblock(stdin_fd);
134111
if (stdout_fd != stdin_fd)
135112
set_nonblock(stdout_fd);
136-
else if ((stdout_fd = fcntl(stdin_fd, F_DUPFD_CLOEXEC, 3)) < 0)
137-
abort(); // not worth handling running out of file descriptors
138113
if (stderr_fd >= 0)
139114
set_nonblock(stderr_fd);
140115

116+
/* Convenience macros that eliminate a ton of error-prone boilerplate */
117+
#define close_stdin() do { \
118+
close_stdio(stdin_fd, stdout_fd, SHUT_WR); \
119+
stdin_fd = -1; \
120+
} while (0)
121+
#define close_stdout() do { \
122+
close_stdio(stdout_fd, stdin_fd, SHUT_RD); \
123+
stdout_fd = -1; \
124+
} while (0)
125+
#pragma GCC poison close_stdio
126+
141127
while(1) {
142128
/* React to SIGCHLD */
143129
if (*sigchld) {
@@ -147,10 +133,7 @@ int process_io(const struct process_io_request *req) {
147133
local_status = 128 + WTERMSIG(status);
148134
else
149135
local_status = WEXITSTATUS(status);
150-
if (stdin_fd >= 0) {
151-
close_stdin(stdin_fd, !use_stdio_socket);
152-
stdin_fd = -1;
153-
}
136+
close_stdin();
154137
}
155138
*sigchld = 0;
156139
}
@@ -193,24 +176,9 @@ int process_io(const struct process_io_request *req) {
193176
}
194177

195178
/* child signaled desire to use single socket for both stdin and stdout */
196-
if (sigusr1 && *sigusr1) {
197-
if (stdout_fd != -1) {
198-
do
199-
errno = 0;
200-
while (dup3(stdin_fd, stdout_fd, O_CLOEXEC) &&
201-
(errno == EINTR || errno == EBUSY));
202-
// other errors are fatal
203-
if (errno) {
204-
PERROR("dup3");
205-
abort();
206-
}
207-
} else {
208-
stdout_fd = fcntl(stdin_fd, F_DUPFD_CLOEXEC, 3);
209-
// all errors are fatal
210-
if (stdout_fd < 3)
211-
abort();
212-
}
213-
use_stdio_socket = true;
179+
if (sigusr1 && *sigusr1 && stdin_fd != stdout_fd) {
180+
close_stdout();
181+
stdout_fd = stdin_fd;
214182
*sigusr1 = 0;
215183
}
216184

@@ -263,10 +231,8 @@ int process_io(const struct process_io_request *req) {
263231
if (libvchan_wait(vchan) < 0)
264232
handle_vchan_error("wait");
265233

266-
if (stdin_fd >= 0 && fds[FD_STDIN].revents & (POLLHUP | POLLERR)) {
267-
close_stdin(stdin_fd, !use_stdio_socket);
268-
stdin_fd = -1;
269-
}
234+
if (fds[FD_STDIN].revents & (POLLHUP | POLLERR))
235+
close_stdin();
270236

271237
/* handle_remote_data will check if any data is available */
272238
switch (handle_remote_data(
@@ -281,16 +247,14 @@ int process_io(const struct process_io_request *req) {
281247
handle_vchan_error("read");
282248
break;
283249
case REMOTE_EOF:
284-
close_stdin(stdin_fd, !use_stdio_socket);
285-
stdin_fd = -1;
250+
close_stdin();
286251
break;
287252
case REMOTE_EXITED:
288253
/* Remote process exited, we don't need any more data from
289254
* local FDs. However, don't exit yet, because there might
290255
* still be some data in stdin_buf waiting to be flushed.
291256
*/
292-
close_stdout(stdout_fd, !use_stdio_socket);
293-
stdout_fd = -1;
257+
close_stdout();
294258
close_stderr(stderr_fd);
295259
stderr_fd = -1;
296260
break;
@@ -303,8 +267,7 @@ int process_io(const struct process_io_request *req) {
303267
handle_vchan_error("send(handle_input stdout)");
304268
break;
305269
case REMOTE_EOF:
306-
close_stdout(stdout_fd, !use_stdio_socket);
307-
stdout_fd = -1;
270+
close_stdout();
308271
break;
309272
}
310273
}
@@ -324,8 +287,8 @@ int process_io(const struct process_io_request *req) {
324287
}
325288
/* make sure that all the pipes/sockets are closed, so the child process
326289
* (if any) will know that the connection is terminated */
327-
close_stdin(stdin_fd, true);
328-
close_stdout(stdout_fd, true);
290+
close_stdin();
291+
close_stdout();
329292
close_stderr(stderr_fd);
330293

331294
/* wait for local process, in case we exited early */

0 commit comments

Comments
 (0)