From 0f9c90e0edd27d2db04d48e7bd98bb46d2c4a918 Mon Sep 17 00:00:00 2001 From: Matt Widmann Date: Sat, 25 Nov 2017 13:59:07 -0800 Subject: [PATCH 1/2] io: retry fgets on EINTR (#7632) The calls to `fgets` in `src/nvim/if_cscope.c` (and elsewhere) can show communication errors to the user if a signal is delivered during its system calls. For plugins that proxy subprocess output into cscope requests, a `SIGCHLD` might *always* interfere with calls into `fgets`. To see this in a debugger, put a breakpoint on `cs_reading_emsg` and watch signals come in (with lldb, using `process handle --notify true --pass true`). Next, run a subcommand from neovim that calls through cscope when it returns. A tag picker plugin, like vim-picker and fzy, with `cscopetag` and `cscopetagorder=0` set, reproduced this reliably. The breakpoint will hit after a `SIGCHLD` is delivered, and `errno` will be set to 4, `EINTR`. The caller of `fgets` should retry when `NULL` is returned with `errno` set to `EINTR`. --- src/nvim/ex_cmds2.c | 6 ++++++ src/nvim/fileio.c | 5 +++++ src/nvim/if_cscope.c | 17 +++++++++++++++-- src/nvim/quickfix.c | 20 +++++++++++++++++--- 4 files changed, 43 insertions(+), 5 deletions(-) diff --git a/src/nvim/ex_cmds2.c b/src/nvim/ex_cmds2.c index 9b8e463aee..ec4ce63e17 100644 --- a/src/nvim/ex_cmds2.c +++ b/src/nvim/ex_cmds2.c @@ -3199,8 +3199,14 @@ static char_u *get_one_sourceline(struct source_cookie *sp) ga_grow(&ga, 120); buf = (char_u *)ga.ga_data; +retry: + errno = 0; if (fgets((char *)buf + ga.ga_len, ga.ga_maxlen - ga.ga_len, sp->fp) == NULL) { + if (errno == EINTR) { + goto retry; + } + break; } len = ga.ga_len + (int)STRLEN(buf + ga.ga_len); diff --git a/src/nvim/fileio.c b/src/nvim/fileio.c index ae6c3f96e3..9214e1e644 100644 --- a/src/nvim/fileio.c +++ b/src/nvim/fileio.c @@ -4448,7 +4448,12 @@ bool vim_fgets(char_u *buf, int size, FILE *fp) FUNC_ATTR_NONNULL_ALL char tbuf[FGETS_SIZE]; buf[size - 2] = NUL; +retry: + errno = 0; eof = fgets((char *)buf, size, fp); + if (eof == NULL && errno == EINTR) { + goto retry; + } if (buf[size - 2] != NUL && buf[size - 2] != '\n') { buf[size - 1] = NUL; /* Truncate the line */ diff --git a/src/nvim/if_cscope.c b/src/nvim/if_cscope.c index 0f9ecdf2d7..21e4b84074 100644 --- a/src/nvim/if_cscope.c +++ b/src/nvim/if_cscope.c @@ -553,9 +553,15 @@ static int cs_cnt_matches(size_t idx) char *buf = xmalloc(CSREAD_BUFSIZE); for (;; ) { + errno = 0; if (!fgets(buf, CSREAD_BUFSIZE, csinfo[idx].fr_fp)) { - if (feof(csinfo[idx].fr_fp)) + if (errno == EINTR) { + continue; + } + + if (feof(csinfo[idx].fr_fp)) { errno = EIO; + } cs_reading_emsg(idx); @@ -1381,9 +1387,16 @@ static char *cs_parse_results(size_t cnumber, char *buf, int bufsize, char *p; char *name; +retry: + errno = 0; if (fgets(buf, bufsize, csinfo[cnumber].fr_fp) == NULL) { - if (feof(csinfo[cnumber].fr_fp)) + if (errno == EINTR) { + goto retry; + } + + if (feof(csinfo[cnumber].fr_fp)) { errno = EIO; + } cs_reading_emsg(cnumber); diff --git a/src/nvim/quickfix.c b/src/nvim/quickfix.c index b9228e15b9..1fc585f0c9 100644 --- a/src/nvim/quickfix.c +++ b/src/nvim/quickfix.c @@ -570,7 +570,12 @@ static int qf_get_next_file_line(qfstate_T *state) { size_t growbuflen; +retry: + errno = 0; if (fgets((char *)IObuff, IOSIZE, state->fd) == NULL) { + if (errno == EINTR) { + goto retry; + } return QF_END_OF_INPUT; } @@ -590,8 +595,12 @@ static int qf_get_next_file_line(qfstate_T *state) growbuflen = state->linelen; for (;;) { + errno = 0; if (fgets((char *)state->growbuf + growbuflen, (int)(state->growbufsiz - growbuflen), state->fd) == NULL) { + if (errno == EINTR) { + continue; + } break; } state->linelen = STRLEN(state->growbuf + growbuflen); @@ -612,9 +621,14 @@ static int qf_get_next_file_line(qfstate_T *state) while (discard) { // The current line is longer than LINE_MAXLEN, continue reading but // discard everything until EOL or EOF is reached. - if (fgets((char *)IObuff, IOSIZE, state->fd) == NULL - || STRLEN(IObuff) < IOSIZE - 1 - || IObuff[IOSIZE - 1] == '\n') { + errno = 0; + if (fgets((char *)IObuff, IOSIZE, state->fd) == NULL) { + if (errno == EINTR) { + continue; + } + break; + } + if (STRLEN(IObuff) < IOSIZE - 1 || IObuff[IOSIZE - 1] == '\n') { break; } } From bab2f8200aea09ad32bbcb2e83404bbd4076a227 Mon Sep 17 00:00:00 2001 From: Matt Widmann Date: Sat, 25 Nov 2017 13:27:59 -0800 Subject: [PATCH 2/2] io: fix handling EOF in vim_fgets If an EOF is returned from `fgets`, `vim_fgets` might spin forever, as it tries to consume the current line. A `NULL` return value from `fgets` should break out of the function (unless `errno` is `EINTR`), and then `feof` should be used to check for the EOF condition on the stream. --- src/nvim/fileio.c | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/src/nvim/fileio.c b/src/nvim/fileio.c index 9214e1e644..1f4cd22754 100644 --- a/src/nvim/fileio.c +++ b/src/nvim/fileio.c @@ -4443,27 +4443,32 @@ char *modname(const char *fname, const char *ext, bool prepend_dot) /// @return true for end-of-file. bool vim_fgets(char_u *buf, int size, FILE *fp) FUNC_ATTR_NONNULL_ALL { - char *eof; -#define FGETS_SIZE 200 - char tbuf[FGETS_SIZE]; + char *retval; + assert(size > 0); buf[size - 2] = NUL; -retry: - errno = 0; - eof = fgets((char *)buf, size, fp); - if (eof == NULL && errno == EINTR) { - goto retry; - } - if (buf[size - 2] != NUL && buf[size - 2] != '\n') { - buf[size - 1] = NUL; /* Truncate the line */ - /* Now throw away the rest of the line: */ + do { + errno = 0; + retval = fgets((char *)buf, size, fp); + } while (retval == NULL && errno == EINTR); + + if (buf[size - 2] != NUL && buf[size - 2] != '\n') { + char tbuf[200]; + + buf[size - 1] = NUL; // Truncate the line. + + // Now throw away the rest of the line: do { - tbuf[FGETS_SIZE - 2] = NUL; - ignoredp = fgets((char *)tbuf, FGETS_SIZE, fp); - } while (tbuf[FGETS_SIZE - 2] != NUL && tbuf[FGETS_SIZE - 2] != '\n'); + tbuf[sizeof(tbuf) - 2] = NUL; + errno = 0; + retval = fgets((char *)tbuf, sizeof(tbuf), fp); + if (retval == NULL && errno != EINTR) { + break; + } + } while (tbuf[sizeof(tbuf) - 2] != NUL && tbuf[sizeof(tbuf) - 2] != '\n'); } - return eof == NULL; + return retval ? false : feof(fp); } /// Read 2 bytes from "fd" and turn them into an int, MSB first.