Re: Bug#355810: coreutils: pwd fails on bind mount

[Jim Meyering]

Kenshi Muto <address@hidden> wrote:
> At Wed, 08 Mar 2006 08:59:55 +0100,
> Jim Meyering wrote:
>> Kenshi Muto <address@hidden> wrote:
>> So far, I am unable to reproduce the failure on ia32.
...
>> also, please run the failing command under strace, e.g.
>>
>>   cd /home/kmuto/d-i
>>   strace -o /tmp/log /bin/pwd
>
> $ strace -o /tmp/log /bin/pwd outputs following to stderr:
> umovestr: Input/output error
...
> /bin/pwd: couldn't find directory entry in `../../..' with matching i-node

Thanks.

Your strace log showed that /bin/pwd was not calling getcwd.
Yet when I build coreutils-5.94 myself on leisner.debian.org
and try the same experiment, that binary does call getcwd, and
works just fine,

  leisner$ dchroot unstable /home/meyering/coreutils-5.94/src/pwd
  Executing pwd in chroot: /org/leisner.debian.org/chroot/sid
  /home/meyering
  leisner$ dchroot unstable /bin/pwd
  Executing pwd in chroot: /org/leisner.debian.org/chroot/sid
  pwd: couldn't find directory entry in `../..' with matching i-node
  [Exit 1]

I suspect that the arm binary was built against an older version of
libc -- one that made coreutils use its getcwd.c replacement.
I confirmed the theory by forcing coreutils to use the replacement getcwd:

  gl_cv_func_getcwd_null=no gl_cv_func_getcwd_path_max=no ./configure

