patch fixing 297991: mmap changing a file descriptor current position

Bug caused by the following problem:
for each mmap, Valgrind reads the 1st 1024 bytes to detect
if this is an mmap-ed file containing debug info to decode.

Reading this 1Kb is done with VG_(pread). VG_(pread) should be
the equivalent of syscall pread but on linux, it is implemented as
a seek+read.

The patch implements VG_(pread) in terms of the underlying pread syscall.

Test mmap_fcntl_bug.c completed to also verify the fd current position
before and after the mmap.

tested on linux x86/amd64/ppc32/ppc64/s390.
(not tested on Darwin)
(manually tested on arm-android)

git-svn-id: svn://svn.valgrind.org/valgrind/trunk@12504

diff --git a/NEWS b/NEWS
index 65e79f8519d42305276c290e5be79f9f1fe8d7df..3199c3feceec04a7ceef0dbfe7e6940174ea5d2e 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -80,6 +80,7 @@ where XXXXXX is the bug number as listed below.
 n-i-bz  s390x: Shadow registers can now be examined using vgdb
 297078  gdbserver signal handling problems caused by diff vki nr/gdb nr
         and non reset of "C-ontinued" signal
+297991  Valgrind interferes with mmap()+ftell() 
 297992  Support systems missing WIFCONTINUED (e.g. pre-2.6.10 Linux) 
 
 Release 3.7.0 (5 November 2011)

diff --git a/coregrind/m_libcfile.c b/coregrind/m_libcfile.c
index fd11cb7bb5bf1654419f4a85dedebd41d6b6179c..281dfbec5053da84495bd4212414673579abaa24 100644 (file)
--- a/coregrind/m_libcfile.c
+++ b/coregrind/m_libcfile.c
@@ -232,7 +232,7 @@ Off64T VG_(lseek) ( Int fd, Off64T offset, Int whence )
 #    error "Unknown plat"
 #  endif
    /* if you change the error-reporting conventions of this, also
-      change VG_(pread) and all other usage points. */
+      change all usage points. */
 }
 
 
@@ -575,26 +575,38 @@ Int VG_(check_executable)(/*OUT*/Bool* is_setuid,
    return 0;
 }
 
-/* DDD: Note this moves (or at least, is believed to move) the file
-   pointer on Linux but doesn't on Darwin.  This inconsistency should
-   be fixed.  (In other words, why isn't the Linux version implemented
-   in terms of pread()?) */
 SysRes VG_(pread) ( Int fd, void* buf, Int count, OffT offset )
 {
    SysRes res;
-#  if defined(VGO_linux)
-   OffT off = VG_(lseek)( fd, offset, VKI_SEEK_SET);
-   if (off < 0)
-      return VG_(mk_SysRes_Error)( VKI_EINVAL );
-   res = VG_(do_syscall3)(__NR_read, fd, (UWord)buf, count );
+   // on 32 bits platforms, we receive a 32 bits OffT but
+   // we must extend it to pass a long long 64 bits.
+#  if defined(VGP_x86_linux)
+   vg_assert(sizeof(OffT) == 4);
+   res = VG_(do_syscall5)(__NR_pread64, fd, (UWord)buf, count, 
+                          offset, 0); // Little endian long long
+   return res;
+#  elif defined(VGP_arm_linux)
+   vg_assert(sizeof(OffT) == 4);
+   res = VG_(do_syscall5)(__NR_pread64, fd, (UWord)buf, count, 
+                          0, offset); // Big endian long long
+   return res;
+#  elif defined(VGP_ppc32_linux)
+   vg_assert(sizeof(OffT) == 4);
+   res = VG_(do_syscall6)(__NR_pread64, fd, (UWord)buf, count, 
+                          0, // Padding needed on PPC32
+                          0, offset); // Big endian long long
+   return res;
+#  elif defined(VGP_amd64_linux) \
+      || defined(VGP_ppc64_linux) || defined(VGP_s390x_linux) 
+   res = VG_(do_syscall4)(__NR_pread64, fd, (UWord)buf, count, offset);
    return res;
 #  elif defined(VGP_amd64_darwin)
    res = VG_(do_syscall4)(__NR_pread_nocancel, fd, (UWord)buf, count, offset);
    return res;
 #  elif defined(VGP_x86_darwin)
-   /* ppc32-darwin is the same, but with the args inverted */
+   vg_assert(sizeof(OffT) == 4);
    res = VG_(do_syscall5)(__NR_pread_nocancel, fd, (UWord)buf, count, 
-                          offset & 0xffffffff, offset >> 32);
+                          offset, 0);
    return res;
 #  else
 #    error "Unknown platform"

diff --git a/none/tests/mmap_fcntl_bug.c b/none/tests/mmap_fcntl_bug.c
index 056b89cfcc67a7596b1b8a50e69e06c763cd85f5..f49639a17689a9a7dc9778bb6ba44c190a454cf1 100644 (file)
--- a/none/tests/mmap_fcntl_bug.c
+++ b/none/tests/mmap_fcntl_bug.c
@@ -19,6 +19,7 @@ int main(int argc, char *argv[])
        const char *file = /* argv[1]; */
                           "mmap_fcntl_bug.c";
        int fd, status;
+        off_t initial;
 
        if (!file)
                errx(1, "Usage: %s <normal-file>", argv[0]);
@@ -27,6 +28,13 @@ int main(int argc, char *argv[])
        if (fd < 0)
                err(1, "Opening %s", file);
 
+        // reproduce bug 297991: mmap interferes with fd position
+        initial = lseek(fd, 123, SEEK_SET);
+        if (123 != initial)
+                err(1, "initial off_t differs from 123 (TEST FAILED)");
+        if (lseek(fd, 0, SEEK_CUR) != 123)
+                err(1, "zero offset from initial differs from 123 (TEST FAILED)");
+
        fl.l_type = F_WRLCK;
        fl.l_whence = SEEK_SET;
        fl.l_start = 0;
@@ -39,6 +47,8 @@ int main(int argc, char *argv[])
        /* If under valgrind, mmap re-opens and closes file, screwing us */
        if (mmap(NULL, getpagesize(), PROT_READ|PROT_WRITE, MAP_PRIVATE, fd, 0) == MAP_FAILED)
                err(1, "mmap of %s", file);
+        if (lseek(fd, 0, SEEK_CUR) != 123)
+                errx(1, "zero offset from initial after mmap differs from 123 (TEST FAILED)");
 
        switch (fork()) {
        case 0: