grep: fix output duplication when number of matches is greater than MAX_MATCHES #1442
Conversation
…AX_MATCHES When color is enable grep(1) searches for MAX_MATCHES, prints the line and then repeats these steps again untill all matches in the line are found. The issue is that every time grep(1) prints the whole line which leads to the situation when line is duplicated (tripled, quadruple, ...) in stdout. Bug can be reproduced with the next simple command. It prints line twice when only single line is expected. $ for i in $(seq 33); do echo -n "foobar"; done | ./grep --color=auto foo
You should be able to run both test suites, the one from FreeBSD you pointed out and the one from NetBSD ( For example:Tested with Cirrus CIhttps://github.com/jlduran/freebsd-src/tree/gh_1442 |
I see now. So, tests are passing, that's a good news. |
|
I've added a dumb test: It would be good if a regression test is added as well. Given the test is under |
|
This is more or less what I have in mind: --- /dev/null
+++ b/contrib/netbsd-tests/usr.bin/grep/d_color_d.out
@@ -0,0 +1 @@
+�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar�[01;31m�[Kfoo�[m�[Kbar
diff --git a/contrib/netbsd-tests/usr.bin/grep/t_grep.sh b/contrib/netbsd-tests/usr.bin/grep/t_grep.sh
index b1412a7a0715..7b181c1bcc54 100755
--- a/contrib/netbsd-tests/usr.bin/grep/t_grep.sh
+++ b/contrib/netbsd-tests/usr.bin/grep/t_grep.sh
@@ -426,6 +426,12 @@ color_body()
atf_check -o file:"$(atf_get_srcdir)/d_color_c.out" \
grep --color=always -f grepfile "$(atf_get_srcdir)/d_color_b.in"
+ # Begin FreeBSD
+ MAX_MATCHES=32
+ for _ in $(seq $((MAX_MATCHES + 1))); do printf "%s" "foobar"; done > grepfile
+ atf_check -o file:"$(atf_get_srcdir)/d_color_d.out" \
+ grep --color=always foo grepfile
+ # End FreeBSD
}
atf_test_case f_file_empty
diff --git a/usr.bin/grep/tests/Makefile b/usr.bin/grep/tests/Makefile
index b3c79657e53c..1db5ebea5c62 100644
--- a/usr.bin/grep/tests/Makefile
+++ b/usr.bin/grep/tests/Makefile
@@ -12,6 +12,7 @@ ${PACKAGE}FILES+= d_color_a.out
${PACKAGE}FILES+= d_color_b.in
${PACKAGE}FILES+= d_color_b.out
${PACKAGE}FILES+= d_color_c.out
+${PACKAGE}FILES+= d_color_d.out
${PACKAGE}FILES+= d_context2_a.out
${PACKAGE}FILES+= d_context2_b.out
${PACKAGE}FILES+= d_context2_c.out |
|
Thank you, @jlduran. I'll add tests tomorrow. I want to run tests locally to make sure everything is fine. |
|
@jlduran, I've added a commit with the testcase you proposed. Unfortunately I was not able to run tests locally, looks like I messed up with permissions or something. |
| @@ -426,6 +426,13 @@ color_body() | |||
|
|
|||
| atf_check -o file:"$(atf_get_srcdir)/d_color_c.out" \ | |||
| grep --color=always -f grepfile "$(atf_get_srcdir)/d_color_b.in" | |||
|
|
|||
There was a problem hiding this comment.
This empty line is not present in the NetBSD test suite. While aesthetically pleasing, it worsens the diffs when importing the changes from NetBSD.
Without I have the next error by the way. But |
Try changing into the /usr/tests/usr.bin/grep directory and listing the tests: $ cd /usr/tests/usr.bin/grep
$ kyua list
grep_freebsd_test:gnuext
grep_freebsd_test:grep_r_implied
...There should be a # kyua testOr debug a specific test: # kyua debug grep_test:color
Executing command [ grep --color=auto -e .* -e a /usr/tests/usr.bin/grep/d_color_a.in ]
Executing command [ grep --color=auto -f grepfile /usr/tests/usr.bin/grep/d_color_b.in ]
Executing command [ grep --color=always -f grepfile /usr/tests/usr.bin/grep/d_color_b.in ]
Executing command [ grep --color=always foo grepfile ]
grep_test:color -> passed |
|
$ kyua debug grep_test:color
Executing command [ grep --color=auto -e .* -e a /usr/tests/usr.bin/grep/d_color_a.in ]
Executing command [ grep --color=auto -f grepfile /usr/tests/usr.bin/grep/d_color_b.in ]
Executing command [ grep --color=always -f grepfile /usr/tests/usr.bin/grep/d_color_b.in ]
Executing command [ grep --color=always foo grepfile ]
grep_test:color -> passed
$ |
|
jlduran, is there any plans to merge the patch into upstream? |
I hope so, yes! This pull request fixes a bug present in BSD grep (known to be present on FreeBSD and macOS) and documents it using a test. I do not want to triage directly to a CODEOWNER, let's wait a few more days as the group in charge of triaging usually works in waves. Feel free to queue up more fixes if you plan on doing so. Thank you for your patience™. |
|
@kevans91 would you have any time to review the change? |
usr.bin/grep/util.c
Outdated
| @@ -223,6 +225,9 @@ procmatch_match(struct mprintc *mc, struct parsec *pc) | |||
| else | |||
| break; | |||
| } | |||
| pc->matchidx = 0; | |||
There was a problem hiding this comment.
Nit: let's add a blank line above this, please
bsdimp
added
ready
|
The conflicts that were detected were larger than I felt comfortable resolving. This needs to be rebased to proceed. |
|
Sorry, someone had woken up a patch that I had from two or three years ago and I committed that in the interim, which seems to have been the exact problem from here that I'd forgotten about... see 4c9ffb1. We should just extract the test additions and commit those from here, I think. |
|
Additional contet for their problem is here: https://lists.freebsd.org/archives/freebsd-current/2025-March/007176.html |
Can you confirm your test cases against the latest main (or stable/14), please? The problem should be fixed already- if it is, then let's pare this down to your test additions and land that. |
This reverts commit edfc42d.
…r than MAX_MATCHES"
@kevans91, yeah, I can confirm that it has been fixed. |
When color is enable grep(1) searches for MAX_MATCHES, prints the line and then repeats these steps again untill all matches in the line are found. The issue is that every time grep(1) prints the whole line which leads to the situation when line is duplicated (tripled, quadruple, ...) in stdout.
Bug can be reproduced with the next simple command. It prints line twice when only single line is expected.
$ for i in $(seq 33); do echo -n "foobar"; done | ./grep --color=auto foo
By the way, I tried to run tests in
usr.bin/grep/testsfolder usingkyua testcommand onmainbranch and looks like tests are broken or something. Many of them fails withfailed: atf-check failed;message.