This is the mail archive of the libc-alpha@sources.redhat.com mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]

[PATCH] for glibc-2.2 linux-specific getdents code



Hi,

  After some discussion over a problem reported by Mogens Kjær whereby
it emerged that the readdir() function was failing to read to the end
of the directory on some NFS mounts, it was found that the bug lies
partly in Linux kernels 2.4.x, and partly in the assumptions of the
glibc-2.2.

In particular, 3 problems were found in the special Linux version of
the getdents() function.

  - The first problem is generic: Glibc uses __lseek rather than
    __lseek64 for the output offsets from getdents. Although Linux
    currently only does 32-bit offsets, it is expected that 64-bit
    offsets will be supported in the future (I've already got a patch
    for this), and indeed the d_off type in struct dirent is signed
    64-bit.

    Despite the appended fix, though, I would urge you to consider
    removing lseek altogether from getdents(). It can break on
    non-POSIX filesystems, and is really something that the user could
    easily do by him/herself.
    I'm not aware of any other *NIX implementations of readdir that
    depend on lseek() in this way.



  - The second problem is more NFS-specific: Glibc is unable to cope
    with NFS allowing unsigned offsets, and returns an EOVERFLOW error
    as if the cookie were a large file offset if the 31st bit is set
    (and the getdents64() function is used).

    The reason for this is the comparison
        dp->d_off != d_off
    Since sizeof(dp->d_off) < sizeof(d_off), it means the former gets
    sign-extended before the comparison. If d_off held a valid 32-bit
    cookie which happened to have bit 31 set, then that is extended to
    bits 32-64, and the comparison fails.

    The patch therefore substitutes the existing signed comparison
    between objects of different size with an unsigned comparison,
    that looks only at whether or not bits 32-63 carry information or
    not. In essence the test allows d_off to be either an unsigned
    32-bit value or a 64-bit sign extension of a 32-bit signed value.

  - Finally, there is the assumption that the value '-1', as used by
    last_offset, cannot occur as a valid cookie. This is again related
    to point 2, but it occurs both for getdents64()-enabled kernels
    and older.

    The fix is to substitute the tests for whether or not last_offset
    is -1 with a test for whether or not the pointer 'dp' has changed.
    This is what we really want to test anyway, as we're trying to
    determine whether or not at least one entry has been filled in for
    the user.

Cheers,
  Trond

--- glibc-2.2/sysdeps/unix/sysv/linux/getdents.c.orig	Sat Dec  9 17:56:53 2000
+++ glibc-2.2/sysdeps/unix/sysv/linux/getdents.c	Tue Feb 13 14:38:48 2001
@@ -47,6 +47,7 @@
 #endif
 
 #define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
+#define test_overflow(VAR, SIZE) ((((VAR) < 0 ? -(VAR):(VAR)) >> 8*(SIZE)) != 0)
 
 extern int __syscall_getdents (int fd, char *__unbounded buf, unsigned int nbytes);
 extern int __syscall_getdents64 (int fd, char *__unbounded buf, unsigned int nbytes);
@@ -98,7 +99,6 @@
 __GETDENTS (int fd, char *buf, size_t nbytes)
 {
   DIRENT_TYPE *dp;
-  off_t last_offset = -1;
   ssize_t retval;
 
 #ifdef __NR_getdents64
@@ -126,6 +126,7 @@
 # endif
 	{
 	  struct kernel_dirent64 *kdp;
+  	  int64_t last_offset = 0;
 	  const size_t size_diff = (offsetof (struct kernel_dirent64, d_name)
 				   - offsetof (DIRENT_TYPE, d_name));
 
@@ -157,14 +158,14 @@
 	      if ((sizeof (dp->d_ino) != sizeof (kdp->d_ino)
 		   && dp->d_ino != d_ino)
 		  || (sizeof (dp->d_off) != sizeof (kdp->d_off)
-		      && dp->d_off != d_off))
+		      && test_overflow(d_off,sizeof(dp->d_off))))
 		{
 		  /* Overflow.  If there was at least one entry
 		     before this one, return them without error,
 		     otherwise signal overflow.  */
-		  if (last_offset != -1)
+		  if (dp != buf)
 		    {
-		      __lseek (fd, last_offset, SEEK_SET);
+		      __lseek64 (fd, last_offset, SEEK_SET);
 		      return (char *) dp - buf;
 		    }
 		  __set_errno (EOVERFLOW);
@@ -193,6 +194,7 @@
   {
     size_t red_nbytes;
     struct kernel_dirent *skdp, *kdp;
+    __kernel_off_t last_offset = 0;
     const size_t size_diff = (offsetof (DIRENT_TYPE, d_name)
 			      - offsetof (struct kernel_dirent, d_name));
 
@@ -221,7 +223,7 @@
 	  {
 	    /* Our heuristic failed.  We read too many entries.  Reset
 	       the stream.  */
-	    assert (last_offset != -1);
+	    assert (dp != buf);
 	    __lseek (fd, last_offset, SEEK_SET);
 
 	    if ((char *) dp == buf)


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]