Re: [RFC/PATCH] pipe_command(): mark stdin descriptor as non-blocking
From: Junio C Hamano <hidden>
Date: 2022-08-02 15:04:46
Jeff King [off-list ref] writes:
Our pipe_command() helper lets you both write to and read from a child process on its stdin/stdout. It's supposed to work without deadlocks because we use poll() to check when descriptors are ready for reading or writing. But there's a bug: if both the data to be written and the data to be read back exceed the pipe buffer, we'll deadlock. ... If you set add.interactive.useBuiltin to false, the problem goes away, because now we're not using pipe_command() anymore (instead, that part happens in perl). But this isn't a bug in the interactive code at all. It's the underlying pipe_command() code which is broken, and has been all along. ... The obvious fix is to put the descriptor into non-blocking mode, and indeed, that makes the problem go away. Callers shouldn't need to care, because they never see the descriptor (they hand us a buffer to feed into it).
Thanks for a very well reasoned and explained patch.
- more importantly, I'm not sure of the portability implications of
the fix. This is our first use of O_NONBLOCK outside of the
compat/simple-ipc unix-socket code. Do we need to abstract this
behind a compat/ layer for Windows?Yup. A very good question to ask for the platform maintainer.
quoted hunk
run-command.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)diff --git a/run-command.c b/run-command.c index 14f17830f5..45bffb4b11 100644 --- a/run-command.c +++ b/run-command.c@@ -1418,6 +1418,14 @@ static int pump_io(struct io_pump *slots, int nr) return 0; } +static int make_nonblock(int fd) +{ + int flags = fcntl(fd, F_GETFL); + if (flags < 0) + return -1; + flags |= O_NONBLOCK; + return fcntl(fd, F_SETFL, flags); +} int pipe_command(struct child_process *cmd, const char *in, size_t in_len,@@ -1438,6 +1446,15 @@ int pipe_command(struct child_process *cmd, return -1; if (in) { + if (make_nonblock(cmd->in) < 0) { + error_errno("unable to make pipe non-blocking"); + close(cmd->in); + if (out) + close(cmd->out); + if (err) + close(cmd->err); + return -1; + } io[nr].fd = cmd->in; io[nr].type = POLLOUT; io[nr].u.out.buf = in;