Re: Builtin read with -n0 or -N0 (nchars == 0) behaves as a read with no -n/-N argument

[Chet Ramey]

On 6/3/17 1:15 PM, Pranav Deshpande wrote:
> Hello,
> Sorry for the late reply.
> 
> My solution is to change *line 294* of builtins/read.def.
> 
> Change
> if (code == 0 || *intval < 0* || intval != (int)intval)
> 
> to
> 
> if (code == 0 || i*ntval <= 0* || intval != (int)intval)

That's a fine solution for behavior 1.  If you'd prefer behavior 2,
obviously you have to do something else. :-)

Chet

-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, UTech, CWRUc...@case.eduhttp://cnswww.cns.cwru.edu/~chet/

Re: Builtin read with -n0 or -N0 (nchars == 0) behaves as a read with no -n/-N argument

[Eduardo Bustamante]

On Sun, Jun 4, 2017 at 12:16 AM, dualbus  wrote:
[...]
> Although there's a problem with the solution:
>
>   dualbus@debian:~$ for sh in bash ~/src/gnu/bash-build/bash ksh93 mksh; do 
> $sh -c ': | read -n 0; echo $?'; done
>   1
>   0
>   1
>   1
>
> Since the read(2) system call doesn't take place, `read -n 0' doesn't detect
> the broken pipe. IMO, it should.

Err, I'm clearly wrong. SIGPIPE is sent to the writer, not to the
reader. My bad.

That doesn't mean the problem is not there though. I think this is a
better test:

  dualbus@debian:~$ for sh in bash ksh93 mksh