Then the resulting binary fails (in leisner's chroot), just as
in the example above.

Debugging that failure, I found the source of the problem: a subtle flaw
in lib/getcwd.c (which is based on glibc's sysdeps/posix/getcwd.c).
See the comments in the patch for details.
(fyi, it looks big, but most of it is due to an indentation change)

Technically, this is grounds for changing m4/getcwd.m4 to detect
the deficiency in glibc's getcwd, but performing the actual test
would be tricky, to say the least, and the replacement would be
of only marginal value, since it'd come into play only in a chroot
when the working directory name is too long for the getcwd syscall
to work.  So I won't bother right now.

I've applied the following only in coreutils, for starters:

2006-03-19  Jim Meyering  <address@hidden>

        Work even in a chroot where d_ino values for entries in "/"
        don't match the stat.st_ino values for the same names.
        * getcwd.c (__getcwd): When no d_ino value matches the target inode
        number, iterate through all entries again, using lstat instead.
        Reported by Kenshi Muto in http://bugs.debian.org/355810.

Index: lib/getcwd.c
===================================================================
RCS file: /fetish/cu/lib/getcwd.c,v
retrieving revision 1.19
retrieving revision 1.20
diff -u -p -u -r1.19 -r1.20
--- lib/getcwd.c        19 Mar 2006 17:18:32 -0000      1.19
+++ lib/getcwd.c        19 Mar 2006 18:27:51 -0000      1.20
@@ -211,6 +211,7 @@ __getcwd (char *buf, size_t size)
       int parent_status;
       size_t dirroom;
       size_t namlen;
+      bool use_d_ino = true;
 
       /* Look at the parent directory.  */
 #ifdef AT_FDCWD
@@ -257,6 +258,21 @@ __getcwd (char *buf, size_t size)
             NULL.  */
          __set_errno (0);
          d = __readdir (dirstream);
+
+         /* When we've iterated through all directory entries without finding
+            one with a matching d_ino, rewind the stream and consider each
+            name again, but this time, using lstat.  This is necessary in a
+            chroot on at least one system (glibc-2.3.6 + linux 2.6.12), where
+            .., ../.., ../../.., etc. all had the same device number, yet the
+            d_ino values for entries in / did not match those obtained
+            via lstat.  */
+         if (d == NULL && errno == 0 && use_d_ino)
+           {
+             use_d_ino = false;
+             rewinddir (dirstream);
+             d = __readdir (dirstream);
+           }
+
          if (d == NULL)
            {
              if (errno == 0)
@@ -269,58 +285,65 @@ __getcwd (char *buf, size_t size)
              (d->d_name[1] == '\0' ||
               (d->d_name[1] == '.' && d->d_name[2] == '\0')))
            continue;
-         if (MATCHING_INO (d, thisino) || mount_point)
+
+         if (use_d_ino)
            {
-             int entry_status;
+             bool match = (MATCHING_INO (d, thisino) || mount_point);
+             if (! match)
+               continue;
+           }
+
+         {
+           int entry_status;
 #ifdef AT_FDCWD
-             entry_status = fstatat (fd, d->d_name, &st, AT_SYMLINK_NOFOLLOW);
+           entry_status = fstatat (fd, d->d_name, &st, AT_SYMLINK_NOFOLLOW);
 #else
-             /* Compute size needed for this file name, or for the file
-                name ".." in the same directory, whichever is larger.
-                Room for ".." might be needed the next time through
-                the outer loop.  */
-             size_t name_alloc = _D_ALLOC_NAMLEN (d);
-             size_t filesize = dotlen + MAX (sizeof "..", name_alloc);
-
-             if (filesize < dotlen)
-               goto memory_exhausted;
-
-             if (dotsize < filesize)
-               {
-                 /* My, what a deep directory tree you have, Grandma.  */
-                 size_t newsize = MAX (filesize, dotsize * 2);
-                 size_t i;
-                 if (newsize < dotsize)
-                   goto memory_exhausted;
-                 if (dotlist != dots)
-                   free (dotlist);
-                 dotlist = malloc (newsize);
-                 if (dotlist == NULL)
-                   goto lose;
-                 dotsize = newsize;
-
-                 i = 0;
-                 do
-                   {
-                     dotlist[i++] = '.';
-                     dotlist[i++] = '.';
-                     dotlist[i++] = '/';
-                   }
-                 while (i < dotlen);
-               }
-
-             memcpy (dotlist + dotlen, d->d_name, _D_ALLOC_NAMLEN (d));
-             entry_status = __lstat (dotlist, &st);
-#endif
-             /* We don't fail here if we cannot stat() a directory entry.
-                This can happen when (network) file systems fail.  If this
-                entry is in fact the one we are looking for we will find
-                out soon as we reach the end of the directory without
-                having found anything.  */
-             if (entry_status == 0 && S_ISDIR (st.st_mode)
-                 && st.st_dev == thisdev && st.st_ino == thisino)
-               break;
-           }
+           /* Compute size needed for this file name, or for the file
+              name ".." in the same directory, whichever is larger.
+              Room for ".." might be needed the next time through
+              the outer loop.  */
+           size_t name_alloc = _D_ALLOC_NAMLEN (d);
+           size_t filesize = dotlen + MAX (sizeof "..", name_alloc);
+
+           if (filesize < dotlen)
+             goto memory_exhausted;
+
+           if (dotsize < filesize)
+             {
+               /* My, what a deep directory tree you have, Grandma.  */
+               size_t newsize = MAX (filesize, dotsize * 2);
+               size_t i;
+               if (newsize < dotsize)
+                 goto memory_exhausted;
+               if (dotlist != dots)
+                 free (dotlist);
+               dotlist = malloc (newsize);
+               if (dotlist == NULL)
+                 goto lose;
+               dotsize = newsize;
+
+               i = 0;
+               do
+                 {
+                   dotlist[i++] = '.';
+                   dotlist[i++] = '.';
+                   dotlist[i++] = '/';
+                 }
+               while (i < dotlen);
+             }
+
+           memcpy (dotlist + dotlen, d->d_name, _D_ALLOC_NAMLEN (d));
+           entry_status = __lstat (dotlist, &st);
+#endif
+           /* We don't fail here if we cannot stat() a directory entry.
+              This can happen when (network) file systems fail.  If this
+              entry is in fact the one we are looking for we will find
+              out soon as we reach the end of the directory without
+              having found anything.  */
+           if (entry_status == 0 && S_ISDIR (st.st_mode)
+               && st.st_dev == thisdev && st.st_ino == thisino)
+             break;
+         }
        }
 
       dirroom = dirp - dir;