|
From: <sv...@va...> - 2012-04-13 23:07:36
|
philippe 2012-04-14 00:07:29 +0100 (Sat, 14 Apr 2012)
New Revision: 12504
Log:
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)
Modified files:
trunk/NEWS
trunk/coregrind/m_libcfile.c
trunk/none/tests/mmap_fcntl_bug.c
Modified: trunk/coregrind/m_libcfile.c (+24 -12)
===================================================================
--- trunk/coregrind/m_libcfile.c 2012-04-13 18:27:40 +01:00 (rev 12503)
+++ trunk/coregrind/m_libcfile.c 2012-04-14 00:07:29 -23:00 (rev 12504)
@@ -232,7 +232,7 @@
# 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 @@
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"
Modified: trunk/none/tests/mmap_fcntl_bug.c (+10 -0)
===================================================================
--- trunk/none/tests/mmap_fcntl_bug.c 2012-04-13 18:27:40 +01:00 (rev 12503)
+++ trunk/none/tests/mmap_fcntl_bug.c 2012-04-14 00:07:29 -23:00 (rev 12504)
@@ -19,6 +19,7 @@
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 @@
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 @@
/* 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:
Modified: trunk/NEWS (+1 -0)
===================================================================
--- trunk/NEWS 2012-04-13 18:27:40 +01:00 (rev 12503)
+++ trunk/NEWS 2012-04-14 00:07:29 -23:00 (rev 12504)
@@ -80,6 +80,7 @@
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)
|