Subject: bug#74159: closed (Re: bug#74159: Unexpected exit code of 0 when -q is set and close_stdout fails)

From: [email-masked] (GNU bug Tracking System)
Date: Sat, 02 Nov 2024 06:03:02 +0000

Your bug report

#74159: Unexpected exit code of 0 when -q is set and close_stdout fails

which was filed against the grep package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
74159: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=74159
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#74159: Unexpected exit code of 0 when -q is set and close_stdout fails
From: Paul Eggert <[email-masked]>
To: Jan Černohorský <[email-masked]>
Cc: [email-masked], [email-masked]
Date: Fri, 1 Nov 2024 23:02:11 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

On 2024-11-01 06:10, Jan Černohorský wrote:

> when the `-q` option is 
> detected (grep.c:2697), the `exit_failure` variable is set to 0. This 
> causes any error, when not specially handled, to exit with 0, even 
> before any match is found.

Yes, that's a mistake. I can't reproduce your bug but I can see other 
consequences of the mistake. Fixed in the attached patch (with a test 
case that's unlike your problem); please give it a try.

That being said, the lower levels of your OS appear to be buggy, as 
'close' should not fail with EACCES. I wouldn't be surprised if other 
software breaks because of this issue. So I suggest also reporting a bug 
to whoever maintains the NFS client code that you're using.

I'm boldly closing this bug report; we can reopen it if the patch 
doesn't work for you.

From fce28c4a5eac89e13a992fb54ce4c0e7502f4415 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Fri, 1 Nov 2024 22:38:54 -0700
Subject: [PATCH] grep: fix -q suppression of diagnostics
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Problem reported by Jan Černohorský (Bug#74159).
* src/grep.c (grepbuf): If exit_on_match, set stdout_errno to
avoid screwups on buggy OSes.  Also, ignore errseen since it
cannot be true here.
(main): Do not clear exit_failure if -q is given, as exit status
should be zero only if an input line is selected.
* tests/write-error-msg: Check that -q suppresses diagnostics
of output errors only if a match is found.
---
 src/grep.c            | 12 +++++++++---
 tests/write-error-msg | 11 ++++++++---
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/src/grep.c b/src/grep.c
index 912bce4..042433a 100644
--- a/src/grep.c
+++ b/src/grep.c
@@ -436,7 +436,11 @@ static const struct color_cap color_dict[] =
     { nullptr, nullptr,            nullptr }
   };
 
-/* Saved errno value from failed output functions on stdout.  */
+/* Saved errno value from failed output functions on stdout.
+   prline polls this to decide whether to die.
+   Setting it to nonzero just before exiting can prevent clean_up_stdout
+   from misbehaving on a buggy OS where 'close (STDOUT_FILENO)' fails
+   with EACCES.  */
 static int stdout_errno;
 
 static void
@@ -1490,7 +1494,10 @@ grepbuf (char *beg, char const *lim)
           if (!outleft || done_on_match)
             {
               if (exit_on_match)
-                exit (errseen ? exit_failure : EXIT_SUCCESS);
+                {
+                  stdout_errno = -1;
+                  exit (EXIT_SUCCESS);
+                }
               break;
             }
         }
@@ -2696,7 +2703,6 @@ main (int argc, char **argv)
 
       case 'q':
         exit_on_match = true;
-        exit_failure = 0;
         break;
 
       case 'R':
diff --git a/tests/write-error-msg b/tests/write-error-msg
index c6644e3..351e32c 100755
--- a/tests/write-error-msg
+++ b/tests/write-error-msg
@@ -37,8 +37,11 @@ returns_ 2 grep --line-buffered -v '^$' <in >/dev/full 2>err1 \
 returns_ 2 grep -v '^$' <in >/dev/full 2>err2 \
     || framework_failure_
 
+# disk-full error with -q and --help
+returns_ 2 grep -q --help >/dev/full 2>err3 || fail=1
+
 # ensure each error message file contains a 'write error' with additional text
-for f in err1 err2 ;
+for f in err1 err2 err3 ;
 do
     grep -Eiq '^[^:]*: write error: [a-z]+' $f \
         || {
@@ -49,7 +52,9 @@ do
 done
 
 # These messages should be identical
-compare err1 err2 \
-    || { warn_ "err1,err2 contain different error messages" ; fail=1 ; }
+for f in err2 err3; do
+  compare err1 $f \
+    || { warn_ "err1,$f contain different error messages" ; fail=1 ; }
+done
 
 Exit $fail
-- 
2.47.0

Subject: Unexpected exit code of 0 when -q is set and close_stdout fails
From: Jan Černohorský <[email-masked]>
To: [email-masked]
Cc: [email-masked]
Date: Fri, 1 Nov 2024 14:10:43 +0100
List-Id: <debbugs-submit.debbugs.gnu.org>

Hello,

I've encountered a particularly interesting bug in GNU grep yesterday while 
hunting some issues with LibreOffice (if you're interested in the whole 
post-mortem, it can be read here: 
https://grsc.cz/blog/loffice-linux-issues/, but it's mostly not relevant 
for this bug).

Whenever the `-q` option is used, grep can exit with a 0, even if no 
match has been found and an error has been encountered. In particular, 
when the `close_stdout` function is called and the `close` syscall 
fails, grep exits with 0, even when it's not supposed to.

While reading the source code, I found out that when the `-q` option is 
detected (grep.c:2697), the `exit_failure` variable is set to 0. This 
causes any error, when not specially handled, to exit with 0, even 
before any match is found. This is also true for the `close_stdout` libc 
function, which hard exits using `_exit(exit_failure)` when the syscall 
fails.

This, at first glance, (and pardon me if I'm wrong, I haven't looked 
into the source code that deeply) seems to me as bad design, since it 
makes the code prone to multiple such mistakes – where someone 
inadvertently uses the `exit_failure` variable without realising it 
makes the program return the wrong code. It would make much more sense 
to me (and again, I don't know whether this is possible) to set the 
`exit_failure` variable to 0 only *after* the first match is found, 
preventing such issues altogether. It also makes more sense semantically 
IMO, as the `-q` option states that errors are ignored only when a match 
is found, not always.

As I feel this is more of a design decision than a straightforward fix, 
I'm not sending a patch, but I'll be glad to assist any efforts to 
fix this.

CC'd are my colleagues who helped discover the issue.

Regards,
Jan