Bug 280676 - grep default recursive behavior differs from manpage
Summary: grep default recursive behavior differs from manpage
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 14.1-STABLE
Hardware: Any Any
: --- Affects Many People
Assignee: John Baldwin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-08-07 18:31 UTC by Radosław Piliszek
Modified: 2024-12-01 05:01 UTC (History)
4 users (show)

See Also:
jhb: mfc-stable14+
jhb: mfc-stable13+


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Radosław Piliszek 2024-08-07 18:31:01 UTC
FreeBSD's `grep(1)` describes that `-p` is the default when using `-R`, i.e., that symlinks are *not* followed. However, it seems that actually `-S` is the default, i.e., symlinks *are* followed (when using only `-R`). Notably, the man page is internally coherent (both `-p` and `-S` descriptions claim that `-p` is the default).

I don't know FreeBSD's previous behavior of grep so cannot comment whether it's the man page that is supposed to be wrong or grep's behavior but they certainly disagree on the matter.

Bug found on Chimera Linux and verified on FreeBSD 14.1-STABLE which is the source of Chimera Linux's base utils at the moment.
Comment 1 Ed Maste freebsd_committer freebsd_triage 2024-08-09 18:08:39 UTC
FYI, from GNU grep's man page:

       -r, --recursive
              Read all files under each directory, recursively, following
              symbolic links only if they are on the command line.  Note that
              if no file operand is given, grep searches the working
              directory.  This is equivalent to the -d recurse option.

       -R, --dereference-recursive
              Read all files under each directory, recursively.  Follow all
              symbolic links, unlike -r.
