[PATCH] A glob with a trailing slash will now match unreadable/unexecutable directories.

[PATCH] A glob with a trailing slash will now match unreadable/unexecutable directories.

[Daniel Shahaf]

---
This fixes the issue, but I'd appreciate some review.

There are some code flow changes, but the basic idea is that instead of
changing "foo/" to "foo/." and calling access(), just leave it as-is and
call stat() and S_ISDIR() instead.

Cheers,

Daniel


 Src/glob.c        | 49 +++++++++++++++++++++++++++++++++++++----------
 Test/D02glob.ztst |  2 +-
 2 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/Src/glob.c b/Src/glob.c
index f67a376b9..bee890caf 100644
--- a/Src/glob.c
+++ b/Src/glob.c
@@ -279,11 +279,11 @@ addpath(char *s, int l)
  * foo/ can be used to reference a non-directory foo.  Returns nonzero if   *
  * the file does not exists.                                                */
 
-/**/
 static int
 statfullpath(const char *s, struct stat *st, int l)
 {
     char buf[PATH_MAX+1];
+    int check_for_being_a_directory = 0;
 
     DPUTS(strlen(s) + !*s + pathpos - pathbufcwd >= PATH_MAX,
 	  "BUG: statfullpath(): pathname too long");
@@ -294,16 +294,44 @@ statfullpath(const char *s, struct stat *st, int l)
 	 * Don't add the '.' if the path so far is empty, since
 	 * then we get bogus empty strings inserted as files.
 	 */
-	buf[pathpos - pathbufcwd] = '.';
-	buf[pathpos - pathbufcwd + 1] = '\0';
-	l = 0;
+	if (st) {
+	    buf[pathpos - pathbufcwd] = '.';
+	    buf[pathpos - pathbufcwd + 1] = '\0';
+	    l = 0;
+	}
+	else {
+	    check_for_being_a_directory = 1;
+	}
     }
     unmetafy(buf, NULL);
-    if (!st) {
+    if (st) {
+	return l ? lstat(buf, st) : stat(buf, st);
+    }
+    else if (check_for_being_a_directory) {
+	struct stat tmp;
+	if (stat(buf, &tmp))
+	    return -1;
+
+	return S_ISDIR(tmp.st_mode) ? 0 : -1;
+    }
+    else {
 	char lbuf[1];
-	return access(buf, F_OK) && (!l || readlink(buf, lbuf, 1) < 0);
+
+	/* If it exists, signal success. */
+	if (access(buf, F_OK) == 0)
+	    return 0;
+
+	/* Would a dangling symlink be good enough? */
+	if (l == 0)
+	    return -1;
+
+	/* Is it a dangling symlink? */
+	if (readlink(buf, lbuf, 1) >= 0)
+	    return 0;
+
+	/* Guess it doesn't exist, then. */
+	return -1;
     }
-    return l ? lstat(buf, st) : stat(buf, st);
 }
 
 /* This may be set by qualifier functions to an array of strings to insert
@@ -327,11 +355,13 @@ insert(char *s, int checked)
     if (gf_listtypes || gf_markdirs) {
 	/* Add the type marker to the end of the filename */
 	mode_t mode;
-	checked = statted = 1;
 	if (statfullpath(s, &buf, 1)) {
 	    unqueue_signals();
 	    return;
 	}