~/src/gnu/bash-build/bash; do echo $sh $($sh -c 'exec < /; read -n 0;
echo $?' 2>&1); done
  bash bash: line 0: read: read error: 0: Is a directory 1
  ksh93 1
  mksh mksh: read: Is a directory 2
  /home/dualbus/src/gnu/bash-build/bash 0

Since bash "fakes" the read, it's not able to detect errors.

I currently have this:

dualbus@debian:~/src/gnu/bash$ git diff
diff --git a/builtins/read.def b/builtins/read.def
index 520a2b34..4e4c1b8a 100644
--- a/builtins/read.def
+++ b/builtins/read.def
@@ -362,10 +362,6 @@ read_builtin (list)
   input_string = (char *)xmalloc (size = 112); /* XXX was 128 */
   input_string[0] = '\0';

-  /* More input and options validation */
-  if (nflag == 1 && nchars == 0)
-goto assign_vars;  /* bail early if asked to read 0 chars */
-
   /* $TMOUT, if set, is the default timeout for read. */
   if (have_timeout == 0 && (e = get_string_value ("TMOUT")))
 {
@@ -381,6 +377,10 @@ read_builtin (list)

   begin_unwind_frame ("read_builtin");

+  /* We were asked to read 0 chars. Do error detection and bail out */
+  if (nflag == 1 && nchars == 0 && (retval=read(fd, NULL, 0)) < 0)
+goto handle_error;
+
 #if defined (BUFFERED_INPUT)
   if (interactive == 0 && default_buffered_input >= 0 && fd_is_bash_input (fd))
 sync_buffered_stream (default_buffered_input);
@@ -714,6 +714,7 @@ add_char:
 free (rlbuf);
 #endif

+handle_error:
   if (retval < 0)
 {
   t_errno = errno;


But it completely ignores the signal handling code down below.

Re: Builtin read with -n0 or -N0 (nchars == 0) behaves as a read with no -n/-N argument

[Pranav Deshpande]

Is that more advantageous?

On Sun, Jun 4, 2017 at 10:46 AM, dualbus  wrote:

> On Sun, Jun 04, 2017 at 01:45:42AM +0530, Pranav Deshpande wrote:
> [...]
> > My solution is to change *line 294* of builtins/read.def.
> >
> > Change
> > if (code == 0 || *intval < 0* || intval != (int)intval)
> >
> > to
> >
> > if (code == 0 || i*ntval <= 0* || intval != (int)intval)
> [...]
>
> > Is this solution ok?
>
> Yes. That works.
>
>
> Chet went with the other option though:
>
>   dualbus@debian:~/src/gnu/bash-build$ ./bash -c 'read -n0; echo $?;
> declare -p REPLY'
>   0
>   declare -- REPLY=""
>
> You can see the change by navigating the `devel' branch of the git
> repository
> in Savannah (commit 1110e30870a8782425067a060d89cc411b014418):
>
>   http://git.savannah.gnu.org/cgit/bash.git/commit/?h=devel=
> 1110e30870a8782425067a060d89cc411b014418
>
> Although there's a problem with the solution:
>
>   dualbus@debian:~$ for sh in bash ~/src/gnu/bash-build/bash ksh93 mksh;
> do $sh -c ': | read -n 0; echo $?'; done
>   1
>   0
>   1
>   1
>
> Since the read(2) system call doesn't take place, `read -n 0' doesn't
> detect
> the broken pipe. IMO, it should.
>
> --
> Eduardo Bustamante
> https://dualbus.me/
>

Re: Builtin read with -n0 or -N0 (nchars == 0) behaves as a read with no -n/-N argument

[dualbus]

On Sun, Jun 04, 2017 at 01:45:42AM +0530, Pranav Deshpande wrote:
[...]
> My solution is to change *line 294* of builtins/read.def.
> 
> Change
> if (code == 0 || *intval < 0* || intval != (int)intval)
> 
> to
> 
> if (code == 0 || i*ntval <= 0* || intval != (int)intval)
[...]

> Is this solution ok?

Yes. That works.


Chet went with the other option though:

  dualbus@debian:~/src/gnu/bash-build$ ./bash -c 'read -n0; echo $?; declare -p 
REPLY'
  0
  declare -- REPLY=""

You can see the change by navigating the `devel' branch of the git repository
in Savannah (commit 1110e30870a8782425067a060d89cc411b014418):

  
http://git.savannah.gnu.org/cgit/bash.git/commit/?h=devel=1110e30870a8782425067a060d89cc411b014418

Although there's a problem with the solution:

  dualbus@debian:~$ for sh in bash ~/src/gnu/bash-build/bash ksh93 mksh; do $sh 
-c ': | read -n 0; echo $?'; done
  1
  0
  1
  1

Since the read(2) system call doesn't take place, `read -n 0' doesn't detect
the broken pipe. IMO, it should.

-- 
Eduardo Bustamante
https://dualbus.me/

Re: Builtin read with -n0 or -N0 (nchars == 0) behaves as a read with no -n/-N argument

[Pranav Deshpande]

Hello,
Sorry for the late reply.

My solution is to change *line 294* of builtins/read.def.

Change
if (code == 0 || *intval < 0* || intval != (int)intval)

to

if (code == 0 || i*ntval <= 0* || intval != (int)intval)

Command:

./bash -c 'read -n0 <<< "abc";declare -p REPLY'

Output:
./bash: line 0: read: 0: invalid number
./bash: line 0: declare: REPLY: not found

i.e. behaviour #1.

Is this solution ok?

Regards,
Pranav

Re: Builtin read with -n0 or -N0 (nchars == 0) behaves as a read with no -n/-N argument

[Pranav Deshpande]

Thank you. I am travelling now and will look at it in 2 days.

Regards,
Pranav

On May 31, 2017 7:48 PM, "Chet Ramey"  wrote:

> On 5/23/17 11:33 AM, Eduardo Bustamante wrote:
> > (I think this is a good problem for Pranav to tackle if you consider
> > this to be a bug, Chet).
>
> Thanks. Another one that he can look at and see how his solution compares
> to mine.
>
> Chet
>
> --
> ``The lyf so short, the craft so long to lerne.'' - Chaucer
>  ``Ars longa, vita brevis'' - Hippocrates
> Chet Ramey, UTech, CWRUc...@case.eduhttp://cnswww.cns.cwru.edu/~
> chet/
>

Re: Builtin read with -n0 or -N0 (nchars == 0) behaves as a read with no -n/-N argument

[Chet Ramey]

On 5/23/17 11:33 AM, Eduardo Bustamante wrote:
> (I think this is a good problem for Pranav to tackle if you consider
> this to be a bug, Chet).

Thanks. Another one that he can look at and see how his solution compares
to mine.

Chet

-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, UTech, CWRUc...@case.eduhttp://cnswww.cns.cwru.edu/~chet/

Builtin read with -n0 or -N0 (nchars == 0) behaves as a read with no -n/-N argument

[Eduardo Bustamante]

(I think this is a good problem for Pranav to tackle if you consider
this to be a bug, Chet).

Current behavior:

dualbus@debian:~$ bash -c 'read -n0 <<< "abc"; declare -p REPLY'
declare -- REPLY="abc"

dualbus@debian:~$ bash --version|head -n1
GNU bash, version 4.4.11(1)-release (x86_64-pc-linux-gnu)

Expected behavior:

#1 nchars <= 0 treated as an error

i.e.

dualbus@debian:~/src/gnu/bash-build$ ./bash -c 'read -n0 <<< "abc";
declare -p REPLY'
./bash: line 0: read: 0: invalid number
./bash: line 0: declare: REPLY: not found

#2 adopt the same semantics as POSIX read(2) syscall

http://pubs.opengroup.org/onlinepubs/9699919799/functions/read.html describes:

| Before any action described below is taken, and if nbyte is zero, the read()
| function may detect and return errors as described below. In the absence of
| errors, or if error detection is not performed, the read() function shall
| return zero and have no other results.

This is what the rest of the shells that support a similar option do:

dualbus@debian:~$ for sh in bash ksh93 mksh zsh; do echo $sh $($sh -c
'set -- -n 0; [ -n "$ZSH_VERSION" ] && set -k 0; read "$@" REPLY <<<
abc; typeset -p REPLY'); done
bash declare -- REPLY="abc"
ksh93 REPLY=''
mksh typeset REPLY=
zsh typeset REPLY=''

I'm in favour of #2.

This was reported by Freenode user `mknod' in #bash