From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp3.mail.ru (smtp3.mail.ru [94.100.179.58]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 2A0454696CE for ; Tue, 14 Apr 2020 14:39:08 +0300 (MSK) From: Alexander Turenko Date: Tue, 14 Apr 2020 14:38:21 +0300 Message-Id: <6873a6070a181c7c47f2e94295539c483fd6e7b7.1586862436.git.alexander.turenko@tarantool.org> In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [Tarantool-patches] [PATCH 12/12] popen: allow to close parent's end of std* fds List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov Cc: tarantool-patches@dev.tarantool.org The function popen_shutdown() checks whether std{in,out,err} was piped and closes the parent's end. A user should have ability to send EOF for child's stdin for stream programs like `grep`. It is better when there is a function that encapsulates proper checks, error messages and the actual actions. This commit in particular reverts 1ef95b99f6553b246729e7bb5bdc19038043db74 ('popen: remove redundant fd check before perform IO'), because now the check is meaningful: an fd may become closed before the whole popen handle will be deleted. Part of #4031 --- src/lib/core/popen.c | 170 ++++++++++++++++++++++++++++++++----------- src/lib/core/popen.h | 3 + 2 files changed, 130 insertions(+), 43 deletions(-) diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c index 640dffc2b..8760429c2 100644 --- a/src/lib/core/popen.c +++ b/src/lib/core/popen.c @@ -34,6 +34,43 @@ static RLIST_HEAD(popen_head); static int dev_null_fd_ro = -1; static int dev_null_fd_wr = -1; +static const struct { + unsigned int mask; + unsigned int mask_devnull; + unsigned int mask_close; + int fileno; + int *dev_null_fd; + int parent_idx; + int child_idx; + bool nonblock; +} pfd_map[POPEN_FLAG_FD_STDEND_BIT] = { + { + .mask = POPEN_FLAG_FD_STDIN, + .mask_devnull = POPEN_FLAG_FD_STDIN_DEVNULL, + .mask_close = POPEN_FLAG_FD_STDIN_CLOSE, + .fileno = STDIN_FILENO, + .dev_null_fd = &dev_null_fd_ro, + .parent_idx = 1, + .child_idx = 0, + }, { + .mask = POPEN_FLAG_FD_STDOUT, + .mask_devnull = POPEN_FLAG_FD_STDOUT_DEVNULL, + .mask_close = POPEN_FLAG_FD_STDOUT_CLOSE, + .fileno = STDOUT_FILENO, + .dev_null_fd = &dev_null_fd_wr, + .parent_idx = 0, + .child_idx = 1, + }, { + .mask = POPEN_FLAG_FD_STDERR, + .mask_devnull = POPEN_FLAG_FD_STDERR_DEVNULL, + .mask_close = POPEN_FLAG_FD_STDERR_CLOSE, + .fileno = STDERR_FILENO, + .dev_null_fd = &dev_null_fd_wr, + .parent_idx = 0, + .child_idx = 1, + }, +}; + /** * Register popen handle in a pids map. */ @@ -213,7 +250,8 @@ handle_free(struct popen_handle *handle) * Returns 0 if so and -1 otherwise (and set a diag). */ static inline int -popen_may_io(struct popen_handle *handle, unsigned int io_flags) +popen_may_io(struct popen_handle *handle, unsigned int idx, + unsigned int io_flags, bool allow_closed) { if (!(io_flags & handle->flags)) { diag_set(IllegalParams, "popen: handle does not support the " @@ -221,6 +259,12 @@ popen_may_io(struct popen_handle *handle, unsigned int io_flags) return -1; } + if (! allow_closed && handle->ios[idx].fd < 0) { + diag_set(IllegalParams, "popen: attempt to operate on a closed " + "file descriptor"); + return -1; + } + return 0; } @@ -299,6 +343,7 @@ stdX_str(unsigned int index) * - count: data is too big. * - flags: POPEN_FLAG_FD_STDIN bit is unset. * - handle: handle does not support the requested IO operation. + * - handle: attempt to operate on a closed fd. * - SocketError: an IO error occurs at write(). * - TimedOut: @a timeout quota is exceeded. * - FiberIsCancelled: cancelled by an outside code. @@ -327,11 +372,11 @@ popen_write_timeout(struct popen_handle *handle, const void *buf, return -1; } - if (popen_may_io(handle, flags) != 0) - return -1; - int idx = STDIN_FILENO; + if (popen_may_io(handle, idx, flags, false) != 0) + return -1; + say_debug("popen: %d: write idx [%s:%d] buf %p count %zu " "fds %d timeout %.9g", handle->pid, stdX_str(idx), idx, buf, count, @@ -362,6 +407,7 @@ popen_write_timeout(struct popen_handle *handle, const void *buf, * - count: buffer is too big. * - flags: stdout and stdrr are both choosen or both missed * - handle: handle does not support the requested IO operation. + * - handle: attempt to operate on a closed fd. * - SocketError: an IO error occurs at read(). * - TimedOut: @a timeout quota is exceeded. * - FiberIsCancelled: cancelled by an outside code. @@ -390,12 +436,12 @@ popen_read_timeout(struct popen_handle *handle, void *buf, return -1; } - if (popen_may_io(handle, flags) != 0) - return -1; - int idx = flags & POPEN_FLAG_FD_STDOUT ? STDOUT_FILENO : STDERR_FILENO; + if (popen_may_io(handle, idx, flags, false) != 0) + return -1; + say_debug("popen: %d: read idx [%s:%d] buf %p count %zu " "fds %d timeout %.9g", handle->pid, stdX_str(idx), idx, buf, count, @@ -405,6 +451,80 @@ popen_read_timeout(struct popen_handle *handle, void *buf, timeout); } +/** + * Close parent's ends of std* fds. + * + * The following @a flags controls which fds should be closed: + * + * POPEN_FLAG_FD_STDIN close parent's end of child's stdin + * POPEN_FLAG_FD_STDOUT close parent's end of child's stdout + * POPEN_FLAG_FD_STDERR close parent's end of child's stderr + * + * The main reason to use this function is to send EOF to + * child's stdin. However parent's end of stdout / stderr + * may be closed too. + * + * The function does not fail on already closed fds (idempotence). + * However it fails on attempt to close the end of a pipe that was + * never exist. In other words, a subset of ..._FD_STD{IN,OUT,ERR} + * flags used at a handle creation may be used here. + * + * The function does not close any fds on a failure: either all + * requested fds are closed or neither of them. + * + * Returns 0 at success, otherwise -1 and set a diag. + * + * Possible errors: + * + * - IllegalParams: a parameter check fails: + * - flags: neither stdid, stdout nor stderr is choosen. + * - handle: handle does not support the requested IO operation + * (one of fds is not piped). + */ +int +popen_shutdown(struct popen_handle *handle, unsigned int flags) +{ + assert(handle != NULL); + + if ((flags & (POPEN_FLAG_FD_STDIN | + POPEN_FLAG_FD_STDOUT | + POPEN_FLAG_FD_STDERR)) == 0) { + diag_set(IllegalParams, + "popen: neither stdin, stdout nor stderr is choosen"); + return -1; + } + + /* Verify the operation. */ + for (int idx = STDIN_FILENO; idx < POPEN_FLAG_FD_STDEND_BIT; ++idx) { + /* Operate only on asked fds. */ + unsigned int op_mask = pfd_map[idx].mask; + if ((flags & op_mask) == 0) + continue; + + if (popen_may_io(handle, idx, op_mask, true) != 0) + return -1; + } + + /* Perform the operation. */ + for (int idx = STDIN_FILENO; idx < POPEN_FLAG_FD_STDEND_BIT; ++idx) { + /* Operate only on asked fds. */ + unsigned int op_mask = pfd_map[idx].mask; + if ((flags & op_mask) == 0) + continue; + + /* Skip already closed fds. */ + if (handle->ios[idx].fd < 0) + continue; + + say_debug("popen: %d: shutdown idx [%s:%d] fd %s", + handle->pid, stdX_str(idx), idx, + handle->ios[idx].fd); + coio_close_io(loop(), &handle->ios[idx]); + } + + return 0; +} + /** * Encode signal status into a human readable form. * @@ -865,42 +985,6 @@ popen_new(struct popen_opts *opts) int saved_errno; size_t i; - static const struct { - unsigned int mask; - unsigned int mask_devnull; - unsigned int mask_close; - int fileno; - int *dev_null_fd; - int parent_idx; - int child_idx; - bool nonblock; - } pfd_map[POPEN_FLAG_FD_STDEND_BIT] = { - { - .mask = POPEN_FLAG_FD_STDIN, - .mask_devnull = POPEN_FLAG_FD_STDIN_DEVNULL, - .mask_close = POPEN_FLAG_FD_STDIN_CLOSE, - .fileno = STDIN_FILENO, - .dev_null_fd = &dev_null_fd_ro, - .parent_idx = 1, - .child_idx = 0, - }, { - .mask = POPEN_FLAG_FD_STDOUT, - .mask_devnull = POPEN_FLAG_FD_STDOUT_DEVNULL, - .mask_close = POPEN_FLAG_FD_STDOUT_CLOSE, - .fileno = STDOUT_FILENO, - .dev_null_fd = &dev_null_fd_wr, - .parent_idx = 0, - .child_idx = 1, - }, { - .mask = POPEN_FLAG_FD_STDERR, - .mask_devnull = POPEN_FLAG_FD_STDERR_DEVNULL, - .mask_close = POPEN_FLAG_FD_STDERR_CLOSE, - .fileno = STDERR_FILENO, - .dev_null_fd = &dev_null_fd_wr, - .parent_idx = 0, - .child_idx = 1, - }, - }; /* * At max we could be skipping each pipe end * plus dev/null variants and logfd diff --git a/src/lib/core/popen.h b/src/lib/core/popen.h index 4cdd95175..c068d5028 100644 --- a/src/lib/core/popen.h +++ b/src/lib/core/popen.h @@ -175,6 +175,9 @@ popen_read_timeout(struct popen_handle *handle, void *buf, size_t count, unsigned int flags, ev_tstamp timeout); +extern int +popen_shutdown(struct popen_handle *handle, unsigned int flags); + extern void popen_state(struct popen_handle *handle, int *state, int *exit_code); -- 2.25.0