+	else {
+	    checked = statted = 1;
+	}
 	mode = buf.st_mode;
 	if (gf_follow) {
 	    if (!S_ISLNK(mode) || statfullpath(s, &buf2, 0))
@@ -387,11 +417,10 @@ insert(char *s, int checked)
 	    qn = qn->next;
 	}
     } else if (!checked) {
-	if (statfullpath(s, &buf, 1)) {
+	if (statfullpath(s, NULL, 1)) {
 	    unqueue_signals();
 	    return;
 	}
-	statted = 1;
 	news = dyncat(pathbuf, news);
     } else
 	news = dyncat(pathbuf, news);
diff --git a/Test/D02glob.ztst b/Test/D02glob.ztst
index 22c663260..50b0f6716 100644
--- a/Test/D02glob.ztst
+++ b/Test/D02glob.ztst
@@ -734,7 +734,7 @@
  mkdir -m 444 glob.tmp/secret-d444
  for 1 in 000 111 444 ; do ln -s secret-d$1 glob.tmp/secret-s$1; done
  print -rC 2 -- glob.tmp/secret-*/ glob.tmp/secret-*(-/)
--f:unreadable directories can be globbed (users/24619, users/24626)
+0:unreadable directories can be globbed (users/24619, users/24626)
 >glob.tmp/secret-d000/  glob.tmp/secret-d000
 >glob.tmp/secret-d111/  glob.tmp/secret-d111
 >glob.tmp/secret-d444/  glob.tmp/secret-d444

Re: [PATCH] A glob with a trailing slash will now match unreadable/unexecutable directories.

[dana]

On 12 Jan 2020, at 18:27, Daniel Shahaf <danielsh@apache.org> wrote:
> This fixes the issue, but I'd appreciate some review.

The change seems good, though i only looked at it briefly.

A cool thing about it is that it apparently also fixes workers/42891 (globs
misbehaving with sudo on macOS). Not sure if there are cases where it'd still
be an issue, but a simple test (see below) run with root now passes where it
wouldn't before.

There is an issue with the D02 test you added before, though: Making the
directories unwriteable prevents ztst from cleaning up properly afterwards
(unless you're root), which causes other tests to fail the next time i run the
script:

  % make check TESTNUM=D02
  ...
  ./D02glob.ztst: starting.
  ./D02glob.ztst: all tests successful.
  rm: /Users/dana/Development/sf.net/zsh/zsh/Test/glob.tmp/secret-d000: Permission denied
  rm: /Users/dana/Development/sf.net/zsh/zsh/Test/glob.tmp/secret-d111: Permission denied
  rm: /Users/dana/Development/sf.net/zsh/zsh/Test/glob.tmp: Directory not empty
  ...

  % make check TESTNUM=D02
  ...
  ./D02glob.ztst: starting.
  ...
  Test ./D02glob.ztst failed: output differs from expected as shown above for:
    ( regress_absolute_path_and_core_dump )
  Was testing: exclusions regression test
  ./D02glob.ztst: test failed.
  ...

dana


diff --git a/Test/D02glob.ztst b/Test/D02glob.ztst
index 50b0f6716..0d4fde2f9 100644
--- a/Test/D02glob.ztst
+++ b/Test/D02glob.ztst
@@ -741,3 +741,14 @@
 >glob.tmp/secret-s000/  glob.tmp/secret-s000
 >glob.tmp/secret-s111/  glob.tmp/secret-s111
 >glob.tmp/secret-s444/  glob.tmp/secret-s444
+
+ # On macOS, stat(2) allows files to be treated as directories if the calling
+ # process has super-user privileges. e.g., stat() on /my/regular/file/. will
+ # succeed as root but (correctly) fail otherwise. This can produce strange
+ # results when globbing, depending on how it's implemented. This test should,
+ # when run with privileges, confirm that the implementation avoids this
+ # problem. See workers/42891 and workers/45291
+ : > glob.tmp/not-a-directory
+ print -r - glob.tmp/not-a-dir*(N) , glob.tmp/not-a-dir*/(N)
+0:non-directories not globbed as directories
+>glob.tmp/not-a-directory ,

Re: [PATCH] A glob with a trailing slash will now match unreadable/unexecutable directories.

[Daniel Shahaf]

dana wrote on Mon, Jan 13, 2020 at 00:24:22 -0600:
> A cool thing about it is that it apparently also fixes workers/42891 (globs
> misbehaving with sudo on macOS). Not sure if there are cases where it'd still
> be an issue, but a simple test (see below) run with root now passes where it
> wouldn't before.

Cool ☺  Please commit it, then.

> There is an issue with the D02 test you added before, though: Making the
> directories unwriteable prevents ztst from cleaning up properly afterwards
> (unless you're root), which causes other tests to fail the next time i run the
> script:

Thanks for the report.  Does appending $'%clean\n chmod +w
glob.tmp/secret*\n' to the file fix the issue?

To reduce communications overhead, feel free to go ahead and commit my
patch from the start of the thread [to 5.9, as per elsethread] with some
appropriate cleanup that makes it pass in your environment.

Cheers,

Daniel

Re: [PATCH] A glob with a trailing slash will now match unreadable/unexecutable directories.

[Peter Stephenson]

On Mon, 2020-01-13 at 00:27 +0000, Daniel Shahaf wrote:
> ---
> This fixes the issue, but I'd appreciate some review.
> 
> There are some code flow changes, but the basic idea is that instead of
> changing "foo/" to "foo/." and calling access(), just leave it as-is and
> call stat() and S_ISDIR() instead.

Looks like that should do the trick --- next step may be to try it out
and see if any odd edge cases turn up.

Not clear if it's needed in the release since it's not a new issue.

pws

Re: [PATCH] A glob with a trailing slash will now match unreadable/unexecutable directories.

[Daniel Shahaf]

Peter Stephenson wrote on Mon, 13 Jan 2020 09:46 +00:00:
> On Mon, 2020-01-13 at 00:27 +0000, Daniel Shahaf wrote:
> > ---
> > This fixes the issue, but I'd appreciate some review.
> > 
> > There are some code flow changes, but the basic idea is that instead of
> > changing "foo/" to "foo/." and calling access(), just leave it as-is and
> > call stat() and S_ISDIR() instead.
> 
> Looks like that should do the trick --- next step may be to try it out
> and see if any odd edge cases turn up.
> 
> Not clear if it's needed in the release since it's not a new issue.

My thoughts exactly; I'll commit to the 5.9 branch.  Thanks for the review.