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.
Created attachment 215407 [details] patch
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.
The patch looks good to me Would be nice if you could also add some regression tests, do you think you could do that?
Looks reasonable to me as well. It should be relatively easy to add some tests, several already exist in usr.bin/grep/tests.
(In reply to Mark Johnston from comment #4) Good idea; I'll work on a patch to do that
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.)
Let me know if there's something I can do to keep this issue from stalling.
(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.
(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.
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
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
^Triage: assign to committer.
(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.
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.
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