Comment 2 Radosław Piliszek 2024-08-09 18:31:44 UTC
(In reply to Ed Maste from comment #1)

Please note the bug report is not about GNU's grep but FreeBSD's grep where `-R` = `-r` (indeed, you could replace all `-R` with `-r` in my report and have it still hold true).
Comment 3 John Baldwin freebsd_committer freebsd_triage 2024-08-09 18:33:32 UTC
Looks like macOS uses BSD grep, and it fixed the code to match the manpage.  BSD grep treats '-r' and '-R' as identical, whereas GNU grep uses '-r' to mean the equivalent of '-rO' in BSD grep and '-R' to mean the equivalent of '-rS'.

However, it seems like the grep in FreeBSD is a bit more broken.  To test, I created a hierarchy like so:

```
> ls -l
total 2
drwxr-xr-x  2 john john  3 Aug  9 14:20 bar
drwxr-xr-x  2 john john  3 Aug  9 14:20 baz
lrwxr-xr-x  1 john john 11 Aug  9 14:20 foo -> bar/foo.txt
> ls -l bar/
total 1
-rw-r--r--  1 john john 5 Aug  9 14:19 foo.txt
> ls -l baz
total 1
lrwxr-xr-x  1 john john 14 Aug  9 14:20 bar.txt -> ../bar/foo.txt
> cat bar/foo.txt 
blah
```

I then tested grep -r on macOS with various options which gave the following:

```
> grep -r blah foo bar baz
bar/foo.txt:blah

> grep -rS blah foo bar baz
foo:blah
bar/foo.txt:blah
baz/bar.txt:blah

> grep -rO blah foo bar baz
foo:blah
bar/foo.txt:blah
```

On FreeBSD it seems that the options don't really work at all:

```
> grep -rS blah foo bar baz
foo:blah
bar/foo.txt:blah
baz/bar.txt:blah

> grep -rO blah foo bar baz
foo:blah
bar/foo.txt:blah
baz/bar.txt:blah

> grep -rp blah foo bar baz
foo:blah
bar/foo.txt:blah
baz/bar.txt:blah
```

That is to say, it acts like -S is always enabled.  One issue is that when -O is specified, we only include FTS_COMFOLLOW in the flags passed to fts_open() in grep_tree(), when it should be FTS_COMFOLLOW | FTS_PHYSICAL in that case (the fts manpage says that one of FTS_LOGICAL or FTS_PHYSICAL must be specified).  But it is also true that in the code the default setting of 'link_behave' is indeed -S.
Comment 4 John Baldwin freebsd_committer freebsd_triage 2024-08-09 18:59:52 UTC
I've debugged this further and uploaded two reviews: one to fix grep to not unconditionally follow symlinks (https://reviews.freebsd.org/D46255), and a second to flip the default (https://reviews.freebsd.org/D46256).
Comment 5 Radosław Piliszek 2024-08-09 19:36:57 UTC
(In reply to John Baldwin from comment #3)

Thank you for your extensive testing.

Just to add to your comment - the `-p` is *not entirely broken* when used currently. It works fine for my simple use case where symlinks were only between directories.
Comment 6 commit-hook freebsd_committer freebsd_triage 2024-09-04 19:55:00 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=fc12c191c087b63e1204fee210ba76082ea40b96

commit fc12c191c087b63e1204fee210ba76082ea40b96
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2024-09-04 19:53:22 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2024-09-04 19:53:22 +0000

    grep: Default to -p instead of -S.

    This matches the documented behavior in the manpage as well as the
    default behavior on macOS.

    PR:             280676
    Reported by:    Radosław Piliszek <radoslaw.piliszek@gmail.com>
    Reviewed by:    kevans
    MFC after:      1 week
    Differential Revision:  https://reviews.freebsd.org/D46256

 usr.bin/grep/grep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 7 commit-hook freebsd_committer freebsd_triage 2024-09-04 19:55:08 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=77eb877714d69ee0279d70eb3331920fba90db95

commit 77eb877714d69ee0279d70eb3331920fba90db95
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2024-09-04 19:53:17 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2024-09-04 19:53:17 +0000

    grep: Fix various bugs in recursive tree handling

    The -OpS options were effectively ignored due to a collection of
    bugs in the use of fts(3):

    - fts_open(3) requires one of FTS_PHYSICAL or FTS_LOGICAL to be
      specified, but in the -O case, only FTS_COMFOLLOW was given.  Fix
      this to use FTS_COMFOLLOW | FTS_PHYSICAL.

    - The switch on the entry type returned by fts_read() did not check
      for symbolic links, so symbolic links fell into the default case and
      were always passed to procfile() even when -p was given.  Fix this
      by adding cases in the switch statement to explicitly ignore FTS_SL.

    - FTS_NOSTAT was passed to fts_open(), so fts_open() couldn't detect
      symbolic links when FTS_PHYSICAL was passed, instead both regular
      files and symbolic links were returned as FTS_NSOK entries.  Fix
      by only using FTS_NOSTAT with FTS_LOGICAL.

    While here, fix a few other nits:

    - Treat FTS_NS as an error like FTS_DNR and FTS_ERR.

    - Just ignore FTS_DP.  The logic to skip descending into skipped
      directories is only relevant when a directory is first visited, not
      after the directory has been visited.

    - Use warnc instead of warnx + strerror.

    PR:             280676
    Reviewed by:    kevans
    MFC after:      1 week
    Differential Revision:  https://reviews.freebsd.org/D46255

 usr.bin/grep/util.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)
Comment 8 Mark Johnston freebsd_committer freebsd_triage 2024-11-11 23:46:53 UTC
John, do you still plan to MFC those commits?
Comment 9 John Baldwin freebsd_committer freebsd_triage 2024-11-12 15:32:14 UTC
Yes, I should.  I do need to do an update to RELNOTES in main like you asked still.
Comment 10 commit-hook freebsd_committer freebsd_triage 2024-11-30 21:25:36 UTC
A commit in branch stable/14 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=83f1b98d822704f87e28f8e0016c3169d75ae87f

commit 83f1b98d822704f87e28f8e0016c3169d75ae87f
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2024-09-04 19:53:22 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2024-11-30 20:43:28 +0000

    grep: Default to -p instead of -S.

    This matches the documented behavior in the manpage as well as the
    default behavior on macOS.

    PR:             280676
    Reported by:    Radosław Piliszek <radoslaw.piliszek@gmail.com>
    Reviewed by:    kevans
    MFC after:      1 week
    Differential Revision:  https://reviews.freebsd.org/D46256

    (cherry picked from commit fc12c191c087b63e1204fee210ba76082ea40b96)

 usr.bin/grep/grep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 11 commit-hook freebsd_committer freebsd_triage 2024-11-30 21:25:41 UTC
A commit in branch stable/14 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=2e18c66924ed0a5af36e01b9bf2e332138da6974

commit 2e18c66924ed0a5af36e01b9bf2e332138da6974
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2024-09-04 19:53:17 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2024-11-30 20:43:14 +0000

    grep: Fix various bugs in recursive tree handling

    The -OpS options were effectively ignored due to a collection of
    bugs in the use of fts(3):

    - fts_open(3) requires one of FTS_PHYSICAL or FTS_LOGICAL to be
      specified, but in the -O case, only FTS_COMFOLLOW was given.  Fix
      this to use FTS_COMFOLLOW | FTS_PHYSICAL.

    - The switch on the entry type returned by fts_read() did not check
      for symbolic links, so symbolic links fell into the default case and
      were always passed to procfile() even when -p was given.  Fix this
      by adding cases in the switch statement to explicitly ignore FTS_SL.

    - FTS_NOSTAT was passed to fts_open(), so fts_open() couldn't detect
      symbolic links when FTS_PHYSICAL was passed, instead both regular
      files and symbolic links were returned as FTS_NSOK entries.  Fix
      by only using FTS_NOSTAT with FTS_LOGICAL.

    While here, fix a few other nits:

    - Treat FTS_NS as an error like FTS_DNR and FTS_ERR.

    - Just ignore FTS_DP.  The logic to skip descending into skipped
      directories is only relevant when a directory is first visited, not
      after the directory has been visited.

    - Use warnc instead of warnx + strerror.

    PR:             280676
    Reviewed by:    kevans
    MFC after:      1 week
    Differential Revision:  https://reviews.freebsd.org/D46255

    (cherry picked from commit 77eb877714d69ee0279d70eb3331920fba90db95)

 usr.bin/grep/util.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)
