From: Julian Seward Date: Mon, 24 Oct 2011 08:53:03 +0000 (+0000) Subject: Don't break fcntl locks when program does mmap. #280965. X-Git-Tag: svn/VALGRIND_3_7_0~15 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=fc489f11018b262642b062b398b198efc1031a56;p=thirdparty%2Fvalgrind.git Don't break fcntl locks when program does mmap. #280965. (Rusty Russell, rusty@rustcorp.com.au) tdb uses fcntl locks and mmap, and some of the tests fail under valgrind. strace showed valgrind opening the tdb file, reading 1024 bytes, then closing it. This is not allowed: POSIX says if you open and close a file, all fcntl locks on it are dropped (insane, yes). Finally got around to hacking the source to track this down: di_notify_mmap is doing the damage. The simplest fix was to hand in an optional fd for it to use, then have it do pread. I had to fix your pread; surely this should seek back even if the platform doesn't have pread support? git-svn-id: svn://svn.valgrind.org/valgrind/trunk@12224 --- diff --git a/coregrind/m_debuginfo/debuginfo.c b/coregrind/m_debuginfo/debuginfo.c index cf7bdf624d..4096d5c7d4 100644 --- a/coregrind/m_debuginfo/debuginfo.c +++ b/coregrind/m_debuginfo/debuginfo.c @@ -660,6 +660,10 @@ static ULong di_notify_ACHIEVE_ACCEPT_STATE ( struct _DebugInfo* di ) carefully control when the thing will read symbols from the Valgrind executable itself. + If use_fd is not -1, that is used instead of the filename; this + avoids perturbing fcntl locks, which are released by simply + re-opening and closing the same file (even via different fd!). + If a call to VG_(di_notify_mmap) causes debug info to be read, then the returned ULong is an abstract handle which can later be used to refer to the debuginfo read as a result of this specific mapping, @@ -667,19 +671,21 @@ static ULong di_notify_ACHIEVE_ACCEPT_STATE ( struct _DebugInfo* di ) will be one or above. If the returned value is zero, no debug info was read. */ -ULong VG_(di_notify_mmap)( Addr a, Bool allow_SkFileV ) +ULong VG_(di_notify_mmap)( Addr a, Bool allow_SkFileV, Int use_fd ) { NSegment const * seg; HChar* filename; Bool is_rx_map, is_rw_map, is_ro_map; DebugInfo* di; - SysRes fd; - Int nread, oflags; + Int actual_fd, oflags; + SysRes preadres; HChar buf1k[1024]; Bool debug = False; SysRes statres; struct vg_stat statbuf; + vg_assert(use_fd >= -1); + /* In short, figure out if this mapping is of interest to us, and if so, try to guess what ld.so is doing and when/if we should read debug info. */ @@ -817,36 +823,46 @@ ULong VG_(di_notify_mmap)( Addr a, Bool allow_SkFileV ) # if defined(VKI_O_LARGEFILE) oflags |= VKI_O_LARGEFILE; # endif - fd = VG_(open)( filename, oflags, 0 ); - if (sr_isError(fd)) { - if (sr_Err(fd) != VKI_EACCES) { - DebugInfo fake_di; - VG_(memset)(&fake_di, 0, sizeof(fake_di)); - fake_di.fsm.filename = filename; - ML_(symerr)(&fake_di, True, "can't open file to inspect ELF header"); + + if (use_fd == -1) { + SysRes fd = VG_(open)( filename, oflags, 0 ); + if (sr_isError(fd)) { + if (sr_Err(fd) != VKI_EACCES) { + DebugInfo fake_di; + VG_(memset)(&fake_di, 0, sizeof(fake_di)); + fake_di.fsm.filename = filename; + ML_(symerr)(&fake_di, True, + "can't open file to inspect ELF header"); + } + return 0; } - return 0; + actual_fd = sr_Res(fd); + } else { + actual_fd = use_fd; } - nread = VG_(read)( sr_Res(fd), buf1k, sizeof(buf1k) ); - VG_(close)( sr_Res(fd) ); - if (nread == 0) - return 0; - if (nread < 0) { + preadres = VG_(pread)( actual_fd, buf1k, sizeof(buf1k), 0 ); + if (use_fd == -1) { + VG_(close)( actual_fd ); + } + + if (sr_isError(preadres)) { DebugInfo fake_di; VG_(memset)(&fake_di, 0, sizeof(fake_di)); fake_di.fsm.filename = filename; ML_(symerr)(&fake_di, True, "can't read file to inspect ELF header"); return 0; } - vg_assert(nread > 0 && nread <= sizeof(buf1k) ); + if (sr_Res(preadres) == 0) + return 0; + vg_assert(sr_Res(preadres) > 0 && sr_Res(preadres) <= sizeof(buf1k) ); /* We're only interested in mappings of object files. */ # if defined(VGO_linux) - if (!ML_(is_elf_object_file)( buf1k, (SizeT)nread )) + if (!ML_(is_elf_object_file)( buf1k, (SizeT)sr_Res(preadres) )) return 0; # elif defined(VGO_darwin) - if (!ML_(is_macho_object_file)( buf1k, (SizeT)nread )) + if (!ML_(is_macho_object_file)( buf1k, (SizeT)sr_Res(preadres) )) return 0; # else # error "unknown OS" diff --git a/coregrind/m_main.c b/coregrind/m_main.c index 17923227fc..d93888d055 100644 --- a/coregrind/m_main.c +++ b/coregrind/m_main.c @@ -1929,7 +1929,8 @@ Int valgrind_main ( Int argc, HChar **argv, HChar **envp ) be True here so that we read info from the valgrind executable itself. */ for (i = 0; i < n_seg_starts; i++) { - anu.ull = VG_(di_notify_mmap)( seg_starts[i], True/*allow_SkFileV*/ ); + anu.ull = VG_(di_notify_mmap)( seg_starts[i], True/*allow_SkFileV*/, + -1/*Don't use_fd*/); /* anu.ull holds the debuginfo handle returned by di_notify_mmap, if any. */ if (anu.ull > 0) { @@ -1949,8 +1950,10 @@ Int valgrind_main ( Int argc, HChar **argv, HChar **envp ) /* show them all to the debug info reader. Don't read from V segments (unlike Linux) */ // GrP fixme really? - for (i = 0; i < n_seg_starts; i++) - VG_(di_notify_mmap)( seg_starts[i], False/*don't allow_SkFileV*/ ); + for (i = 0; i < n_seg_starts; i++) { + VG_(di_notify_mmap)( seg_starts[i], False/*don't allow_SkFileV*/, + -1/*don't use_fd*/); + } VG_(free)( seg_starts ); } diff --git a/coregrind/m_syswrap/syswrap-darwin.c b/coregrind/m_syswrap/syswrap-darwin.c index 27923bfcd0..2778957064 100644 --- a/coregrind/m_syswrap/syswrap-darwin.c +++ b/coregrind/m_syswrap/syswrap-darwin.c @@ -3550,7 +3550,8 @@ POST(mmap) if (RES != -1) { ML_(notify_core_and_tool_of_mmap)(RES, ARG2, ARG3, ARG4, ARG5, ARG6); // Try to load symbols from the region - VG_(di_notify_mmap)( (Addr)RES, False/*allow_SkFileV*/ ); + VG_(di_notify_mmap)( (Addr)RES, False/*allow_SkFileV*/, + -1/*don't use_fd*/ ); } } diff --git a/coregrind/m_syswrap/syswrap-generic.c b/coregrind/m_syswrap/syswrap-generic.c index f140f403d0..eca71229ad 100644 --- a/coregrind/m_syswrap/syswrap-generic.c +++ b/coregrind/m_syswrap/syswrap-generic.c @@ -2070,7 +2070,7 @@ ML_(generic_PRE_sys_mmap) ( ThreadId tid, ); /* Load symbols? */ di_handle = VG_(di_notify_mmap)( (Addr)sr_Res(sres), - False/*allow_SkFileV*/ ); + False/*allow_SkFileV*/, (Int)arg5 ); /* Notify the tool. */ notify_tool_of_mmap( (Addr)sr_Res(sres), /* addr kernel actually assigned */ diff --git a/coregrind/pub_core_debuginfo.h b/coregrind/pub_core_debuginfo.h index dbe6a28a8d..fa1b129550 100644 --- a/coregrind/pub_core_debuginfo.h +++ b/coregrind/pub_core_debuginfo.h @@ -55,9 +55,15 @@ extern void VG_(di_initialise) ( void ); refer to the debuginfo read as a result of this specific mapping, in later queries to m_debuginfo. In this case the handle value will be one or above. If the returned value is zero, no debug info - was read. */ + was read. + + For VG_(di_notify_mmap), if use_fd is not -1, that is used instead + of the filename; this avoids perturbing fcntl locks, which are + released by simply re-opening and closing the same file (even via + different fd!). +*/ #if defined(VGO_linux) || defined(VGO_darwin) -extern ULong VG_(di_notify_mmap)( Addr a, Bool allow_SkFileV ); +extern ULong VG_(di_notify_mmap)( Addr a, Bool allow_SkFileV, Int use_fd ); extern void VG_(di_notify_munmap)( Addr a, SizeT len ); diff --git a/none/tests/Makefile.am b/none/tests/Makefile.am index f5828d959a..a9c498f0e4 100644 --- a/none/tests/Makefile.am +++ b/none/tests/Makefile.am @@ -92,6 +92,8 @@ EXTRA_DIST = \ manythreads.stdout.exp manythreads.stderr.exp manythreads.vgtest \ map_unaligned.stderr.exp map_unaligned.vgtest \ map_unmap.stderr.exp map_unmap.stdout.exp map_unmap.vgtest \ + mmap_fcntl_bug.vgtest mmap_fcntl_bug.stdout.exp \ + mmap_fcntl_bug.stderr.exp \ mq.stderr.exp mq.vgtest \ munmap_exe.stderr.exp munmap_exe.vgtest \ nestedfns.stderr.exp nestedfns.stdout.exp nestedfns.vgtest \ @@ -168,6 +170,7 @@ check_PROGRAMS = \ fdleak_fcntl fdleak_ipv4 fdleak_open fdleak_pipe \ fdleak_socketpair \ floored fork fucomip \ + mmap_fcntl_bug \ munmap_exe map_unaligned map_unmap mq \ nestedfns \ pending \ diff --git a/none/tests/mmap_fcntl_bug.c b/none/tests/mmap_fcntl_bug.c new file mode 100644 index 0000000000..056b89cfcc --- /dev/null +++ b/none/tests/mmap_fcntl_bug.c @@ -0,0 +1,68 @@ + +/* Test program to demonstrate valgrind breaking fcntl locks during + * mmap. Feed it a r/w file, such as its own source code. */ + +/* See bug 280965. */ + +#include +#include +#include +#include +#include +#include +#include +#include + +int main(int argc, char *argv[]) +{ + struct flock fl; + const char *file = /* argv[1]; */ + "mmap_fcntl_bug.c"; + int fd, status; + + if (!file) + errx(1, "Usage: %s ", argv[0]); + + fd = open(file, O_RDWR); + if (fd < 0) + err(1, "Opening %s", file); + + fl.l_type = F_WRLCK; + fl.l_whence = SEEK_SET; + fl.l_start = 0; + fl.l_len = 1; + + /* I'm assuming noone else tries to lock this! */ + if (fcntl(fd, F_SETLK, &fl) != 0) + err(1, "Locking %s", file); + + /* 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); + + switch (fork()) { + case 0: + /* Child. Lock should fail. */ + if (fcntl(fd, F_SETLK, &fl) == 0) + exit(1); + exit(0); + case -1: + err(1, "Fork failed"); + } + + if (wait(&status) == -1) + err(1, "Child vanished?"); + + if (!WIFEXITED(status)) + errx(1, "Child died with signal %i", WTERMSIG(status)); + + switch (WEXITSTATUS(status)) { + case 1: + errx(1, "Child got lock, we must have dropped it (TEST FAILED)"); + case 0: + fprintf(stderr, "Child exited with zero (TEST PASSED).\n"); + return 0; + default: + errx(1, "Child weird exit status %i", WEXITSTATUS(status)); + } +} diff --git a/none/tests/mmap_fcntl_bug.stderr.exp b/none/tests/mmap_fcntl_bug.stderr.exp new file mode 100644 index 0000000000..7885ee97b5 --- /dev/null +++ b/none/tests/mmap_fcntl_bug.stderr.exp @@ -0,0 +1 @@ +Child exited with zero (TEST PASSED). diff --git a/none/tests/mmap_fcntl_bug.stdout.exp b/none/tests/mmap_fcntl_bug.stdout.exp new file mode 100644 index 0000000000..e69de29bb2 diff --git a/none/tests/mmap_fcntl_bug.vgtest b/none/tests/mmap_fcntl_bug.vgtest new file mode 100644 index 0000000000..47a4e5e3ae --- /dev/null +++ b/none/tests/mmap_fcntl_bug.vgtest @@ -0,0 +1,2 @@ +prog: mmap_fcntl_bug +vgopts: -q