public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
From: Nick Alcock <nix@esperi.org.uk>
To: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Cc: libc-alpha@sourceware.org
Subject: Re: [PATCH v2] Fix 32-bit getcwd() on filesystems with 64-bit inodes.
Date: Tue, 27 Feb 2018 00:12:00 -0000	[thread overview]
Message-ID: <87606j5nkn.fsf_-_@esperi.org.uk> (raw)
In-Reply-To: <2a84a9a8-c838-af48-e549-bb7394e92287@linaro.org> (Adhemerval Zanella's message of "Mon, 26 Feb 2018 10:15:48 -0300")

Here's the latest patch round, with your review comments incorporated,
and retested on trunk. Thanks for the review!

On 26 Feb 2018, Adhemerval Zanella said:

> Thanks for checking on it and I see no reason to continue using non-LFS
> calls on loader for 32-bits architectures.

I'm sure there are more. I might do a big audit for all remaining
external references to non-LFS stuff from stuff that is not itself a
non-LFS implementation, and squash them all at once. (But maybe we
should do this differently, by internally defining __readdir() etc to be
__readdir64() as is true when _FILE_OFFSET_BITS=64, and adding an
internal __readdir32() for the non-LFS version? That way you'd have to
do something unusual to call the non-LFS variant, which is surely the
right way around.)

I audited all other readdir64() implementations: no others define any
non-default symbol versions except by #including the i386 one, so I
think we're fine on other arches. The Hurd version calls __dir_readdir()
which is not in glibc so I just have to hope that it doesn't have the
same limitation as Linux getdents(). :)

(I hope scissors lines work here: git am --scissors.)

8<---------------------->8
From: Nick Alcock <nick.alcock@oracle.com>

When compiling a 32-bit glibc on a filesystem with 64-bit inodes,
we observe failures of io/tst-getcwd-abspath:

tst-getcwd-abspath.c:42: numeric comparison failure
   left: 75 (0x4b); from: errno
  right: 2 (0x2); from: ENOENT
tst-getcwd-abspath.c:47: numeric comparison failure
   left: 75 (0x4b); from: errno
  right: 2 (0x2); from: ENOENT
error: 2 test failures

Errno 75 is EOVERFLOW, which is most unexpected from getcwd()! Having
had experience with this class of pain in zic before (a patch which I
should perhaps resubmit or combine with this one), the cause is clear:
the getcwd() implementation was calling readdir() because it was in a
disconnected subtree, and in glibc that is the non-LFS implementation
so it ends up calling getdents(), which falls over with just that
error if it sees a single inode with a value nonrepresentable in 32
bits in the directories it fetches, and it scans all parents of the
current directory so it has a lot of opportunities to hit such an
inode.

