Bug 247126
| Summary: | zgrep(1) does not handle -f FILE correctly | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Craig Leres <leres> | ||||||||
| Component: | bin | Assignee: | Craig Leres <leres> | ||||||||
| Status: | Closed FIXED | ||||||||||
| Severity: | Affects Some People | CC: | kevans, markj | ||||||||
| Priority: | --- | Keywords: | needs-qa, regression | ||||||||
| Version: | 12.1-RELEASE | Flags: | koobs:
maintainer-feedback?
(bapt) koobs: maintainer-feedback? (kevans) koobs: mfc-stable12? |
||||||||
| Hardware: | Any | ||||||||||
| OS: | Any | ||||||||||
| Attachments: |
|
||||||||||
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 |
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.