* [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, 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).