From ff385e5a8928071f2ada262016d24964da70f255 Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Wed, 19 Aug 2020 01:36:01 +0100 Subject: [PATCH] Speed up 'read', fixing macOS hang This fixes a hanging bug that could occur on macOS when using the 'read' command to read from a FIFO and encountering end-of-file without a final newline character. It also makes the 'read' command perform 15-25% faster on macOS and Linux and maybe other systems. src/lib/libast/sfio/sfpkrd.c: sfpkrd(): - Get rid of the optional stuff that uses the poll(2) or select(2) syscalls. The only thing that is required to avoid regressions is the code that was conditional upon the socket_peek feature test, which tests for the correct functioning of the recv(2) syscall. This has now been made mandatory. The rest now uses what was previously a fallback in plain C, resulting in a function that is not only more readable, but actually faster than the syscalls. Resolves: https://github.com/ksh93/ksh/issues/118 --- NEWS | 5 + src/cmd/ksh93/include/version.h | 2 +- src/cmd/ksh93/tests/io.sh | 14 ++ src/lib/libast/sfio/sfpkrd.c | 251 +++----------------------------- 4 files changed, 40 insertions(+), 232 deletions(-) diff --git a/NEWS b/NEWS index b6fdf57da..7cf84b4c2 100644 --- a/NEWS +++ b/NEWS @@ -3,6 +3,11 @@ For full details, see the git log at: https://github.com/ksh93/ksh Any uppercase BUG_* names are modernish shell bug IDs. +2020-08-18: + +- Sped up the 'read' command on most systems by 15-25%. Fixed a hanging bug + on reading from a FIFO that could occur on macOS. + 2020-08-17: - 'command -p' incorrectly used the hash table entry (a.k.a. tracked alias) diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h index 8899cd5ca..20c569990 100644 --- a/src/cmd/ksh93/include/version.h +++ b/src/cmd/ksh93/include/version.h @@ -17,4 +17,4 @@ * David Korn * * * ***********************************************************************/ -#define SH_RELEASE "93u+m 2020-08-17" +#define SH_RELEASE "93u+m 2020-08-18" diff --git a/src/cmd/ksh93/tests/io.sh b/src/cmd/ksh93/tests/io.sh index 2dfd036e9..cf37d0ce5 100755 --- a/src/cmd/ksh93/tests/io.sh +++ b/src/cmd/ksh93/tests/io.sh @@ -583,5 +583,19 @@ then exec 3>&- err_exit "Open file descriptor leaks out of subshell" fi +# ====== +# On unpatched ksh on macOS, 'read' used to block when reading from a FIFO and there was no final newline. +if mkfifo "$tmp/fifo_no_lf" +then trap 'sleep_pid=0; kill "$ksh_pid"; err_exit "'\''read'\'' hangs on EOF without final linefeed when reading from FIFO"' TERM + (sleep 1; kill "$$") & + sleep_pid=$! + "$SHELL" -c 'print -n foo >$0 & while read f; do :; done <$0' "$tmp/fifo_no_lf" & + ksh_pid=$! + wait "$ksh_pid" + trap - TERM + ((sleep_pid)) && kill "$sleep_pid" +else err_exit "mkfifo failed; cannot test reading from FIFO" +fi + # ====== exit $((Errors<125?Errors:125)) diff --git a/src/lib/libast/sfio/sfpkrd.c b/src/lib/libast/sfio/sfpkrd.c index 2572e6dc6..a48561f76 100644 --- a/src/lib/libast/sfio/sfpkrd.c +++ b/src/lib/libast/sfio/sfpkrd.c @@ -20,11 +20,12 @@ * * ***********************************************************************/ #include "sfhdr.h" -#if !_PACKAGE_ast -#ifndef FIONREAD -#if _sys_ioctl -#include -#endif + +#if !_socket_peek +#if __APPLE__ +#error The socket_peek feature is required. (Hey Apple, revert your src__lib__libast__features__lib.diff patch; it caused multiple regressions, and the hanging bug it fixed is now fixed correctly. See .) +#else +#error The socket_peek feature is required. #endif #endif @@ -33,9 +34,6 @@ ** Written by Kiem-Phong Vo. */ -#define STREAM_PEEK 001 -#define SOCKET_PEEK 002 - #if __STD_C ssize_t sfpkrd(int fd, Void_t* argbuf, size_t n, int rc, long tm, int action) #else @@ -52,237 +50,28 @@ int action; /* >0: peeking, if rc>=0, get action records, #endif { reg ssize_t r; - reg int ntry, t; + reg int t; reg char *buf = (char*)argbuf, *endbuf; if(rc < 0 && tm < 0 && action <= 0) return sysreadf(fd,buf,n); - t = (action > 0 || rc >= 0) ? (STREAM_PEEK|SOCKET_PEEK) : 0; -#if !_stream_peek - t &= ~STREAM_PEEK; -#endif -#if !_socket_peek - t &= ~SOCKET_PEEK; -#endif - - for(ntry = 0; ntry < 2; ++ntry) - { - r = -1; -#if _stream_peek - if((t&STREAM_PEEK) && (ntry == 1 || tm < 0) ) - { -#ifdef __sun - /* - * I_PEEK on stdin can hang rsh+ksh on solaris - * this kludge will have to do until sun^H^H^Horacle fixes I_PEEK/rsh - */ - static int stream_peek; - if (stream_peek == 0) /* this will be done just once */ - { char *e; - stream_peek = ( - getenv("LOGNAME") == 0 && - getenv("MAIL") == 0 && - ((e = getenv("LANG")) == 0 || strcmp(e, "C") == 0) && - ((e = getenv("PATH")) == 0 || strncmp(e, "/usr/bin:", 9) == 0) - ) ? -1 : 1; - } - if(stream_peek < 0) - t &= ~STREAM_PEEK; - else -#endif - { struct strpeek pbuf; - pbuf.flags = 0; - pbuf.ctlbuf.maxlen = -1; - pbuf.ctlbuf.len = 0; - pbuf.ctlbuf.buf = NIL(char*); - pbuf.databuf.maxlen = n; - pbuf.databuf.buf = buf; - pbuf.databuf.len = 0; - - if((r = ioctl(fd,I_PEEK,&pbuf)) < 0) - { if(errno == EINTR) - return -1; - t &= ~STREAM_PEEK; - } - else - { t &= ~SOCKET_PEEK; - if(r > 0 && (r = pbuf.databuf.len) <= 0) - { if(action <= 0) /* read past eof */ - r = sysreadf(fd,buf,1); - return r; - } - if(r == 0) - r = -1; - else if(r > 0) - break; - } - } - } -#endif /* stream_peek */ - - if(ntry == 1) - break; - - /* poll or select to see if data is present. */ - while(tm >= 0 || action > 0 || - /* block until there is data before peeking again */ - ((t&STREAM_PEEK) && rc >= 0) || - /* let select be interrupted instead of recv which autoresumes */ - (t&SOCKET_PEEK) ) - { r = -2; -#if _lib_poll - if(r == -2) - { - struct pollfd po; - po.fd = fd; - po.events = POLLIN; - po.revents = 0; - - if((r = SFPOLL(&po,1,tm)) < 0) - { if(errno == EINTR) - return -1; - else if(errno == EAGAIN) - { errno = 0; - continue; - } - else r = -2; - } - else r = (po.revents&POLLIN) ? 1 : -1; - } -#endif /*_lib_poll*/ -#if _lib_select - if(r == -2) - { -#if _hpux_threads && vt_threaded -#define fd_set int -#endif - fd_set rd; - struct timeval tmb, *tmp; - FD_ZERO(&rd); - FD_SET(fd,&rd); - if(tm < 0) - tmp = NIL(struct timeval*); - else - { tmp = &tmb; - tmb.tv_sec = tm/SECOND; - tmb.tv_usec = (tm%SECOND)*SECOND; - } - r = select(fd+1,&rd,NIL(fd_set*),NIL(fd_set*),tmp); - if(r < 0) - { if(errno == EINTR) - return -1; - else if(errno == EAGAIN) - { errno = 0; - continue; - } - else r = -2; - } - else r = FD_ISSET(fd,&rd) ? 1 : -1; - } -#endif /*_lib_select*/ - if(r == -2) - { -#if !_lib_poll && !_lib_select /* both poll and select can't be used */ -#ifdef FIONREAD /* quick and dirty check for availability */ - long nsec = tm < 0 ? 0 : (tm+999)/1000; - while(nsec > 0 && r < 0) - { long avail = -1; - if((r = ioctl(fd,FIONREAD,&avail)) < 0) - { if(errno == EINTR) - return -1; - else if(errno == EAGAIN) - { errno = 0; - continue; - } - else /* ioctl failed completely */ - { r = -2; - break; - } - } - else r = avail <= 0 ? -1 : (ssize_t)avail; - - if(r < 0 && nsec-- > 0) - sleep(1); - } -#endif -#endif - } - - if(r > 0) /* there is data now */ - { if(action <= 0 && rc < 0) - return sysreadf(fd,buf,n); - else r = -1; - } - else if(tm >= 0) /* timeout exceeded */ + r = -1; + t = action > 0 || rc >= 0; /* socket peek using recv(2)? */ + if(t) + { while(t && (r = recv(fd,(char*)buf,n,MSG_PEEK)) < 0) + { if(errno == EINTR) return -1; - else r = -1; - break; + else if(errno == EAGAIN) + errno = 0; + else t = 0; } - -#if _socket_peek - if(t&SOCKET_PEEK) - { -#if __MACH__ && __APPLE__ /* check 10.4 recv(MSG_PEEK) bug that consumes pipe data */ - static int recv_peek_pipe; - if (recv_peek_pipe == 0) /* this will be done just once */ - { int fds[2], r; - char tst[2]; - - tst[0] = 'a'; tst[1] = 'z'; - - /* open a pipe and write to it */ - recv_peek_pipe = 1; - if(recv_peek_pipe == 1 && pipe(fds) < 0) - recv_peek_pipe = -1; - if(recv_peek_pipe == 1 && write(fds[1], tst, 2) != 2) - recv_peek_pipe = -1; - - /* try recv() to see if it gets anything */ - tst[0] = tst[1] = 0; - if(recv_peek_pipe == 1 && (r = recv(fds[0], tst, 1, MSG_PEEK)) != 1) - recv_peek_pipe = -1; - if(recv_peek_pipe == 1 && tst[0] != 'a') - recv_peek_pipe = -1; - - /* make sure that recv() did not consume data */ - tst[0] = tst[1] = 0; - if(recv_peek_pipe == 1 && (r = recv(fds[0], tst, 2, MSG_PEEK)) != 2) - recv_peek_pipe = -1; - if(recv_peek_pipe == 1 && (tst[0] != 'a' || tst[1] != 'z') ) - recv_peek_pipe = -1; - - close(fds[0]); - close(fds[1]); - } - - if(recv_peek_pipe < 0) - { struct stat st; /* recv should work on sockets */ - if(fstat(fd, &st) < 0 || !S_ISSOCK(st.st_mode) ) - { r = -1; - t &= ~SOCKET_PEEK; - } - } -#endif - while((t&SOCKET_PEEK) && (r = recv(fd,(char*)buf,n,MSG_PEEK)) < 0) - { if(errno == EINTR) - return -1; - else if(errno == EAGAIN) - errno = 0; - else t &= ~SOCKET_PEEK; - } - if(r >= 0) - { t &= ~STREAM_PEEK; - if(r > 0) - break; - else /* read past eof */ - { if(action <= 0) - r = sysreadf(fd,buf,1); - return r; - } - } + if(r == 0) + { /* read past eof */ + if(action <= 0) + r = sysreadf(fd,buf,1); + return r; } -#endif } if(r < 0)