* [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 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
* 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
end of thread, other threads:[~2020-01-13 16:42 UTC | newest] 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
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/zsh/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).