zsh-workers
 help / color / Atom feed
* [PATCH] A glob with a trailing slash will now match unreadable/unexecutable directories.
@ 2020-01-13  0:27 ` Daniel Shahaf
  2020-01-13  6:24   ` dana
  2020-01-13  9:46   ` Peter Stephenson
  0 siblings, 2 replies; 5+ messages in thread
From: Daniel Shahaf @ 2020-01-13  0:27 UTC (permalink / raw)
  To: zsh-workers

---
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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] A glob with a trailing slash will now match unreadable/unexecutable directories.
  2020-01-13  0:27 ` [PATCH] A glob with a trailing slash will now match unreadable/unexecutable directories Daniel Shahaf
@ 2020-01-13  6:24   ` dana
  2020-01-13 16:41     ` Daniel Shahaf
  2020-01-13  9:46   ` Peter Stephenson
  1 sibling, 1 reply; 5+ messages in thread
From: dana @ 2020-01-13  6:24 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: Zsh hackers list

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 ,


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] A glob with a trailing slash will now match unreadable/unexecutable directories.
  2020-01-13  0:27 ` [PATCH] A glob with a trailing slash will now match unreadable/unexecutable directories Daniel Shahaf
  2020-01-13  6:24   ` dana
@ 2020-01-13  9:46   ` Peter Stephenson
  2020-01-13 16:34     ` Daniel Shahaf
  1 sibling, 1 reply; 5+ messages in thread
From: Peter Stephenson @ 2020-01-13  9:46 UTC (permalink / raw)
  To: zsh-workers

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] A glob with a trailing slash will now match unreadable/unexecutable directories.
  2020-01-13  9:46   ` Peter Stephenson
@ 2020-01-13 16:34     ` Daniel Shahaf
  0 siblings, 0 replies; 5+ messages in thread
From: Daniel Shahaf @ 2020-01-13 16:34 UTC (permalink / raw)
  To: Peter Stephenson, zsh-workers

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] A glob with a trailing slash will now match unreadable/unexecutable directories.
  2020-01-13  6:24   ` dana
@ 2020-01-13 16:41     ` Daniel Shahaf
  0 siblings, 0 replies; 5+ messages in thread
From: Daniel Shahaf @ 2020-01-13 16:41 UTC (permalink / raw)
  To: zsh-workers

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20200113002821eucas1p2884acfad76834011d777bf81f0562c50@eucas1p2.samsung.com>
2020-01-13  0:27 ` [PATCH] A glob with a trailing slash will now match unreadable/unexecutable directories Daniel Shahaf
2020-01-13  6:24   ` dana
2020-01-13 16:41     ` Daniel Shahaf
2020-01-13  9:46   ` Peter Stephenson
2020-01-13 16:34     ` Daniel Shahaf

zsh-workers

Archives are clonable: git clone --mirror http://inbox.vuxu.org/zsh-workers

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://inbox.vuxu.org/vuxu.archive.zsh.workers


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git