sh: Don't assume EINTR means SIGALRM

While waiting for input in the read builtin, if select() is interrupted
but there is no pending signal, we act like we timed out, and return the
same status as if we had been interrupted by SIGALRM, instead of looping
until we actually do time out.

* Replace the single select() call with a ppoll() loop.

* Improve validation of the timeout value.  We now accept things like
  "1h30m15s", which we used to silently truncate to "1h".  The flip side
  is that we no longer accept things like "1hour" or "5sec".

* Modify the existing `read -t 0` test case to verify that read returns
  immediately when there is input and fails immediately when there isn't.

* Add a second test case which performs the same tests with a non-zero
  timeout value.

PR:		290844
MFC after:	1 week
Fixes:          c4539460e3 ("sh: Improve error handling in read builtin:")
Reviewed by:	jilles, bdrewery
Differential Revision:	https://reviews.freebsd.org/D53761
This commit is contained in:
Dag-Erling Smørgrav
2025-11-19 11:43:13 +01:00
parent 9b0102837e
commit 3c2643a7db
5 changed files with 112 additions and 29 deletions
+57 -26
View File
@@ -40,11 +40,14 @@
#include <sys/stat.h>
#include <sys/time.h>
#include <sys/resource.h>
#include <unistd.h>
#include <errno.h>
#include <poll.h>
#include <signal.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include "shell.h"
#include "options.h"
@@ -162,17 +165,18 @@ readcmd(int argc __unused, char **argv __unused)
int is_ifs;
int saveall = 0;
ptrdiff_t lastnonifs, lastnonifsws;
struct timeval tv;
char *tvptr;
fd_set ifds;
sigset_t set, oset;
intmax_t number, timeout;
struct timespec tnow, tend, tresid;
struct pollfd pfd;
char *endptr;
ssize_t nread;
int sig;
struct fdctx fdctx;
rflag = 0;
prompt = NULL;
tv.tv_sec = -1;
tv.tv_usec = 0;
timeout = -1;
while ((i = nextopt("erp:t:")) != '\0') {
switch(i) {
case 'p':
@@ -184,22 +188,29 @@ readcmd(int argc __unused, char **argv __unused)
rflag = 1;
break;
case 't':
tv.tv_sec = strtol(shoptarg, &tvptr, 0);
if (tvptr == shoptarg)
error("timeout value");
switch(*tvptr) {
case 0:
case 's':
break;
case 'h':
tv.tv_sec *= 60;
/* FALLTHROUGH */
case 'm':
tv.tv_sec *= 60;
break;
default:
error("timeout unit");
}
timeout = 0;
do {
number = strtol(shoptarg, &endptr, 0);
if (number < 0 || endptr == shoptarg)
error("timeout value");
switch (*endptr) {
case 's':
endptr++;
break;
case 'h':
number *= 60;
/* FALLTHROUGH */
case 'm':
number *= 60;
endptr++;
break;
}
if (*endptr != '\0' &&
!(*endptr >= '0' && *endptr <= '9'))
error("timeout unit");
timeout += number;
shoptarg = endptr;
} while (*shoptarg != '\0');
break;
}
}
@@ -212,13 +223,33 @@ readcmd(int argc __unused, char **argv __unused)
if ((ifs = bltinlookup("IFS", 1)) == NULL)
ifs = " \t\n";
if (tv.tv_sec >= 0) {
if (timeout >= 0) {
/*
* Wait for something to become available.
*/
FD_ZERO(&ifds);
FD_SET(0, &ifds);
status = select(1, &ifds, NULL, NULL, &tv);
pfd.fd = STDIN_FILENO;
pfd.events = POLLIN;
status = sig = 0;
sigfillset(&set);
sigprocmask(SIG_SETMASK, &set, &oset);
if (pendingsig) {
/* caught a signal already */
status = -1;
} else if (timeout == 0) {
status = poll(&pfd, 1, 0);
} else {
clock_gettime(CLOCK_UPTIME, &tnow);
tend = tnow;
tend.tv_sec += timeout;
do {
timespecsub(&tend, &tnow, &tresid);
status = ppoll(&pfd, 1, &tresid, &oset);
if (status >= 0 || pendingsig != 0)
break;
clock_gettime(CLOCK_UPTIME, &tnow);
} while (timespeccmp(&tnow, &tend, <));
}
sigprocmask(SIG_SETMASK, &oset, NULL);
/*
* If there's nothing ready, return an error.
*/
+5 -1
View File
@@ -31,7 +31,7 @@
.\" OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
.\" SUCH DAMAGE.
.\"
.Dd May 1, 2025
.Dd November 17, 2025
.Dt SH 1
.Os
.Sh NAME
@@ -2544,6 +2544,10 @@ to explicitly specify seconds, minutes or hours.
If none is supplied,
.Ql s
is assumed.
Multiple value-unit groups may be stringed together, in which case
they are added up, e.g.\&
.Ql 1h30m15s
which adds up to 5,415 seconds.
.Pp
The
.Fl e
+1
View File
@@ -143,6 +143,7 @@ ${PACKAGE}FILES+= read8.0
${PACKAGE}FILES+= read9.0
${PACKAGE}FILES+= read10.0
${PACKAGE}FILES+= read11.0
${PACKAGE}FILES+= read12.0
${PACKAGE}FILES+= return1.0
${PACKAGE}FILES+= return2.1
${PACKAGE}FILES+= return3.1
+17 -2
View File
@@ -1,3 +1,5 @@
# Verify that `read -t 0 v` succeeds immediately if input is available
# and fails immediately if not
set -e
@@ -6,12 +8,25 @@ trap 'rm -rf "$T"' 0
cd $T
mkfifo fifo1
# Open fifo1 for writing
{ sleep 10; } >fifo1 &
{ echo new_value; sleep 10; } >fifo1 &
# Wait for the child to open fifo1 for writing
exec 3<fifo1
v=original_value
r=0
ts=$(date +%s%3N)
read -t 0 v <&3 || r=$?
te=$(date +%s%3N)
[ "$r" -eq 0 ]
[ $((te-ts)) -lt 250 ]
[ "$v" = "new_value" ]
v=original_value
r=0
ts=$(date +%s%3N)
read -t 0 v <&3 || r=$?
te=$(date +%s%3N)
kill -TERM "$!" || :
{ [ "$r" -gt 128 ] && [ "$(kill -l "$r")" = ALRM ]; } || exit
[ "$r" -gt 128 ] && [ "$(kill -l "$r")" = ALRM ]
[ $((te-ts)) -lt 250 ]
[ -z "$v" ]
+32
View File
@@ -0,0 +1,32 @@
# Verify that `read -t 3 v` succeeds immediately if input is available
# and times out after 3 s if not
set -e
T=$(mktemp -d ${TMPDIR:-/tmp}/sh-test.XXXXXX)
trap 'rm -rf "$T"' 0
cd $T
mkfifo fifo1
# Open fifo1 for writing
{ echo new_value; sleep 10; } >fifo1 &
# Wait for the child to open fifo1 for writing
exec 3<fifo1
v=original_value
r=0
ts=$(date +%s%3N)
read -t 3 v <&3 || r=$?
te=$(date +%s%3N)
[ "$r" -eq 0 ]
[ $((te-ts)) -lt 250 ]
[ "$v" = "new_value" ]
v=original_value
r=0
ts=$(date +%s%3N)
read -t 3 v <&3 || r=$?
te=$(date +%s%3N)
kill -TERM "$!" || :
[ "$r" -gt 128 ] && [ "$(kill -l "$r")" = ALRM ]
[ $((te-ts)) -gt 3000 ] && [ $((te-ts)) -lt 3250 ]
[ -z "$v" ]