Bug 247126 - zgrep(1) does not handle -f FILE correctly
Summary: zgrep(1) does not handle -f FILE correctly
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 12.1-RELEASE
Hardware: Any Any
: --- Affects Some People
Assignee: Craig Leres
URL:
Keywords: needs-qa, regression
Depends on:
Blocks:
 
Reported: 2020-06-09 21:53 UTC by Craig Leres
Modified: 2020-08-28 13:05 UTC (History)
2 users (show)

See Also:
koobs: maintainer-feedback? (bapt)
koobs: maintainer-feedback? (kevans)
koobs: mfc-stable12?


Attachments
patch (1.16 KB, patch)
2020-06-09 21:53 UTC, Craig Leres
no flags Details | Diff
revised patch (1.66 KB, patch)
2020-06-10 19:42 UTC, Craig Leres
no flags Details | Diff
Add two zgrep test cases (1.22 KB, patch)
2020-06-23 01:03 UTC, Craig Leres
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Craig Leres freebsd_committer 2020-06-09 21:53:08 UTC
Somewhere between FreeBSD 11 and 12, zfgrep and friends became a wrapper shell script. We've found that it does not handle the -f flag correctly:

    # Works
    % zfgrep RELEASE /etc/motd
    FreeBSD 12.1-RELEASE-p3 GENERIC 

    # Hangs reading from stdin
    % echo RELEASE > /tmp/0
    % zfgrep -f /tmp/0 /etc/motd
    /etc/motd:FreeBSD 12.1-RELEASE-p3 GENERIC
    ^Z
    Suspended
    leviathan 89 % ps t
      PID TT  STAT    TIME COMMAND
      11249  4  Ss   0:00.11 -csh (csh)
      12190  4  T    0:00.00 /bin/sh /usr/bin/zfgrep -f /tmp/0 /etc/motd
      12191  4  T    0:00.00 /usr/bin/zcat -f -
      12192  4  T    0:00.00 grep -F -f /tmp/0 -- /etc/motd -
      12193  4  R+   0:00.00 ps t

The problem is when -f is used there is no command line pattern and the wrapper script incorrectly uses the first non-flag argument as a pattern and incorrectly decides we want to read from stdin. Meanwhile grep is invoked with the pattern (really the target file) and a '-'. In then outputs any matches and hangs, reading from stdin.

Here's the most minimal patch I could develop in two minutes.
Comment 1 Craig Leres freebsd_committer 2020-06-09 21:53:49 UTC
Created attachment 215407 [details]
patch
Comment 2 Craig Leres freebsd_committer 2020-06-10 19:42:07 UTC
Created attachment 215428 [details]
revised patch

I see now the wrapper script fails when the flags are not discrete:

    # Works
    % fgrep -we RELEASE /etc/motd
    FreeBSD 12.1-RELEASE-p3 GENERIC

    # Fails
    % zfgrep -we RELEASE /etc/motd
    grep: RELEASE: No such file or directory

This updated patch takes advantage of the fact that "extra argument" flags must come last. It also lets "-e PATTERN" pass through to *grep with other flags instead of moving the pattern to the end.
Comment 3 Baptiste Daroussin freebsd_committer 2020-06-16 12:36:40 UTC
The patch looks good to me

Would be nice if you could also add some regression tests, do you think you could do that?
Comment 4 Mark Johnston freebsd_committer 2020-06-19 17:37:34 UTC
Looks reasonable to me as well.

