Subject: bug#21534: closed (Re: bug#21534: Bug in mkdir?!)

From: [email-masked] (GNU bug Tracking System)
Date: Wed, 23 Sep 2015 03:10:04 +0000

Your bug report

#21534: Bug in mkdir?!

which was filed against the coreutils package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
21534: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=21534
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#21534: Bug in mkdir?!
From: Paul Eggert <[email-masked]>
To: Sebastian Unger <[email-masked]>, [email-masked]
Date: Tue, 22 Sep 2015 20:09:35 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

Sebastian Unger wrote:
> Why is it trying to open the directory
> in the first place?

Security.

Apparently POSIX doesn't allow this level of paranoia for mkdir -p, so I removed 
it in the attached Gnulib patch, and this should appear in the next coreutils 
release.

A filesystem that doesn't let you read your own directory that you just created 
is likely to run into other problems like this -- i.e., the practice may 
introduce more security problems than it closes.  But I digress.
From 1f869b77a7e1bb871d58d30edd2ea04a9ac04d32 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Tue, 22 Sep 2015 20:04:13 -0700
Subject: [PATCH] savewd: remove SAVEWD_CHDIR_READABLE

It was problematic in the light of file systems that ignore umask.
Problem reported by Sebastian Ungar in: http://bugs.gnu.org/21534
* NEWS: Document this.
* lib/mkancesdirs.c (mkancesdirs): MAKE_DIR now returns 0 if
successful, -1 (setting errno) on failure, rather than something
more complicated than that.
* lib/mkdir-p.c (make_dir_parents):
Do not use SAVEWD_CHDIR_READABLE.
* lib/savewd.c (savewd_chdir):
Remove support for SAVEWD_CHDIR_READABLE.
* lib/savewd.h (SAVEWD_CHDIR_READABLE): Remove.
---
 ChangeLog         | 13 +++++++++++++
 NEWS              |  2 ++
 lib/mkancesdirs.c | 24 ++++++------------------
 lib/mkdir-p.c     |  4 +---
 lib/savewd.c      |  2 +-
 lib/savewd.h      |  9 ++-------
 6 files changed, 25 insertions(+), 29 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 62458b2..1bcc6f1 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,18 @@
 2015-09-22  Paul Eggert  <[email-masked]>
 
+	savewd: remove SAVEWD_CHDIR_READABLE
+	It was problematic in the light of file systems that ignore umask.
+	Problem reported by Sebastian Ungar in: http://bugs.gnu.org/21534
+	* NEWS: Document this.
+	* lib/mkancesdirs.c (mkancesdirs): MAKE_DIR now returns 0 if
+	successful, -1 (setting errno) on failure, rather than something
+	more complicated than that.
+	* lib/mkdir-p.c (make_dir_parents):
+	Do not use SAVEWD_CHDIR_READABLE.
+	* lib/savewd.c (savewd_chdir):
+	Remove support for SAVEWD_CHDIR_READABLE.
+	* lib/savewd.h (SAVEWD_CHDIR_READABLE): Remove.
+
 	c-ctype: port better to EBCDIC
 	Problems reported by Daniel Richard G. in
 	http://lists.gnu.org/archive/html/bug-gnulib/2015-09/msg00020.html
diff --git a/NEWS b/NEWS
index b294edd..b54a992 100644
--- a/NEWS
+++ b/NEWS
@@ -42,6 +42,8 @@ User visible incompatible changes
 
 Date        Modules         Changes
 
+2015-09-22  savewd          SAVEWD_CHDIR_READABLE constant removed.
+
 2015-07-24  fprintftime     Exported functions' time zone arguments are now of
             strftime        type timezone_t (with NULL denoting UTC) instead of
                             type int (with nonzero denoting UTC).  These
diff --git a/lib/mkancesdirs.c b/lib/mkancesdirs.c
index 91ed694..2747d4a 100644
--- a/lib/mkancesdirs.c
+++ b/lib/mkancesdirs.c
@@ -42,11 +42,9 @@
 
    Create any ancestor directories that don't already exist, by
    invoking MAKE_DIR (FILE, COMPONENT, MAKE_DIR_ARG).  This function
