GNU bug report logs - #16940
[PATCH] tests: Do not access /dev/tty if it does not exist

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 16940 in the body.
You can then email your comments to 16940 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Cyril Roelandt <tipecaml <at> gmail.com>
To: bug-coreutils <at> gnu.org
Cc: Cyril Roelandt <tipecaml <at> gmail.com>
Subject: [PATCH] tests: Do not access /dev/tty if it does not exist
Date: Wed,  5 Mar 2014 04:34:47 +0100

* tests/misc/nohup.sh: Do not try to access /dev/tty if it does not exist. This
  happens on GNU Guix, for instance.
---
 tests/misc/nohup.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/misc/nohup.sh b/tests/misc/nohup.sh
index 6d2b515..e331eab 100755
--- a/tests/misc/nohup.sh
+++ b/tests/misc/nohup.sh
@@ -61,7 +61,7 @@ rm -f nohup.out err
 
 # Bug present through coreutils 8.0: failure to print advisory message
 # to stderr must be fatal.  Requires stdout to be terminal.
-if test -w /dev/full && test -c /dev/full; then
+if test -w /dev/full && test -c /dev/full && test -f /dev/tty; then
 (
   exec >/dev/tty
   test -t 1 || exit 0
-- 
1.9.0

Message #8 received at 16940 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Cyril Roelandt <tipecaml <at> gmail.com>
Cc: 16940 <at> debbugs.gnu.org
Subject: Re: bug#16940: [PATCH] tests: Do not access /dev/tty if it does not
 exist
Date: Wed, 05 Mar 2014 04:20:04 +0000

On 03/05/2014 03:34 AM, Cyril Roelandt wrote:
> * tests/misc/nohup.sh: Do not try to access /dev/tty if it does not exist. This
>   happens on GNU Guix, for instance.
> ---
>  tests/misc/nohup.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/misc/nohup.sh b/tests/misc/nohup.sh
> index 6d2b515..e331eab 100755
> --- a/tests/misc/nohup.sh
> +++ b/tests/misc/nohup.sh
> @@ -61,7 +61,7 @@ rm -f nohup.out err
>  
>  # Bug present through coreutils 8.0: failure to print advisory message
>  # to stderr must be fatal.  Requires stdout to be terminal.
> -if test -w /dev/full && test -c /dev/full; then
> +if test -w /dev/full && test -c /dev/full && test -f /dev/tty; then
>  (
>    exec >/dev/tty
>    test -t 1 || exit 0

We might as well ensure it's a writeable char device while we're at it.
I'll apply it in your name in the morning with the following adjustment,
unless you have an objection:

  if test -w /dev/full && test -c /dev/full &&
     test -w /dev/tty && test -c /dev/tty; then

Note this might also handle this Fedora rawhide issue
that I noticed in passing yesterday:
http://pkgs.fedoraproject.org/cgit/coreutils.git/commit/?id=156e6cc

thanks!
Pádraig.

Message #11 received at 16940 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Pádraig Brady <P <at> draigBrady.com>, 
 Cyril Roelandt <tipecaml <at> gmail.com>
Cc: 16940 <at> debbugs.gnu.org
Subject: Re: bug#16940: [PATCH] tests: Do not access /dev/tty if it does not
 exist
Date: Wed, 05 Mar 2014 10:17:06 +0100

On 03/05/2014 05:20 AM, Pádraig Brady wrote:
>    if test -w /dev/full && test -c /dev/full &&
>       test -w /dev/tty && test -c /dev/tty; then

This looks right.
However, I'm wondering why the 'test -t 1' right after
the exec redirection didn't already catch this:

  if test -w /dev/full && test -c /dev/full; then
  (
    exec >/dev/tty
    test -t 1 || exit 0
    nohup echo hi 2> /dev/full
    test $? = 125 || fail=1
    test -f nohup.out || fail=1
    test -s nohup.out && fail=1
    rm -f nohup.out
    exit $fail
  ) || fail=1
  fi

Does someone have a test log file with the failure?

Have a nice day,
Berny

Message #14 received at 16940 <at> debbugs.gnu.org (full text, mbox):

From: Cyril Roelandt <tipecaml <at> gmail.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>, Pádraig Brady <P <at> draigBrady.com>
Cc: 16940 <at> debbugs.gnu.org
Subject: Re: bug#16940: [PATCH] tests: Do not access /dev/tty if it does not
 exist
Date: Wed, 05 Mar 2014 16:03:34 +0100

On 03/05/2014 10:17 AM, Bernhard Voelker wrote:
> On 03/05/2014 05:20 AM, Pádraig Brady wrote:
>>     if test -w /dev/full && test -c /dev/full &&
>>        test -w /dev/tty && test -c /dev/tty; then
>
> This looks right.
> However, I'm wondering why the 'test -t 1' right after
> the exec redirection didn't already catch this:
>
>     if test -w /dev/full && test -c /dev/full; then
>     (
>       exec >/dev/tty
>       test -t 1 || exit 0
>       nohup echo hi 2> /dev/full
>       test $? = 125 || fail=1
>       test -f nohup.out || fail=1
>       test -s nohup.out && fail=1
>       rm -f nohup.out
>       exit $fail
>     ) || fail=1
>     fi
>
> Does someone have a test log file with the failure?
>

The bug was originally reported on GNU Guix, here: 
https://lists.gnu.org/archive/html/guix-devel/2014-03/msg00009.html

Cyril.

Message #17 received at 16940 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>, 
 Cyril Roelandt <tipecaml <at> gmail.com>
Cc: 16940 <at> debbugs.gnu.org
Subject: Re: bug#16940: [PATCH] tests: Do not access /dev/tty if it does not
 exist
Date: Wed, 05 Mar 2014 15:21:37 +0000

[Message part 1 (text/plain, inline)]

On 03/05/2014 09:17 AM, Bernhard Voelker wrote:
> On 03/05/2014 05:20 AM, Pádraig Brady wrote:
>>    if test -w /dev/full && test -c /dev/full &&
>>       test -w /dev/tty && test -c /dev/tty; then
> 
> This looks right.
> However, I'm wondering why the 'test -t 1' right after
> the exec redirection didn't already catch this:

Good point.

>   if test -w /dev/full && test -c /dev/full; then
>   (
>     exec >/dev/tty
>     test -t 1 || exit 0
>     nohup echo hi 2> /dev/full
>     test $? = 125 || fail=1
>     test -f nohup.out || fail=1
>     test -s nohup.out && fail=1
>     rm -f nohup.out
>     exit $fail
>   ) || fail=1
>   fi
> 
> Does someone have a test log file with the failure?

Ondřej Vašík ran the rawhide build again producing:

  ./tests/misc/nohup.sh: line 66: /dev/tty: No such device or address
  + fail=1

So exec >/dev/tty is throwing ENXIO and exiting the subshell.
ENXIO is fine and corresponds to the process not having a controlling tty.

I guessed the change here is with the newly released bash 4.3
which is exiting the subshell immediately in this case, and confirmed
the following fails:

  setsid make SHELL=~/bash-4.3/bash TESTS=tests/misc/nohup.sh SUBDIRS=. check

Now reducing the case and comparing different shells we get:

  $ setsid bash --posix -c 'echo $BASH_VERSION; (exec >/dev/tty; exit 0) || echo failed_early'
  4.2.45(1)-release
  bash: /dev/tty: No such device or address

  $ setsid ./bash --posix -c 'echo $BASH_VERSION; (exec >/dev/tty; exit 0) || echo failed_early'
  4.3.0(1)-release
  ./bash: /dev/tty: No such device or address
  failed_early

  $ setsid ksh -c '(exec >/dev/tty; exit 0) || echo failed_early'
  $ ksh: /dev/tty: cannot create [No such device or address]
  failed_early

  $ setsid dash -c '(exec >/dev/tty; exit 0) || echo failed_early'
  dash: 1: cannot create /dev/tty: No such device or address
  failed_early

So rather than newer bash being the difference here,
it's the older bash that is the outlier.
So we should adjust the test case to handle all shells.
Proposed patch attached.

thanks,
Pádraig.

[nohup-bash-4.3.patch (text/x-patch, attachment)]

Message #20 received at 16940 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Pádraig Brady <P <at> draigBrady.com>, 
 Cyril Roelandt <tipecaml <at> gmail.com>
Cc: 16940 <at> debbugs.gnu.org
Subject: Re: bug#16940: [PATCH] tests: Do not access /dev/tty if it does not
 exist
Date: Wed, 05 Mar 2014 16:43:49 +0100

On 03/05/2014 04:21 PM, Pádraig Brady wrote:
> Ondřej Vašík ran the rawhide build again producing:
>
>    ./tests/misc/nohup.sh: line 66: /dev/tty: No such device or address
>    + fail=1
>
> So exec >/dev/tty is throwing ENXIO and exiting the subshell.
> ENXIO is fine and corresponds to the process not having a controlling tty.
>
> I guessed the change here is with the newly released bash 4.3
> which is exiting the subshell immediately in this case, and confirmed
> the following fails:
>
>    setsid make SHELL=~/bash-4.3/bash TESTS=tests/misc/nohup.sh SUBDIRS=. check
>
> Now reducing the case and comparing different shells we get:
>
>    $ setsid bash --posix -c 'echo $BASH_VERSION; (exec >/dev/tty; exit 0) || echo failed_early'
>    4.2.45(1)-release
>    bash: /dev/tty: No such device or address
>
>    $ setsid ./bash --posix -c 'echo $BASH_VERSION; (exec >/dev/tty; exit 0) || echo failed_early'
>    4.3.0(1)-release
>    ./bash: /dev/tty: No such device or address
>    failed_early
>
>    $ setsid ksh -c '(exec >/dev/tty; exit 0) || echo failed_early'
>    $ ksh: /dev/tty: cannot create [No such device or address]
>    failed_early
>
>    $ setsid dash -c '(exec >/dev/tty; exit 0) || echo failed_early'
>    dash: 1: cannot create /dev/tty: No such device or address
>    failed_early

oh, that's a tricky one.
Thanks for the in-depth analysis.

> So rather than newer bash being the difference here,
> it's the older bash that is the outlier.
> So we should adjust the test case to handle all shells.
> Proposed patch attached.

> +  # POSIX shells immediately exit the subshell on exec error.
> +  # So check we can write to /dev/tty before the exec, which
> +  # isn't possible if we've no controlling tty for example.
> +  >/dev/tty || exit 0
> +

The above '>/dev/tty' would also work if /dev/tty does not exist
and /dev is writable.  Therefore - although the reason of the failure
originates from something completely different -, I think it'd be better
to go back to checking explicitly for a character device again:

  test -c /dev/tty && :>/dev/tty || exit 0

WDYT?

Thanks & have a nice day,
Berny

Message #23 received at 16940 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Cyril Roelandt <tipecaml <at> gmail.com>, Pádraig Brady
 <P <at> draigBrady.com>
Cc: 16940 <at> debbugs.gnu.org
Subject: Re: bug#16940: [PATCH] tests: Do not access /dev/tty if it does not
 exist
Date: Wed, 05 Mar 2014 16:54:13 +0100

On 03/05/2014 04:03 PM, Cyril Roelandt wrote:
> On 03/05/2014 10:17 AM, Bernhard Voelker wrote:
>> Does someone have a test log file with the failure?
>>
>
> The bug was originally reported on GNU Guix, here:
> https://lists.gnu.org/archive/html/guix-devel/2014-03/msg00009.html

That makes clear that you hit the same issue as
Ondrej on rawhide. Thanks!

Have a nice day,
Berny

Message #26 received at 16940 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>
Cc: 16940 <at> debbugs.gnu.org, Cyril Roelandt <tipecaml <at> gmail.com>
Subject: Re: bug#16940: [PATCH] tests: Do not access /dev/tty if it does not
 exist
Date: Wed, 05 Mar 2014 16:46:19 +0000

On 03/05/2014 03:43 PM, Bernhard Voelker wrote:
> On 03/05/2014 04:21 PM, Pádraig Brady wrote:
>> Ondřej Vašík ran the rawhide build again producing:
>>
>>    ./tests/misc/nohup.sh: line 66: /dev/tty: No such device or address
>>    + fail=1
>>
>> So exec >/dev/tty is throwing ENXIO and exiting the subshell.
>> ENXIO is fine and corresponds to the process not having a controlling tty.
>>
>> I guessed the change here is with the newly released bash 4.3
>> which is exiting the subshell immediately in this case, and confirmed
>> the following fails:
>>
>>    setsid make SHELL=~/bash-4.3/bash TESTS=tests/misc/nohup.sh SUBDIRS=. check
>>
>> Now reducing the case and comparing different shells we get:
>>
>>    $ setsid bash --posix -c 'echo $BASH_VERSION; (exec >/dev/tty; exit 0) || echo failed_early'
>>    4.2.45(1)-release
>>    bash: /dev/tty: No such device or address
>>
>>    $ setsid ./bash --posix -c 'echo $BASH_VERSION; (exec >/dev/tty; exit 0) || echo failed_early'
>>    4.3.0(1)-release
>>    ./bash: /dev/tty: No such device or address
>>    failed_early
>>
>>    $ setsid ksh -c '(exec >/dev/tty; exit 0) || echo failed_early'
>>    $ ksh: /dev/tty: cannot create [No such device or address]
>>    failed_early
>>
>>    $ setsid dash -c '(exec >/dev/tty; exit 0) || echo failed_early'
>>    dash: 1: cannot create /dev/tty: No such device or address
>>    failed_early
> 
> oh, that's a tricky one.
> Thanks for the in-depth analysis.
> 
>> So rather than newer bash being the difference here,
>> it's the older bash that is the outlier.
>> So we should adjust the test case to handle all shells.
>> Proposed patch attached.
> 
>> +  # POSIX shells immediately exit the subshell on exec error.
>> +  # So check we can write to /dev/tty before the exec, which
>> +  # isn't possible if we've no controlling tty for example.
>> +  >/dev/tty || exit 0
>> +
> 
> The above '>/dev/tty' would also work if /dev/tty does not exist
> and /dev is writable.  Therefore - although the reason of the failure
> originates from something completely different -, I think it'd be better
> to go back to checking explicitly for a character device again:
> 
>   test -c /dev/tty && :>/dev/tty || exit 0

The test -c is safer. I'll add that.

Note the ':>' is problematic though as the : always returns true.
Well it does for solaris, freebsd, dash, bash-4.3 at least.
In ksh and bash<=4.2 it does return an error as you assumed.
The ':>' construct is only needed for csh anyway I think
which we don't support.

I notice a few more cases: git grep ": *>.*"
which I'll fix in a follow up patch with maybe a syntax check.

thanks,
Pádraig.

Message #29 received at 16940 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>
Cc: 16940 <at> debbugs.gnu.org
Subject: Re: bug#16940: [PATCH] tests: Do not access /dev/tty if it does not
 exist
Date: Wed, 05 Mar 2014 19:05:33 +0000

[Message part 1 (text/plain, inline)]

On 03/05/2014 04:46 PM, Pádraig Brady wrote:
> Note the ':>' is problematic though as the : always returns true.
> Well it does for solaris, freebsd, dash, bash-4.3 at least.
> In ksh and bash<=4.2 it does return an error as you assumed.
> The ':>' construct is only needed for csh anyway I think
> which we don't support.
> 
> I notice a few more cases: git grep ": *>.*"
> which I'll fix in a follow up patch with maybe a syntax check.

Attached.

thanks,
Pádraig.

[leading-colon-redirections.patch (text/x-patch, attachment)]

Message #32 received at 16940 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 16940 <at> debbugs.gnu.org
Subject: Re: bug#16940: [PATCH] tests: Do not access /dev/tty if it does not
 exist
Date: Wed, 05 Mar 2014 23:02:04 +0100

On 03/05/2014 08:05 PM, Pádraig Brady wrote:
> On 03/05/2014 04:46 PM, Pádraig Brady wrote:
>> Note the ':>' is problematic though as the : always returns true.
>> Well it does for solaris, freebsd, dash, bash-4.3 at least.
>> In ksh and bash<=4.2 it does return an error as you assumed.
>> The ':>' construct is only needed for csh anyway I think
>> which we don't support.
>>
>> I notice a few more cases: git grep ": *>.*"
>> which I'll fix in a follow up patch with maybe a syntax check.
> 
> Attached.

Great, looks good.

Thanks & have a nice day,
Berny

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.