It should be relatively easy to add some tests, several already exist in usr.bin/grep/tests.
Comment 5 Craig Leres freebsd_committer 2020-06-19 19:51:26 UTC
(In reply to Mark Johnston from comment #4)
Good idea; I'll work on a patch to do that
Comment 6 Craig Leres freebsd_committer 2020-06-23 01:03:00 UTC
Created attachment 215867 [details]
Add two zgrep test cases

Here's a patch that adds tests for the two failure cases. These tests pass under 11.3-RELEASE-p10 and fail under 13.0-CURRENT (r362459). The tests also pass on 13.0-CURRENT when zgrep has been patched.

(Fixing the wrapper script was a lot easier than figuring out how to add the tests! But it's nice to understand how tests work.)
Comment 7 Craig Leres freebsd_committer 2020-07-10 20:24:30 UTC
Let me know if there's something I can do to keep this issue from stalling.
Comment 8 Mark Johnston freebsd_committer 2020-07-10 20:25:44 UTC
(In reply to Craig Leres from comment #7)
Sorry for the delay.  Would you mind putting the combined patch into phabricator?  I will spend time on it during the bug squashing session tomorrow.
Comment 9 Craig Leres freebsd_committer 2020-07-10 21:39:49 UTC
(In reply to Mark Johnston from comment #8)
Done:

    https://reviews.freebsd.org/D25613

Let me know if I made any mistakes, this is most certainly my first phabricator in head.
Comment 10 Craig Leres freebsd_committer 2020-07-11 23:25:55 UTC
Here are some additional regressions the review has turned up (so far):

Flag processing ends after the first -e PATTERN:

    echo foobar > test

    grep -e foo -i < test
    foobar

    zgrep -e foo -i < test
    zcat: can't stat: -i: No such file or directory

This bug hides another bug; multiple -e patterns are supposed to behave
like multiple patterns in a -f file:

    echo foobar > test6
    echo howdy >> test6

    grep -e foo -e how test6
    foobar
    howdy

The zgrep wrapper only uses the last -e pattern seen.

Long flags that end with certain characters:

    echo foobar > test

    grep -e foo --ignore-case < test
    foobar

    zgrep -e foo --ignore-case < test
    zcat: can't stat: --ignore-case: No such file or directory

Long flags that take arguments are problematic as well:

    echo tiresome > test5
    grep --regexp=some  test5
    tiresome

    zgrep --regexp=some test5 < /dev/null
    test5:tiresome
    zcat: (stdin): unexpected end of file

No whitespace between a flag and its argument:
    
    echo foobar > test
 
    grep -efoo test
    foobar
    
    zgrep -efoo test < /dev/null
    zcat: (stdin): unexpected end of file
    test:foobar
Comment 11 commit-hook freebsd_committer 2020-07-20 23:58:08 UTC
A commit references this bug:

Author: leres
Date: Mon Jul 20 23:57:54 UTC 2020
New revision: 363381
URL: https://svnweb.freebsd.org/changeset/base/363381

Log:
  Fix some regressions with the zgrep(1) wrapper.

   - Handle whitespace with long flags that take arguments:

  	echo 'foo bar' > test
  	zgrep --regexp='foo bar' test

   - Do not hang reading from stdin with patterns in a file:

  	echo foobar > test
  	echo foo > pattern
  	zgrep -f pattern test
  	zgrep --file=pattern test

   - Handle any flags after -e:

  	echo foobar > test
  	zgrep -e foo --ignore-case < test

  These two are still outstanding problems:

   - Does not handle flags that take an argument if there is no
     whitespace:

  	zgrep -enfs /etc/rpc

   - When more than one -e pattern used matching should occur for all
     patterns (similar to multiple patterns supplied with -f file).
     Instead only the last pattern is used for matching:

  	zgrep -e rex -e nfs /etc/rpc

     (This problem is masked in the unpatched version by the "any
     flags after -e" problem.)

  Add tests for the above problems.

  Update the mange and add references to gzip(1) and zstd(1) and also
  document the remaining known problems.

  PR:		247126
  Approved by:	markj
  MFC after:	2 weeks
  Differential Revision:	https://reviews.freebsd.org/D25613

Changes:
  head/contrib/netbsd-tests/usr.bin/grep/t_grep.sh
  head/usr.bin/grep/zgrep.1
  head/usr.bin/grep/zgrep.sh
Comment 12 Mark Linimon freebsd_committer freebsd_triage 2020-07-30 23:18:50 UTC
^Triage: assign to committer.
Comment 13 Mark Johnston freebsd_committer 2020-08-20 17:31:33 UTC
(In reply to commit-hook from comment #11)
Any reason not to merge this to stable/12?  I haven't seen any fallout from it.  I'm happy to do the merge.
Comment 14 Craig Leres freebsd_committer 2020-08-20 17:55:11 UTC
I wanted to do the merge but am unclear on the procedure. I'll contact you offline later today with what I've believe the steps are.
Comment 15 commit-hook freebsd_committer 2020-08-23 17:46:58 UTC
A commit references this bug:

Author: leres
Date: Sun Aug 23 17:46:11 UTC 2020
New revision: 364502
URL: https://svnweb.freebsd.org/changeset/base/364502

Log:
  MFC r363381:
  Fix some regressions with the zgrep(1) wrapper.

  PR:		247126

Changes:
_U  stable/12/
  stable/12/contrib/netbsd-tests/usr.bin/grep/t_grep.sh
  stable/12/usr.bin/grep/zgrep.1
  stable/12/usr.bin/grep/zgrep.sh