pkgtools/grep bug in slackware[64]-current

chrisVV · 11-06-2016, 05:47 PM

I have some scripts which call up upgradepkg, log anything on stderr to a log file and send anything on stdout to /dev/null. The recent upgrade to grep-2.26 in slackware-current and slackware64-current breaks these scripts.

In particular, if a package test-pkg-0.0.1-noarch-1 is upgraded with the following:

Code:

upgradepkg test-pkg-0.0.1-noarch-2.txz > /dev/null

the new package will be installed but the old one will not be removed. /var/log/packages will show two packages installed, namely (taking the date of an actual test of mine):

test-pkg-0.0.1-noarch-1-upgraded-2016-11-06,23:38:15
test-pkg-0.0.1-noarch-2

The files from both packages will remain installed on the system, so far as the upgraded package does not overwrite the old one.

upgradepkg behaves normally if stdout is not forwarded to /dev/null.

This problem can be resolved by reverting to grep-2.25 from slackware-14.2.

phenixia2003 · 11-07-2016, 03:56 AM

Hello,

I confirm this issue.

There's only one grep call in upgradepkg :

Code:

325:    if [ "$VERBOSE" = "verbose" ]; then
326:      /sbin/removepkg "${rempkg##*/}"
327:    else
328:      /sbin/removepkg "${rempkg##*/}" | grep -v 'Skipping\.\|Removing files:'
329:    fi

To workaround the reported issue, you can run upgradepkg in verbose mode :

Code:

$ upgradepkg --verbose test-pkg-0.0.1-noarch-2.txz > /dev/null
$ ls /var/log/packages/test-pkg
/var/log/packages/test-pkg-0.0.1-noarch-2

The source of that issue is this commit. I rebuild 2.25 with it and got the same issue :

Code:

$ BUILD=1BUGGED ./grep.SlackBuild
grep-2.25/
grep-2.25/ABOUT-NLS
grep-2.25/cfg.mk
...
patching file src/grep.c
...
Slackware package /tmp/grep-2.25-x86_64-1BUGGED.txz created.
...
$ upgradepkg /tmp/grep-2.25-x86_64-1BUGGED.txz
...
$ installpkg ~/test-pkg-0.0.1-noarch-1.txz
...
$ upgradepkg ~/test-pkg-0.0.1-noarch-2.txz > /dev/null
...
$ ls /var/log/packages/test-pkg*
/var/log/packages/test-pkg-0.0.1-noarch-1-upgraded-2016-11-07,11:01:35
/var/log/packages/test-pkg-0.0.1-noarch-2

Edit :
The (dirty) patch below (for grep-2.26) fixes the issue :

Code:

--- src/grep.c.org      2016-11-07 11:15:19.228569445 +0100
+++ src/grep.c  2016-11-07 11:16:26.163567348 +0100
@@ -2703,10 +2703,10 @@
       else if (S_ISCHR (tmp_stat.st_mode))
         {
-          struct stat null_stat;
-          if (stat ("/dev/null", &null_stat) == 0
-              && SAME_INODE (tmp_stat, null_stat))
-            exit_on_match = true;
-          else
+//          struct stat null_stat;
+//          if (stat ("/dev/null", &null_stat) == 0
+//              && SAME_INODE (tmp_stat, null_stat))
+//            exit_on_match = true;
+//          else
             possibly_tty = true;
         }
     }

--
SeB

GazL · 11-07-2016, 04:49 AM

Oh FFS! I wonder how many scripts they're gonna break because of premature SIGPIPEs with this "improvement". pkgtools aren't the only place I've seen grep used in this manner.

BTW, from a quick test on the command-line command | grep -v wibble | cat seems to be a work-around (as long as you don't care about capturing the exit code from the pipe!) as the output of grep will be the next stage of the pipe and not /dev/null.

This is an idiotic change that the GNU grep devs clearly haven't thought through and needs to be reverted upstream.

55020 · 11-07-2016, 05:18 AM

The big clue for the committer should have been that he actually had to commit a change to the regression test

allend · 11-07-2016, 05:20 AM

I like your work.

Actually I like the intent of the patch. The test is poorly constructed.

the3dfxdude · 11-07-2016, 08:06 AM

Um, wait. Isn't the dev screwing around with a pipe thinking it should be doing a "grep -m 1"?

Quote:

Originally Posted by grep committer

This sped up 'seq 10000000000 | grep . >/dev/null' by a factor of 380,000 on my platform (Fedora 23, x86-64, AMD Phenom II X4 910e, en_US.UTF-8 locale).

That is what he justifies this with?

There is already a switch for it. It is better to do the job as originally intended and let the script writer optimize it if it makes sense rather than screwing around with how the pipe is interpreted.

chris.willing · 11-13-2016, 04:32 PM

This bug also affects (and is fixed by the dirty patch above) compilation of vim in -current. See: https://www.linuxquestions.org/quest...ity-4175593362

In brief, with grep-2.26, building vim leaves a number of gcc's intermediate files in /tmp. With grep 2.25, or patched grep-2.26, the garbage files are no longer left in /tmp.

Has anyone reported the grep issue upstream?

chris

CTM · 11-13-2016, 09:04 PM

Quote:

Originally Posted by chris.willing

Has anyone reported the grep issue upstream?

They're aware of it because of the Vim compilation problem (actually with a link back to this thread), but there doesn't seem to be a great rush to do anything about it. There ought to be, because this is embarrassing.

rdsherman · 11-13-2016, 11:46 PM

Quote:

Originally Posted by CTM

[url=https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24941] They ought to be, because this is embarrassing.

Agreed!

I use linux utils like grep everywhere in numerous bash scripts without ever pausing to think there might be a "bug".

Richard

GazL · 11-19-2016, 05:59 AM

Well, looks like they've done some work on this and closed the bug report.

Quote:

grep by default now reads all of standard input if it is a pipe, even if this cannot affect grep's output or exit status. This works better with nonportable scripts that run "PROGRAM | grep PATTERN+ >/dev/null" where PROGRAM dies when writing into a broken pipe
[bug introduced in grep-2.26]

... though I'm not sure what is "nonportable" about using program | grep 'something' in a script. Anyway, that aside, they deserve thanks for fixing it.

Just a matter of waiting for 2.27 now I guess.

kjhambrick · 11-19-2016, 07:38 AM

Thanks again GazL ...

This kind of comment worries me more than a little.

It seems that there are people in the 'upstream world' making changes to the way UNIX and Linux tools have worked for decades and when they break something, it's because we've been doing it wrong all this time.

I like Linus' policy: WE DO NOT BREAK USERSPACE

Sigh ... If only this policy would be applied across the board for all components of a working Linux System ...

-- kjh

chris.willing · 11-20-2016, 05:15 PM

In -current ChangeLog.txt

Code:

Sat Nov 19 22:45:38 UTC 2016
a/grep-2.26-x86_64-2.txz:  Rebuilt.
  Reverted a speedup patch that is causing regressions when output is directed
  to /dev/null.  Thanks to SeB.

rdsherman · 11-20-2016, 11:38 PM

Quote:

Originally Posted by chris.willing

In -current ChangeLog.txt

Code:

Sat Nov 19 22:45:38 UTC 2016
a/grep-2.26-x86_64-2.txz:  Rebuilt.
  Reverted a speedup patch that is causing regressions when output is directed
  to /dev/null.  Thanks to SeB.

Success!

Xsane · 11-21-2016, 01:13 PM

Fixed in this commit.

Quote:

(main): Do not exit on first match merely because output is /dev/null.