-   should return 0 if successful and the resulting directory is
-   readable, 1 if successful but the resulting directory might not be
-   readable, -1 (setting errno) otherwise.  If COMPONENT is relative,
-   it is relative to the temporary working directory, which may differ
-   from *WD.
+   should return 0 if successful, -1 (setting errno) otherwise.  If
+   COMPONENT is relative, it is relative to the temporary working
+   directory, which may differ from *WD.
 
    Ordinarily MAKE_DIR is executed with the working directory changed
    to reflect the already-made prefix, and mkancesdirs returns with
@@ -112,20 +110,10 @@ mkancesdirs (char *file, struct savewd *wd,
             if (sep - component == 2
                 && component[0] == '.' && component[1] == '.')
               made_dir = false;
+            else if (make_dir (file, component, make_dir_arg) < 0)
+              make_dir_errno = errno;
             else
-              switch (make_dir (file, component, make_dir_arg))
-                {
-                case -1:
-                  make_dir_errno = errno;
-                  break;
-
-                case 0:
-                  savewd_chdir_options |= SAVEWD_CHDIR_READABLE;
-                  /* Fall through.  */
-                case 1:
-                  made_dir = true;
-                  break;
-                }
+              made_dir = true;
 
             if (made_dir)
               savewd_chdir_options |= SAVEWD_CHDIR_NOFOLLOW;
diff --git a/lib/mkdir-p.c b/lib/mkdir-p.c
index e55b1dd..e518272 100644
--- a/lib/mkdir-p.c
+++ b/lib/mkdir-p.c
@@ -136,9 +136,7 @@ make_dir_parents (char *dir,
               announce (dir, options);
               preserve_existing = (keep_owner & keep_special_mode_bits
                                    & umask_must_be_ok);
-              savewd_chdir_options |=
-                (SAVEWD_CHDIR_NOFOLLOW
-                 | (mode & S_IRUSR ? SAVEWD_CHDIR_READABLE : 0));
+              savewd_chdir_options |= SAVEWD_CHDIR_NOFOLLOW;
             }
           else
             {
diff --git a/lib/savewd.c b/lib/savewd.c
index 16f56a8..f835074 100644
--- a/lib/savewd.c
+++ b/lib/savewd.c
@@ -116,7 +116,7 @@ savewd_chdir (struct savewd *wd, char const *dir, int options,
           open_result[1] = errno;
         }
 
-      if (fd < 0 && (errno != EACCES || (options & SAVEWD_CHDIR_READABLE)))
+      if (fd < 0 && errno != EACCES)
         result = -1;
     }
 
diff --git a/lib/savewd.h b/lib/savewd.h
index bb442b3..69b3718 100644
--- a/lib/savewd.h
+++ b/lib/savewd.h
@@ -82,20 +82,15 @@ savewd_init (struct savewd *wd)
 }