Comment 12 commit-hook freebsd_committer freebsd_triage 2024-11-30 21:25:42 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=6ce090af28326d9c25a4258278a69952b41e2091

commit 6ce090af28326d9c25a4258278a69952b41e2091
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2024-09-04 19:53:17 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2024-11-30 20:43:20 +0000

    grep: Fix various bugs in recursive tree handling

    The -OpS options were effectively ignored due to a collection of
    bugs in the use of fts(3):

    - fts_open(3) requires one of FTS_PHYSICAL or FTS_LOGICAL to be
      specified, but in the -O case, only FTS_COMFOLLOW was given.  Fix
      this to use FTS_COMFOLLOW | FTS_PHYSICAL.

    - The switch on the entry type returned by fts_read() did not check
      for symbolic links, so symbolic links fell into the default case and
      were always passed to procfile() even when -p was given.  Fix this
      by adding cases in the switch statement to explicitly ignore FTS_SL.

    - FTS_NOSTAT was passed to fts_open(), so fts_open() couldn't detect
      symbolic links when FTS_PHYSICAL was passed, instead both regular
      files and symbolic links were returned as FTS_NSOK entries.  Fix
      by only using FTS_NOSTAT with FTS_LOGICAL.

    While here, fix a few other nits:

    - Treat FTS_NS as an error like FTS_DNR and FTS_ERR.

    - Just ignore FTS_DP.  The logic to skip descending into skipped
      directories is only relevant when a directory is first visited, not
      after the directory has been visited.

    - Use warnc instead of warnx + strerror.

    PR:             280676
    Reviewed by:    kevans
    MFC after:      1 week
    Differential Revision:  https://reviews.freebsd.org/D46255

    (cherry picked from commit 77eb877714d69ee0279d70eb3331920fba90db95)

 usr.bin/grep/util.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)
Comment 13 commit-hook freebsd_committer freebsd_triage 2024-11-30 21:25:43 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=35b193572545e09682f56494ac445b58dd89697c

commit 35b193572545e09682f56494ac445b58dd89697c
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2024-09-04 19:53:22 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2024-11-30 20:43:34 +0000

    grep: Default to -p instead of -S.

    This matches the documented behavior in the manpage as well as the
    default behavior on macOS.

    PR:             280676
    Reported by:    Radosław Piliszek <radoslaw.piliszek@gmail.com>
    Reviewed by:    kevans
    MFC after:      1 week
    Differential Revision:  https://reviews.freebsd.org/D46256

    (cherry picked from commit fc12c191c087b63e1204fee210ba76082ea40b96)

 usr.bin/grep/grep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)