[Bug] Strange Globing Behaviour when used with sudo

[Bug] Strange Globing Behaviour when used with sudo

[Bengt Brodersen]

[-- Attachment #1: Type: text/plain, Size: 368 bytes --]

*Issue Example*

If you create a dummy file and dummy folder

```
mkdir -p folder
touch file
```

...,if you want to print only folders globing works as expected
```
echo ./*/
>> ./folder/
```

..., however if you run the same command with sudo it will also print the
file, as a folder.
```
sudo zsh -c 'echo ./*/'
>> ./file/ ./folder/
```

Why zsh behaves like this?

Re: [Bug] Strange Globing Behaviour when used with sudo

[Bart Schaefer]

On Wed, May 30, 2018 at 10:11 AM, Bengt Brodersen
<bengt.brodersen@gmail.com> wrote:
>
> ```
> echo ./*/
>>> ./folder/
> ```
>
> ```
> sudo zsh -c 'echo ./*/'
>>> ./file/ ./folder/
> ```

I am not able to reproduce this.  More info about your execution
environment, please (operating system, etc.)  Also, does it happen
with "zsh -fc ..." ?

Re: [Bug] Strange Globing Behaviour when used with sudo

[Daniel Shahaf]

Bart Schaefer wrote on Wed, 30 May 2018 11:06 -0700:
> On Wed, May 30, 2018 at 10:11 AM, Bengt Brodersen
> <bengt.brodersen@gmail.com> wrote:
> >
> > ```
> > echo ./*/
> >>> ./folder/
> > ```
> >
> > ```
> > sudo zsh -c 'echo ./*/'
> >>> ./file/ ./folder/
> > ```
> 
> I am not able to reproduce this.  More info about your execution
> environment, please (operating system, etc.)  Also, does it happen
> with "zsh -fc ..." ?

Bengt reported the same issue to z-sy-h
(https://github.com/zsh-users/zsh-syntax-highlighting/pull/518), there
he said he was using:

- macOS 10.13.4 (17E202)
- zsh 5.5.1 (x86_64-apple-darwin17.5.0)

and that the issue still occurred under -fc.

(Bengt: you were correct to report the zsh issue here, but you should
have cross-linked the two threads, like I just did.)

Cheers,

Daniel

Re: [Bug] Strange Globing Behaviour when used with sudo

[Phil Pennock]

On 2018-05-30 at 19:15 +0000, Daniel Shahaf wrote:
> > On Wed, May 30, 2018 at 10:11 AM, Bengt Brodersen
> > > sudo zsh -c 'echo ./*/'
> > >>> ./file/ ./folder/

> - macOS 10.13.4 (17E202)
> - zsh 5.5.1 (x86_64-apple-darwin17.5.0)
> 
> and that the issue still occurred under -fc.

This appears to be macOS returning different results for stat64() for
root vs non-root.  Using dtruss:

non-root:
open_nocancel("./\0", 0x1100004, 0x10040A3F8)		 = 3 0
fstatfs64(0x3, 0x7FFF5F8A1338, 0x10040A3F8)		 = 0 0
getdirentries64(0x3, 0x7FA30F805E00, 0x1000)		 = 112 0
getdirentries64(0x3, 0x7FA30F805E00, 0x1000)		 = 0 0
close_nocancel(0x3)		 = 0 0
stat64("./dummy/.\0", 0x7FFF5F8A1948, 0x1000)		 = -1 Err#20
stat64("./folder/.\0", 0x7FFF5F8A1948, 0x1000)		 = 0 0

root:
open_nocancel("./\0", 0x1100004, 0x10B5F8B28)		 = 3 0
fstatfs64(0x3, 0x7FFF547471B8, 0x10B5F8B28)		 = 0 0
getdirentries64(0x3, 0x7FAFD9821400, 0x1000)		 = 112 0
getdirentries64(0x3, 0x7FAFD9821400, 0x1000)		 = 0 0
close_nocancel(0x3)		 = 0 0
stat64("./dummy/.\0", 0x7FFF547477C8, 0x1000)		 = 0 0
stat64("./folder/.\0", 0x7FFF547477C8, 0x1000)		 = 0 0

<sys/errno.h> defines ENOTDIR as 20.

-Phil

Re: [Bug] Strange Globing Behaviour when used with sudo

[Bengt Brodersen]

[-- Attachment #1: Type: text/plain, Size: 1568 bytes --]

If you google *mac stat64 deprecated* you will get a lot of results.
So maybe it caused by the depreciation on darwin os

On Wed, 30 May 2018 at 22:24 Phil Pennock <
zsh-workers+phil.pennock@spodhuis.org> wrote:

> On 2018-05-30 at 19:15 +0000, Daniel Shahaf wrote:
> > > On Wed, May 30, 2018 at 10:11 AM, Bengt Brodersen
> > > > sudo zsh -c 'echo ./*/'
> > > >>> ./file/ ./folder/
>
> > - macOS 10.13.4 (17E202)
> > - zsh 5.5.1 (x86_64-apple-darwin17.5.0)
> >
> > and that the issue still occurred under -fc.
>
> This appears to be macOS returning different results for stat64() for
> root vs non-root.  Using dtruss:
>
> non-root:
> open_nocancel("./\0", 0x1100004, 0x10040A3F8)            = 3 0
> fstatfs64(0x3, 0x7FFF5F8A1338, 0x10040A3F8)              = 0 0
> getdirentries64(0x3, 0x7FA30F805E00, 0x1000)             = 112 0
> getdirentries64(0x3, 0x7FA30F805E00, 0x1000)             = 0 0
> close_nocancel(0x3)              = 0 0
> stat64("./dummy/.\0", 0x7FFF5F8A1948, 0x1000)            = -1 Err#20
> stat64("./folder/.\0", 0x7FFF5F8A1948, 0x1000)           = 0 0
>
> root:
> open_nocancel("./\0", 0x1100004, 0x10B5F8B28)            = 3 0
> fstatfs64(0x3, 0x7FFF547471B8, 0x10B5F8B28)              = 0 0
> getdirentries64(0x3, 0x7FAFD9821400, 0x1000)             = 112 0
> getdirentries64(0x3, 0x7FAFD9821400, 0x1000)             = 0 0
> close_nocancel(0x3)              = 0 0
> stat64("./dummy/.\0", 0x7FFF547477C8, 0x1000)            = 0 0
> stat64("./folder/.\0", 0x7FFF547477C8, 0x1000)           = 0 0
>
> <sys/errno.h> defines ENOTDIR as 20.
>
> -Phil
>

Re: [Bug] Strange Globing Behaviour when used with sudo

[Bengt Brodersen]

[-- Attachment #1: Type: text/plain, Size: 5583 bytes --]

Just found a paragraph in stat(2) manpage on macos

_DARWIN_FEATURE_64_BIT_INODE
     In order to accommodate advanced capabilities of newer file systems,
the struct stat, struct statfs, and struct dirent data structures were
updated in Mac OSX 10.5.

     The most obvious change is the increased size of ino_t from 32 bits to
64 bits.  As a consequence, storing an ino_t in an int is no longer safe,
and file formats storing ino_t as 32-bit values may need to be
     updated.  There are other changes as well, such as the widening of
f_fstypename, f_mntonname, and f_mntfromname in struct statfs.  Please
refer to stat(2) and dir(5) for more detail on the specific changes to
     the other affected data structures.

     On platforms that existed before these updates were available, ABI
compatibility is achieved by providing two implementations for related
functions: one using the legacy data structures and one using the
     updated data structures.  Variants which make use of the newer
structures have their symbols suffixed with $INODE64.  These $INODE64
suffixes are automatically appended by the compiler tool-chain and should
     not be used directly.

     Platforms that were released after these updates only have the newer
variants available to them.  These platforms have the macro
_DARWIN_FEATURE_ONLY_64_BIT_INODE defined.

     The _DARWIN_FEATURE_64_BIT_INODE macro should not be set directly.
Instead, developers should make use of the _DARWIN_NO_64_BIT_INODE or
_DARWIN_USE_64_BIT_INODE macros when the default variant is not
     desired.  The following table details the effects of defining these
macros for different deployment targets.


          _DARWIN_FEATURE_ONLY_64_BIT_INODE not defined

     -------------------------+-------------------------------

                              |       Deployment Target

          user defines:       |   < 10.5       10.5    > 10.5

     -------------------------+-------------------------------

              (none)          |   32-bit      32-bit   64-bit

     _DARWIN_NO_64_BIT_INODE  |   32-bit      32-bit   32-bit

     _DARWIN_USE_64_BIT_INODE |   32-bit      64-bit   64-bit

     -------------------------+-------------------------------


            _DARWIN_FEATURE_ONLY_64_BIT_INODE defined

     -------------------------+-------------------------------

          user defines:       | Any Deployment Target

     -------------------------+-------------------------------

              (none)          | 64-bit-only

     _DARWIN_NO_64_BIT_INODE  |   (error)

     _DARWIN_USE_64_BIT_INODE | 64-bit-only

     -------------------------+-------------------------------

           32-bit       32-bit inode values are enabled, and the legacy
structures involving the ino_t type are in use.  The macro
_DARWIN_FEATURE_64_BIT_INODE is not defined.

           64-bit       64-bit inode values are enabled, and the expanded
structures involving the ino_t type are in use.  The macro
_DARWIN_FEATURE_64_BIT_INODE is defined, and loader symbols will contain the
                        $INODE64 suffix.

           64-bit-only  Like 64-bit, except loader symbols do not have the
$INODE64 suffix.

           (error)      A compile time error is generated.

     Due to the increased benefits of the larger structure, it is highly
recommended that developers not define _DARWIN_NO_64_BIT_INODE and make use
of _DARWIN_USE_64_BIT_INODE when targeting Mac OSX 10.5.

     In addition to the $INODE64 suffixed symbols, variants suffixed with
64 are also available for related functions.  These functions were provided
as a way for developers to use the updated structures in code
     that also made use of the legacy structures.  The enlarged stat
structures were also prefixed with 64 to distinguish them from their legacy
variants.  These functions have been deprecated and should be
     avoided.

On Wed, 30 May 2018 at 22:44 Bengt Brodersen <bengt.brodersen@gmail.com>
wrote:

> If you google *mac stat64 deprecated* you will get a lot of results.
> So maybe it caused by the depreciation on darwin os
>
> On Wed, 30 May 2018 at 22:24 Phil Pennock <
> zsh-workers+phil.pennock@spodhuis.org> wrote:
>
>> On 2018-05-30 at 19:15 +0000, Daniel Shahaf wrote:
>> > > On Wed, May 30, 2018 at 10:11 AM, Bengt Brodersen
>> > > > sudo zsh -c 'echo ./*/'
>> > > >>> ./file/ ./folder/
>>
>> > - macOS 10.13.4 (17E202)
>> > - zsh 5.5.1 (x86_64-apple-darwin17.5.0)
>> >
>> > and that the issue still occurred under -fc.
>>
>> This appears to be macOS returning different results for stat64() for
>> root vs non-root.  Using dtruss:
>>
>> non-root:
>> open_nocancel("./\0", 0x1100004, 0x10040A3F8)            = 3 0
>> fstatfs64(0x3, 0x7FFF5F8A1338, 0x10040A3F8)              = 0 0
>> getdirentries64(0x3, 0x7FA30F805E00, 0x1000)             = 112 0
>> getdirentries64(0x3, 0x7FA30F805E00, 0x1000)             = 0 0
>> close_nocancel(0x3)              = 0 0
>> stat64("./dummy/.\0", 0x7FFF5F8A1948, 0x1000)            = -1 Err#20
>> stat64("./folder/.\0", 0x7FFF5F8A1948, 0x1000)           = 0 0
>>
>> root:
>> open_nocancel("./\0", 0x1100004, 0x10B5F8B28)            = 3 0
>> fstatfs64(0x3, 0x7FFF547471B8, 0x10B5F8B28)              = 0 0
>> getdirentries64(0x3, 0x7FAFD9821400, 0x1000)             = 112 0
>> getdirentries64(0x3, 0x7FAFD9821400, 0x1000)             = 0 0
>> close_nocancel(0x3)              = 0 0
>> stat64("./dummy/.\0", 0x7FFF547477C8, 0x1000)            = 0 0
>> stat64("./folder/.\0", 0x7FFF547477C8, 0x1000)           = 0 0
>>
>> <sys/errno.h> defines ENOTDIR as 20.
>>
>> -Phil
>>
>

Re: [Bug] Strange Globing Behaviour when used with sudo

[Peter Stephenson]

On Wed, 30 May 2018 22:49:32 +0200
Bengt Brodersen <bengt.brodersen@gmail.com> wrote:
> Just found a paragraph in stat(2) manpage on macos

If I've read this correctly, it sounds like it's worth trying to

#define _DARWIN_NO_64_BIT_INODE

in the zsh configuration just to see what happens (and then "make
clean" and "make").  This should have one of two effects (i) ino_t
becomes 32 bits, which works around (but does not properly fix) the
problem (ii) the compilation will fail because 32-bit sizes aren't
available in that configuration, which doesn't confirm this is
necessarily a problem but might be a hint.

If it is the issue, it's going to take some detective work by someone on
MacOS as grepping the code doesn't reveal an obviously missing use of
ino_t in the zsh code --- worth checking if ino_t is actually
correctly typedef'd.

If that's not the issue, then back to the drawing board.

pws

Re: [Bug] Strange Globing Behaviour when used with sudo

[Peter Stephenson]

On Thu, 31 May 2018 09:44:03 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> If it is the issue, it's going to take some detective work by someone
> on MacOS as grepping the code doesn't reveal an obviously missing use
> of ino_t in the zsh code --- worth checking if ino_t is actually
> correctly typedef'd.

Also, worth doing some easy checks anyway in config.h.  There's an
INO_T_IS_64_BIT definition that should be set, is it?  If not, how about
trying the other configuration definition

#define _DARWIN_USE_64_BIT_INODE

and see if that fixes the matter?  That would indicate the problem
wasn't that ino_t wasn't being used correctly in the code, just that
the type as defined wasn't large enough for certain situations in
configuration.

(Apologies if I'm teaching my grandmother to suck eggs but the thread so
far didn't suggest anyone on MacOS was actually looking down here.)

pws

Re: [Bug] Strange Globing Behaviour when used with sudo

[Jun T]

> 2018/05/31 17:44, Peter Stephenson <p.stephenson@samsung.com> wrote:
> 
> On Wed, 30 May 2018 22:49:32 +0200
> Bengt Brodersen <bengt.brodersen@gmail.com> wrote:
>> Just found a paragraph in stat(2) manpage on macos
> 
> If I've read this correctly, it sounds like it's worth trying to
> 
> #define _DARWIN_NO_64_BIT_INODE

I think the problem is not related with these macros (or inode size),
but anyway I manually added

#define _DARWIN_NO_64_BIT_INODE
#undef _DARWIN_USE_64_BIT_INODE   (this is defined by AC_SYS_LARGEFILE)

to config.h and built zsh; still the same problem for root.

As Phil Pennock pointed out, the problem originates from:

> 2018/05/31 5:23, Phil Pennock <zsh-workers+phil.pennock@spodhuis.org> wrote:
> non-root:
> stat64("./dummy/.\0", 0x7FFF5F8A1948, 0x1000)		 = -1 Err#20
> 
> root:
> stat64("./dummy/.\0", 0x7FFF547477C8, 0x1000)		 = 0 0


I believe this is a bug of macOS (or a "feature" related with the "rootless"
nature of the OS?).

% ls dummy/.
ls: dummy/.: Not a directory
% sudo ls dummy/.
dummy/.

Same for "stat dummy/.".

I've tested three system calls stat(2), lstat(2) and access(2) with
the path "dummy/." and found that all succeed if called by root.

If we want to workaround this "bug", we need to check st_mode of dummy
somewhere in glob.c.

Re: [Bug] Strange Globing Behaviour when used with sudo

[Peter Stephenson]

On Thu, 31 May 2018 20:39:45 +0900
Jun T <takimoto-j@kba.biglobe.ne.jp> wrote:
> If we want to workaround this "bug", we need to check st_mode of dummy
> somewhere in glob.c.

The key point is probably the return from statfullpath(), but as far as
ad-hoc workarounds go you're on your own, I'm afraid.

pws

Re: [Bug] Strange Globing Behaviour when used with sudo

[Daniel Tameling]

On Thu, May 31, 2018 at 04:29:39PM +0100, Peter Stephenson wrote:
> On Thu, 31 May 2018 20:39:45 +0900
> Jun T <takimoto-j@kba.biglobe.ne.jp> wrote:
> > If we want to workaround this "bug", we need to check st_mode of dummy
> > somewhere in glob.c.
> 
> The key point is probably the return from statfullpath(), but as far as
> ad-hoc workarounds go you're on your own, I'm afraid.
> 

I came up with a patch, but it's more a proof of concept. It fixes the
issue at hand, but I am not sure about it's side effects so I didn't
clean it up.
What is happening is the following: the problem arises when
statfullpath is called in glob.c at the following place:

} else if (!checked) {
	if (statfullpath(s, &buf, 1)) {
		unqueue_signals();
		return;
	}

As s is NULL and buf is in the example ./file/, statfullpath adds to
buf a . so that it's "./file/.". At the end of the function stat is
called for this path, which should return a non-zero value, but which
doesn't happen with sudo on Mac OS X. Instead it returns incorrectly
zero, which is then the return value of statfullpath, and thus
                unqueue_signals();
		return;
isn't called, and we get matches. So one possibility to fix this is to
call stat before adding the . to ./file/, and the return non-zero if
it is not a directory. But again, I don't know whether this fix would
introduce some other bugs. I'm also not sure whether there are even
more "bugs" because Mac OS behaves so strangely with sudo. 

--
Best,
Daniel

diff --git a/Src/glob.c b/Src/glob.c
index 66a95329f..19a8766f9 100644
--- a/Src/glob.c
+++ b/Src/glob.c
@@ -294,6 +294,8 @@ 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.
         */
+        stat(buf, st);
+        return !S_ISDIR(st->st_mode);
	 buf[pathpos - pathbufcwd] = '.';
	 buf[pathpos - pathbufcwd + 1] = '\0';
         l = 0;

Re: [Bug] Strange Globing Behaviour when used with sudo

[Jun T.]

> 2018/06/01 03:15, Daniel Tameling <tamelingdaniel@gmail.com> wrote:
> 
> As s is NULL and buf is in the example ./file/, statfullpath adds to
> buf a . so that it's "./file/.".
(snip)
>  So one possibility to fix this is to
> call stat before adding the . to ./file/, and the return non-zero if
> it is not a directory.

Yes, but users can add '.' by themselves, for example
sudo zsh -c 'echo */.'. And of course you can not just
"return !S_ISDIR(st->st_mode)", and st may be NULL.

I've also fond that stat("file/..") is similarly broken.

In the patch below, I enclosed everything within "#ifdef __APPLE__"
to minimize the side effects. It is difficult to detect the bug
by configure since it requires sudo.


diff --git a/Src/glob.c b/Src/glob.c
index 66a95329f..95b3c7ca0 100644
--- a/Src/glob.c
+++ b/Src/glob.c
@@ -288,6 +288,26 @@ statfullpath(const char *s, struct stat *st, int l)
     DPUTS(strlen(s) + !*s + pathpos - pathbufcwd >= PATH_MAX,
 	  "BUG: statfullpath(): pathname too long");
     strcpy(buf, pathbuf + pathbufcwd);
+ 
+#ifdef __APPLE__
+    /*
+     * Workaround for broken stat()/lstat()/access() on macOS.
+     * If called by root, these syscalls mistakenly return success for
+     * path like "foo/bar/." or "foo/bar/.." even if "bar" is a file.
+     * We should make sure "bar" is a directory.
+     */
+    if (!*s || !strcmp(s, ".") || !strcmp(s, "..")) {
+	struct stat stbuf;
+	if (stat(unmeta(buf), &stbuf) || !S_ISDIR(stbuf.st_mode))
+		return -1;
+	if (strcmp(s, "..")) {	    /* done if s is "" or "." */
+	    if (st)
+		*st = stbuf;
+	    return 0;
+	}
+    }
+#endif
+
     strcpy(buf + pathpos - pathbufcwd, s);
     if (!*s && *buf) {
 	/*

Re: [Bug] Strange Globing Behaviour when used with sudo

[Daniel Tameling]

On Fri, Jun 01, 2018 at 11:08:30PM +0900, Jun T. wrote:
> 
> > 2018/06/01 03:15, Daniel Tameling <tamelingdaniel@gmail.com> wrote:
> > 
> > As s is NULL and buf is in the example ./file/, statfullpath adds to
> > buf a . so that it's "./file/.".
> (snip)
> >  So one possibility to fix this is to
> > call stat before adding the . to ./file/, and the return non-zero if
> > it is not a directory.
> 
> Yes, but users can add '.' by themselves, for example
> sudo zsh -c 'echo */.'. And of course you can not just
> "return !S_ISDIR(st->st_mode)", and st may be NULL.
> 
> I've also fond that stat("file/..") is similarly broken.
> 
> In the patch below, I enclosed everything within "#ifdef __APPLE__"
> to minimize the side effects. It is difficult to detect the bug
> by configure since it requires sudo.
> 

Your patch works, but it didn't solve everything. So while
$ sudo zsh -c 'echo */..
results in "no matches found", but already adding a slash and you get
again output:
$ sudo zsh -c 'echo */../
file/../
This is not so easy to fix because you can continue the path after .. :
$ sudo zsh -c 'echo */../.././directory/'
file/../.././directory/
So fixing this would probably mean, you need to iterate over the path,
and whenever encountering "." or ".." checking whether everything
before is really a directory.

But note that fixing the problem would also lead to a behavior that
some might find counter-intuitive:
$ sudo zsh -c 'ls file/..'
file
$ sudo zsh -c 'ls */..'
zsh:1: no matches found: */..

Btw:
I noticed bash manages to trigger the ls error message when zsh
outputs "no matches found" 
$ bash -c 'ls */..'
ls: cannot access '*/..': No such file or directory

--
Best,
Daniel

Re: [Bug] Strange Globing Behaviour when used with sudo

[Jun T.]

Do we need to fix (or workaround) this problem, given that
very basic commands like "ls" are also broken on macOS?

> 2018/06/03 03:17, Daniel Tameling <tamelingdaniel@gmail.com> wrote:
> 
> $ sudo zsh -c 'echo */../
> file/../

Thanks.

If we really need to workaround this bug, probably the simplest way 
would be not to add a file to pathbuf as in the patch below.

I couldn't put all the patch within a single "#ifdef __APPLE__" because
the return value of addpath() is changed to int.

> Btw:
> I noticed bash manages to trigger the ls error message when zsh
> outputs "no matches found" 
> $ bash -c 'ls */..'
> ls: cannot access '*/..': No such file or directory

Try 'setopt no_nomatch'. See the option NOMATCH in zshoptions(1).


diff --git a/Src/glob.c b/Src/glob.c
index 66a95329f..a36d76ac1 100644
--- a/Src/glob.c
+++ b/Src/glob.c
@@ -259,11 +259,28 @@ struct complist {
 /* Add a component to pathbuf: This keeps track of how    *
  * far we are into a file name, since each path component *
  * must be matched separately.                            */
+#ifdef __APPLE__
+#define BROKEN_STAT
+/*
+ * Workaround for broken stat()/lstat()/access() on macOS.
+ * If called by root, these syscalls mistakenly returns success for paths
+ * like "foo/bar/.", "foo/bar/..", "foo/bar/../" etc. even if bar is a file.
+ * This causes statfullpath() to also succeed for these paths.
+ * To workaround this problem, we do not add s to pathbuf if it is not a
+ * directory and return -1 instead.
+ *
+ * On systems other than macOS, always add s to pathbuf and return 0.
+ */
+#endif
 
 /**/
-static void
+static int
 addpath(char *s, int l)
 {
+#ifdef BROKEN_STAT
+    struct stat stbuf;
+    int oppos = pathpos;
+#endif
     DPUTS(!pathbuf, "BUG: pathbuf not initialised");
     while (pathpos + l + 1 >= pathbufsz)
 	pathbuf = zrealloc(pathbuf, pathbufsz *= 2);
@@ -271,6 +288,14 @@ addpath(char *s, int l)
 	pathbuf[pathpos++] = *s++;
     pathbuf[pathpos++] = '/';
     pathbuf[pathpos] = '\0';
+#ifdef BROKEN_STAT
+    if (stat(unmeta(pathbuf), &stbuf) || !S_ISDIR(stbuf.st_mode)) {
+	pathbuf[pathpos = oppos] = '\0';
+	return -1;
+    }
+    else
+#endif
+	return 0;
 }
 
 /* stat the filename s appended to pathbuf.  l should be true for lstat,    *
@@ -531,9 +556,12 @@ scanner(Complist q, int shortcircuit)
 			       sr.st_dev != sc.st_dev);
 		    }
 		}
-		if (add) {
-		    addpath(str, l);
-		    if (!closure || !statfullpath("", NULL, 1)) {
+		if (add && !addpath(str,l)) {
+		    if (!closure
+#ifndef BROKEN_STAT
+			    || !statfullpath("", NULL, 1)
+#endif
+		    ) {
 			scanner((q->closure) ? q : q->next, shortcircuit);
 			if (shortcircuit && shortcircuit == matchct)
 			    return;
@@ -650,15 +678,17 @@ scanner(Complist q, int shortcircuit)
 
 	    for (fn = subdirs; fn < subdirs+subdirlen; ) {
 		int l = strlen(fn);
-		addpath(fn, l);
+		int fn_is_dir = !addpath(fn, l);
 		fn += l + 1;
 		memcpy((char *)&errsfound, fn, sizeof(int));
 		fn += sizeof(int);
-		/* scan next level */
-		scanner((q->closure) ? q : q->next, shortcircuit); 
-		if (shortcircuit && shortcircuit == matchct)
-		    return;
-		pathbuf[pathpos = oppos] = '\0';
+		if (fn_is_dir) {
+		    /* scan next level */
+		    scanner((q->closure) ? q : q->next, shortcircuit); 
+		    if (shortcircuit && shortcircuit == matchct)
+			return;
+		    pathbuf[pathpos = oppos] = '\0';
+		}
 	    }
 	    hrealloc(subdirs, subdirlen, 0);
 	}

Re: [Bug] Strange Globing Behaviour when used with sudo

[Daniel Tameling]

On Mon, Jun 04, 2018 at 01:30:14AM +0900, Jun T. wrote:
> 
> Do we need to fix (or workaround) this problem, given that
> very basic commands like "ls" are also broken on macOS?

Personally, I would say no.
I find that the workarounds make the code very ugly. Then there is the
resulting inconsistency with other commands. Moreover, as the
weirdness occurs only with sudo, it is also very difficult to make
sure that we fix all that could go wrong and don't introduce any
unwanted side effects since globbing with sudo will be rarely used.
So I wouldn't do anything, but would also be fine with something small
that addressed the original problem, like your previous patch.

> > Btw:
> > I noticed bash manages to trigger the ls error message when zsh
> > outputs "no matches found" 
> > $ bash -c 'ls */..'
> > ls: cannot access '*/..': No such file or directory
> 
> Try 'setopt no_nomatch'. See the option NOMATCH in zshoptions(1).

Thanks.

-- 
Best,
Daniel

Re: [Bug] Strange Globing Behaviour when used with sudo

[dana]

On 3 Jun 2018, at 13:48, Daniel Tameling <tamelingdaniel@gmail.com> wrote:
>So I wouldn't do anything, but would also be fine with something small
>that addressed the original problem, like your previous patch.

I didn't see anyone mention submitting a report to Apple, so i've done that, for
whatever it's worth. Radar # 40757266. Will get back to the list if they respond
(unless someone else here beat me to it)

dana

Re: [Bug] Strange Globing Behaviour when used with sudo

[Jun T]

> 2018/06/04 4:17, dana <dana@dana.is> wrote:
> 
> I didn't see anyone mention submitting a report to Apple, so i've done that, for
> whatever it's worth. Radar # 40757266.

Sorry I've sent a bug report but it would be OK (or even better) to send the report
from multiple users.

Re: [Bug] Strange Globing Behaviour when used with sudo

[Jun T]

> 2018/06/04 1:30、I wrote:
> 
> If we really need to workaround this bug, probably the simplest way 
> would be not to add a file to pathbuf as in the patch below.

No, this patch will not work if pathbuf is longer than PATH_MAX.
We need to handle PATH_MAX/pathbufcwd in addpath(), or call statfullpath()
(with my 1st patch) from addpath().

Re: [Bug] Strange Globing Behaviour when used with sudo

[Bengt Brodersen]

[-- Attachment #1: Type: text/plain, Size: 5923 bytes --]

Any news on this?

On Wed, 30 May 2018 at 22:49 Bengt Brodersen <bengt.brodersen@gmail.com>
wrote:

> Just found a paragraph in stat(2) manpage on macos
>
> _DARWIN_FEATURE_64_BIT_INODE
>      In order to accommodate advanced capabilities of newer file systems,
> the struct stat, struct statfs, and struct dirent data structures were
> updated in Mac OSX 10.5.
>
>      The most obvious change is the increased size of ino_t from 32 bits
> to 64 bits.  As a consequence, storing an ino_t in an int is no longer
> safe, and file formats storing ino_t as 32-bit values may need to be
>      updated.  There are other changes as well, such as the widening of
> f_fstypename, f_mntonname, and f_mntfromname in struct statfs.  Please
> refer to stat(2) and dir(5) for more detail on the specific changes to
>      the other affected data structures.
>
>      On platforms that existed before these updates were available, ABI
> compatibility is achieved by providing two implementations for related
> functions: one using the legacy data structures and one using the
>      updated data structures.  Variants which make use of the newer
> structures have their symbols suffixed with $INODE64.  These $INODE64
> suffixes are automatically appended by the compiler tool-chain and should
>      not be used directly.
>
>      Platforms that were released after these updates only have the newer
> variants available to them.  These platforms have the macro
> _DARWIN_FEATURE_ONLY_64_BIT_INODE defined.
>
>      The _DARWIN_FEATURE_64_BIT_INODE macro should not be set directly.
> Instead, developers should make use of the _DARWIN_NO_64_BIT_INODE or
> _DARWIN_USE_64_BIT_INODE macros when the default variant is not
>      desired.  The following table details the effects of defining these
> macros for different deployment targets.
>
>
>             _DARWIN_FEATURE_ONLY_64_BIT_INODE not defined
>
>        -------------------------+-------------------------------
>
>                                 |       Deployment Target
>
>             user defines:       |   < 10.5       10.5    > 10.5
>
>        -------------------------+-------------------------------
>
>                 (none)          |   32-bit      32-bit   64-bit
>
>        _DARWIN_NO_64_BIT_INODE  |   32-bit      32-bit   32-bit
>
>        _DARWIN_USE_64_BIT_INODE |   32-bit      64-bit   64-bit
>
>        -------------------------+-------------------------------
>
>
>               _DARWIN_FEATURE_ONLY_64_BIT_INODE defined
>
>        -------------------------+-------------------------------
>
>             user defines:       | Any Deployment Target
>
>        -------------------------+-------------------------------
>
>                 (none)          | 64-bit-only
>
>        _DARWIN_NO_64_BIT_INODE  |   (error)
>
>        _DARWIN_USE_64_BIT_INODE | 64-bit-only
>
>        -------------------------+-------------------------------
>
>            32-bit       32-bit inode values are enabled, and the legacy
> structures involving the ino_t type are in use.  The macro
> _DARWIN_FEATURE_64_BIT_INODE is not defined.
>
>            64-bit       64-bit inode values are enabled, and the expanded
> structures involving the ino_t type are in use.  The macro
> _DARWIN_FEATURE_64_BIT_INODE is defined, and loader symbols will contain the
>                         $INODE64 suffix.
>
>            64-bit-only  Like 64-bit, except loader symbols do not have the
> $INODE64 suffix.
>
>            (error)      A compile time error is generated.
>
>      Due to the increased benefits of the larger structure, it is highly
> recommended that developers not define _DARWIN_NO_64_BIT_INODE and make use
> of _DARWIN_USE_64_BIT_INODE when targeting Mac OSX 10.5.
>
>      In addition to the $INODE64 suffixed symbols, variants suffixed with
> 64 are also available for related functions.  These functions were provided
> as a way for developers to use the updated structures in code
>      that also made use of the legacy structures.  The enlarged stat
> structures were also prefixed with 64 to distinguish them from their legacy
> variants.  These functions have been deprecated and should be
>      avoided.
>
> On Wed, 30 May 2018 at 22:44 Bengt Brodersen <bengt.brodersen@gmail.com>
> wrote:
>
>> If you google *mac stat64 deprecated* you will get a lot of results.
>> So maybe it caused by the depreciation on darwin os
>>
>> On Wed, 30 May 2018 at 22:24 Phil Pennock <
>> zsh-workers+phil.pennock@spodhuis.org> wrote:
>>
>>> On 2018-05-30 at 19:15 +0000, Daniel Shahaf wrote:
>>> > > On Wed, May 30, 2018 at 10:11 AM, Bengt Brodersen
>>> > > > sudo zsh -c 'echo ./*/'
>>> > > >>> ./file/ ./folder/
>>>
>>> > - macOS 10.13.4 (17E202)
>>> > - zsh 5.5.1 (x86_64-apple-darwin17.5.0)
>>> >
>>> > and that the issue still occurred under -fc.
>>>
>>> This appears to be macOS returning different results for stat64() for
>>> root vs non-root.  Using dtruss:
>>>
>>> non-root:
>>> open_nocancel("./\0", 0x1100004, 0x10040A3F8)            = 3 0
>>> fstatfs64(0x3, 0x7FFF5F8A1338, 0x10040A3F8)              = 0 0
>>> getdirentries64(0x3, 0x7FA30F805E00, 0x1000)             = 112 0
>>> getdirentries64(0x3, 0x7FA30F805E00, 0x1000)             = 0 0
>>> close_nocancel(0x3)              = 0 0
>>> stat64("./dummy/.\0", 0x7FFF5F8A1948, 0x1000)            = -1 Err#20
>>> stat64("./folder/.\0", 0x7FFF5F8A1948, 0x1000)           = 0 0
>>>
>>> root:
>>> open_nocancel("./\0", 0x1100004, 0x10B5F8B28)            = 3 0
>>> fstatfs64(0x3, 0x7FFF547471B8, 0x10B5F8B28)              = 0 0
>>> getdirentries64(0x3, 0x7FAFD9821400, 0x1000)             = 112 0
>>> getdirentries64(0x3, 0x7FAFD9821400, 0x1000)             = 0 0
>>> close_nocancel(0x3)              = 0 0
>>> stat64("./dummy/.\0", 0x7FFF547477C8, 0x1000)            = 0 0
>>> stat64("./folder/.\0", 0x7FFF547477C8, 0x1000)           = 0 0
>>>
>>> <sys/errno.h> defines ENOTDIR as 20.
>>>
>>> -Phil
>>>
>>

Re: [Bug] Strange Globing Behaviour when used with sudo

[Martijn Dekker]

Op 30-05-18 om 22:23 schreef Phil Pennock:
> On 2018-05-30 at 19:15 +0000, Daniel Shahaf wrote:
>>> On Wed, May 30, 2018 at 10:11 AM, Bengt Brodersen
>>>> sudo zsh -c 'echo ./*/'
>>>>>> ./file/ ./folder/
> 
>> - macOS 10.13.4 (17E202)
>> - zsh 5.5.1 (x86_64-apple-darwin17.5.0)
>>
>> and that the issue still occurred under -fc.
> 
> This appears to be macOS returning different results for stat64() for
> root vs non-root.

I can confirm this behaviour on Mac OS X 10.11.6 with zsh versions 5.0.7 
through 5.5.1 (I didn't test earlier zsh versions).

According to my testing, bash, dash, yash, mksh/lksh do *not* act this 
way when running as root on the Mac -- only zsh does.

FYI, FWIW, HTH.

- M.