read -N 1 </dev/zero runs forever

[John Passaro]

Configuration Information [Automatically generated, do not change]:
Machine: x86_64
OS: darwin17.5.0
Compiler: clang
Compilation CFLAGS:  -DPROGRAM='bash' -DCONF_HOSTTYPE='x86_64' 
-DCONF_OSTYPE='darwin17.5.0' -DCONF_MACHTYPE='x86_64-apple-darwin17.5.0' 
-DCONF_VENDOR='apple' -DLOCALEDIR='/usr/local/Cellar/bash/4.4.23/share/locale' 
-DPACKAGE='bash' -DSHELL -DHAVE_CONFIG_H -DMACOSX   -I.  -I. -I./include 
-I./lib -I./lib/intl -I/private/tmp/bash-20180602-817-1lkxzgr/bash-4.4/lib/intl 
 -DSSH_SOURCE_BASHRC -Wno-parentheses -Wno-format-security
uname output: Darwin pop-jpas-mbpr-4.local 17.7.0 Darwin Kernel Version 17.7.0: 
Fri Nov  2 20:43:16 PDT 2018; root:xnu-4570.71.17~1/RELEASE_X86_64 x86_64
Machine Type: x86_64-apple-darwin17.5.0

Bash Version: 4.4
Patch Level: 23
Release Status: release

Description:
        The `read` builtin skips null characters and does not count
        them toward the limit specified by -n/-N. This leads to
        surprising behavior with /dev/zero: read -N 1 </dev/zero
        never terminates!

        This is obviously a corner case but it violates what ought
        to be a safe user assumption that specifying -N prevents
        read from running for too long.

Repeat-By:
        $ tr \\0 a </dev/zero | read -N # terminates immediately
        $ tr \\0 \\0 </dev/zero | read -N # runs forever

        or in the absence of /dev/zero:
        $ while true ; do echo -n a ; done | read -N 1 # terminates
        $ while true ; do echo -n a ; done | tr a \\0 | read -N 1 # runs forever

Fix:
        I propose to count nulls toward nchars.

        In the patch I replaced a `continue` with a `goto` that should
        result in equivalent behavior except for the case I'm raising
        here.  The only other place that the input char doesn't count
        toward nchars is when it's a backslash or a newline just after a
        backslash, and -r is not specified. I see no need to disrupt
        that behavior, but the case could be made; I leave it to the
        maintainers.

diff --git a/builtins/read.def b/builtins/read.def
index b57c8c398e18..5713766e4c3a 100644
--- a/builtins/read.def
+++ b/builtins/read.def
@@ -672,7 +672,7 @@ read_builtin (list)
        break;
 
       if (c == '\0' && delim != '\0')
-       continue;               /* skip NUL bytes in input */
+       goto increment_byte_count;              /* skip NUL bytes in input */
 
       if ((skip_ctlesc == 0 && c == CTLESC) || (skip_ctlnul == 0 && c == 
CTLNUL))
        {
@@ -713,6 +713,7 @@ add_char:
        }
 #endif
 
+increment_byte_count:
       nr++;
 
       if (nchars > 0 && nr >= nchars)
diff --git a/tests/read.right b/tests/read.right
index 73cb7042fbca..649c78a568c5 100644
--- a/tests/read.right
+++ b/tests/read.right
@@ -45,6 +45,7 @@ abcde
 abc
 ab
 abc
+ac
 #
 while read -u 3 var
 do
diff --git a/tests/read3.sub b/tests/read3.sub
index af41e3f27930..02334fdad6d2 100644
--- a/tests/read3.sub
+++ b/tests/read3.sub
@@ -20,5 +20,11 @@ echo abc | {
        echo $foo
 }
 
+# does not consume too many characters
+echo abcde | tr b '\0' | {
+       read -N 3 foo
+       echo $foo
+}
+
 read -n 1 < $0
 echo "$REPLY"