-/* Options for savewd_chdir.  */
+/* Options for savewd_chdir.  Can be ORed together.  */
 enum
   {
     /* Do not follow symbolic links, if supported.  */
     SAVEWD_CHDIR_NOFOLLOW = 1,
 
-    /* The directory should be readable, so fail if it happens to be
-       discovered that the directory is not readable.  (Unreadable
-       directories are not necessarily diagnosed, though.)  */
-    SAVEWD_CHDIR_READABLE = 2,
-
     /* Do not chdir if the directory is readable; simply succeed
        without invoking chdir if the directory was opened.  */
-    SAVEWD_CHDIR_SKIP_READABLE = 4
+    SAVEWD_CHDIR_SKIP_READABLE = 2
   };
 
 /* Change the directory, and if successful, record into *WD the fact
-- 
2.1.0

Subject: Bug in mkdir?!
From: Sebastian Unger <[email-masked]>
To: [email-masked]
Date: Tue, 22 Sep 2015 22:29:44 +1200
List-Id: <debbugs-submit.debbugs.gnu.org>

Hi,

I'm working on a little project of mine involving a Fuse file system on
Linux. I'm having a problem where mkdir from coreutils fails with
Permission denied while mkdir from busybox works.

I'm running the command *mkdir -p a/b/c* and I get the following strace
output (showing only the relevant lines at the end):

*umask(0)                                = 022mkdir("a",
0755)                        = 0open("a",
O_RDONLY|O_NOCTTY|O_NONBLOCK|O_DIRECTORY|O_NOFOLLOW) = -1 EACCES
(Permission denied)open("/usr/share/locale/locale.alias",
O_RDONLY|O_CLOEXEC) = 3fstat(3, {st_mode=S_IFREG|0644, st_size=2570, ...})
= 0mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0)
= 0x7f6a86f20000read(3, "# Locale name alias data base.\n#"..., 4096) =
2570read(3, "", 4096)                       =
0close(3)                                = 0munmap(0x7f6a86f20000,
4096)            =
0open("/usr/share/locale/en_NZ/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1
ENOENT (No such file or
directory)open("/usr/share/locale/en/LC_MESSAGES/coreutils.mo", O_RDONLY) =
-1 ENOENT (No such file or
directory)open("/usr/share/locale-langpack/en_NZ/LC_MESSAGES/coreutils.mo",
O_RDONLY) = -1 ENOENT (No such file or
directory)open("/usr/share/locale-langpack/en/LC_MESSAGES/coreutils.mo",
O_RDONLY) = 3fstat(3, {st_mode=S_IFREG|0644, st_size=619, ...}) =
0mmap(NULL, 619, PROT_READ, MAP_PRIVATE, 3, 0) =
0x7f6a86f20000close(3)                                =
0open("/usr/lib/charset.alias", O_RDONLY|O_NOFOLLOW) = -1 ENOENT (No such
file or directory)write(2, "mkdir: ", 7mkdir: )                  =
7write(2, "cannot create directory \342\200\230a\342\200\231", 31cannot
create directory ‘a’) =
31open("/usr/share/locale/en_NZ/LC_MESSAGES/libc.mo", O_RDONLY) = -1 ENOENT
(No such file or directory)open("/usr/share/locale/en/LC_MESSAGES/libc.mo",
O_RDONLY) = -1 ENOENT (No such file or
directory)open("/usr/share/locale-langpack/en_NZ/LC_MESSAGES/libc.mo",
O_RDONLY) = -1 ENOENT (No such file or
directory)open("/usr/share/locale-langpack/en/LC_MESSAGES/libc.mo",
O_RDONLY) = -1 ENOENT (No such file or directory)write(2, ": Permission
denied", 19: Permission denied)     = 19write(2, "\n",
1)                       = 1close(1)                                =
0close(2)                                =
0exit_group(1)                           = ?+++ exited with 1 +++*
Please note that while the creation of directory 'a' succeeds, the
directory is created by the fuse with mode 0311 (-wx--x--x) and as such the
open call following the mkdir fails as expected.
If I try to simulate this on a normal file system by setting the umask to
0466, I get the following strace output:

*umask(0)                                = 0466mkdir("a",
0311)                        = 0open("a",
O_RDONLY|O_NOCTTY|O_NONBLOCK|O_DIRECTORY|O_NOFOLLOW) = -1 EACCES
(Permission denied)chdir("a")                              = 0mkdir("b",
0311)                        = 0open("b",
O_RDONLY|O_NOCTTY|O_NONBLOCK|O_DIRECTORY|O_NOFOLLOW) = -1 EACCES
(Permission denied)chdir("b")                              = 0mkdir("c",
0311)                        = 0close(1)                                =
0close(2)                                =
0exit_group(0)                           = ?+++ exited with 0 +++*
I.e. the open still fails but apparently mkdir can live with it.  Since it
also works on my fuse if I set the umask to 0466, I conclude that mkdir
inspects the umask to see if it expects to be able to open the directory?!
That sounds like a race to me since anything may change the mode of the
directory between the mkdir and open calls even on a normal FS and even
more so on a networked file system. Why is it trying to open the directory
in the first place? Shouldn't it just chdir into it and carry on?

Is this a bug or expected behaviour? Any ideas as to how to make mkdir
behave?

Cheers,
Seb