getcwd() is used in the dynamic linker as part of $ORIGIN support, so
the usual SHLIB_COMPAT dance is needed there to prevent versioned symbols
getting into it and causing disaster.

	[BZ #22899]
	* include/dirent.h: Make __readdir64 hidden in rtld too.
	* sysdeps/unix/sysv/linux/i386/readdir64.c: Mark SHLIB_COMPAT
	in libc only.
	* posix/getcwd.c [__GNU_LIBRARY__] (__readdir64): Define.
	(__getcwd): Use dirent64 and __readdir64.
---
 include/dirent.h                         |  4 +++-
 sysdeps/posix/getcwd.c                   |  5 +++--
 sysdeps/unix/sysv/linux/i386/readdir64.c | 14 ++++++++------
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/include/dirent.h b/include/dirent.h
index caaeb0be85..9ca588a4e3 100644
--- a/include/dirent.h
+++ b/include/dirent.h
@@ -21,7 +21,9 @@ extern DIR *__fdopendir (int __fd) attribute_hidden;
 extern int __closedir (DIR *__dirp) attribute_hidden;
 extern struct dirent *__readdir (DIR *__dirp) attribute_hidden;
 extern struct dirent64 *__readdir64 (DIR *__dirp);
-libc_hidden_proto (__readdir64)
+#  if IS_IN (libc) || (IS_IN (rtld) && !defined NO_RTLD_HIDDEN)
+   hidden_proto (__readdir64)
+#  endif
 extern int __readdir_r (DIR *__dirp, struct dirent *__entry,
 			struct dirent **__result);
 extern int __readdir64_r (DIR *__dirp, struct dirent64 *__entry,
diff --git a/sysdeps/posix/getcwd.c b/sysdeps/posix/getcwd.c
index b53433a2dc..f547cd6612 100644
--- a/sysdeps/posix/getcwd.c
+++ b/sysdeps/posix/getcwd.c
@@ -194,6 +194,7 @@ extern char *alloca ();
 
 #ifndef __GNU_LIBRARY__
 # define __lstat64	stat64
+# define __readdir64	readdir64
 #endif
 
 #ifndef _LIBC
@@ -366,14 +367,14 @@ __getcwd (char *buf, size_t size)
 	goto lose;
       fd_needs_closing = false;
 
-      struct dirent *d;
+      struct dirent64 *d;
       bool use_d_ino = true;
       while (1)
 	{
 	  /* Clear errno to distinguish EOF from error if readdir returns
 	     NULL.  */
 	  __set_errno (0);
-	  d = __readdir (dirstream);
+	  d = __readdir64 (dirstream);
 	  if (d == NULL)
 	    {
 	      if (errno == 0)
diff --git a/sysdeps/unix/sysv/linux/i386/readdir64.c b/sysdeps/unix/sysv/linux/i386/readdir64.c
index 42b73023e0..e2981c7f9c 100644
--- a/sysdeps/unix/sysv/linux/i386/readdir64.c
+++ b/sysdeps/unix/sysv/linux/i386/readdir64.c
@@ -28,19 +28,21 @@
 #undef DIRENT_TYPE
 
 libc_hidden_def (__readdir64)
+#if IS_IN (libc)
 versioned_symbol (libc, __readdir64, readdir64, GLIBC_2_2);
 
-#if SHLIB_COMPAT(libc, GLIBC_2_1, GLIBC_2_2)
+#  if SHLIB_COMPAT(libc, GLIBC_2_1, GLIBC_2_2)
 
-#include <olddirent.h>
+#  include <olddirent.h>
 
-#define __READDIR attribute_compat_text_section __old_readdir64
-#define __GETDENTS __old_getdents64
-#define DIRENT_TYPE struct __old_dirent64
+#  define __READDIR attribute_compat_text_section __old_readdir64
+#  define __GETDENTS __old_getdents64
+#  define DIRENT_TYPE struct __old_dirent64
 
-#include <sysdeps/posix/readdir.c>
+#  include <sysdeps/posix/readdir.c>
 
 libc_hidden_def (__old_readdir64)
 
 compat_symbol (libc, __old_readdir64, readdir64, GLIBC_2_1);
 #endif
+#endif
-- 
2.16.2.226.gdbca7b3d5

  parent reply	other threads:[~2018-02-26 23:40 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-26 10:48 [PATCH] " Nick Alcock
2018-02-26 13:44 ` Adhemerval Zanella
2018-02-26 21:03   ` Nix
2018-02-27 16:32     ` Adhemerval Zanella
2018-02-27  0:12   ` Nick Alcock [this message]
2018-02-27 16:33     ` [PATCH v2] " Adhemerval Zanella
2018-02-27 18:08       ` Nick Alcock
2018-02-27 20:34         ` Zack Weinberg
2018-02-26 18:08 ` [PATCH] " Joseph Myers
2018-02-26 20:47   ` Nix
2018-02-26 23:57     ` Nick Alcock

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87606j5nkn.fsf_-_@esperi.org.uk \
    --to=nix@esperi.org.uk \
    --cc=adhemerval.zanella@linaro.org \
    --cc=libc-alpha@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).