From: Philippe Waroquiers Date: Sat, 1 Sep 2018 21:36:42 +0000 (+0200) Subject: Fix 393146 failing assert "is_DebugInfo_active(di)" X-Git-Tag: VALGRIND_3_14_0~42 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d457604d498081e402cb223e1a364719dc0c0793;p=thirdparty%2Fvalgrind.git Fix 393146 failing assert "is_DebugInfo_active(di)" Some applications are mapping an object ro, and then unmaps it directly. In such a case, we have a di that contains obsolete fsm.maps (not matching OS mappings). The di for this unmapped object is not active, and has no dinfo (have_dinfo == False). (more generally, fsm.maps can contain a whole bunch of obsolete mappings). Later on, some other libs can be mapped with a mapping overlapping this obsolete mapping. A di that never had its debug info loaded can really be discarded, even if CG_(clo_keep_debuginfo). In such a case, it is normal to have to discard a not active di. (it might be better to keep fsm.maps in sync with the real OS mapping, but that is a much bigger change/fix). The FSM debug tracing was static, it is now dynamic according to debug loglevel >= 3. The below is an extract of the trace showing what happens. SYSCALL[4384,1](257) sys_openat ( 4294967196, 0x4244398(/usr/lib/x86_64-linux-gnu/qt5/plugins/platforms/libqeglfs.so), 524288 ) --> [async] ... SYSCALL[4384,1](257) ... [async] --> Success(0x3) SYSCALL[4384,1](72) sys_fcntl[ARG3=='arg'] ( 3, 2, 1 )[sync] --> Success(0x0) SYSCALL[4384,1](5) sys_newfstat ( 3, 0x1ffefff8b0 )[sync] --> Success(0x0) SYSCALL[4384,1](5) sys_newfstat ( 3, 0x1ffefff9c0 )[sync] --> Success(0x0) SYSCALL[4384,1](9) sys_mmap ( 0x0, 10520, 1, 1, 3, 0 )--4384-- di_notify_mmap-0: --4384-- di_notify_mmap-1: 0x4027000-0x4029fff r-- --4384-- di_notify_mmap-2: /usr/lib/x86_64-linux-gnu/qt5/plugins/platforms/libqeglfs.so --4384-- di_notify_mmap-3: is_rx_map 0, is_rw_map 0, is_ro_map 1 --4384-- di_notify_mmap-4: noting details in DebugInfo* at 0x10024CEA10 --4384-- di_notify_mmap-6: no dinfo loaded /usr/lib/x86_64-linux-gnu/qt5/plugins/platforms/libqeglfs.so (no rx or no rw mapping) --> [pre-success] Success(0x4027000) SYSCALL[4384,1](3) sys_close ( 3 )[sync] --> Success(0x0) SYSCALL[4384,1](11) sys_munmap ( 0x4027000, 10520 )[sync] --> Success(0x0) ^^^^ the above munmap has not cleaned up or removed anything in DebugInfo* at 0x10024CEA10 Later on, /usr/lib/x86_64-linux-gnu/qt5/plugins/platforms/libqxcb.so is mapped overlapping the memory where libqeglfs.so was mapped ro. Now, this cleans up the (useless) di that never had have_dinfo true, e.g. ------ start ELF OBJECT ------------------------------------------------------- ------ name = /usr/lib/x86_64-linux-gnu/qt5/plugins/platforms/libqxcb.so ... --4384-- Discarding syms at 0x0-0x0 in /usr/lib/x86_64-linux-gnu/qt5/plugins/platforms/libqeglfs.so (have_dinfo 0) (the 0x0-0x0 in the trace is because there was never any text mapping for libqeglfs.so). --- diff --git a/NEWS b/NEWS index e14e44c245..bd3c61d3e6 100644 --- a/NEWS +++ b/NEWS @@ -136,6 +136,7 @@ where XXXXXX is the bug number as listed below. 393023 callgrind_control risks using the wrong vgdb 393062 build-id ELF phdrs read causes "debuginfo reader: ensure_valid failed" 393099 posix_memalign() invalid write if alignment == 0 +393146 failing assert "is_DebugInfo_active(di)" 395709 PPC64 is missing support for the xvnegsp instruction 395682 Accept read-only PT_LOAD segments and .rodata by ld -z separate-code == 384727 diff --git a/coregrind/m_debuginfo/debuginfo.c b/coregrind/m_debuginfo/debuginfo.c index a30c7b44e5..0fc54bf1dd 100644 --- a/coregrind/m_debuginfo/debuginfo.c +++ b/coregrind/m_debuginfo/debuginfo.c @@ -34,6 +34,7 @@ #include "pub_core_vki.h" #include "pub_core_threadstate.h" #include "pub_core_debuginfo.h" /* self */ +#include "pub_core_debuglog.h" #include "pub_core_demangle.h" #include "pub_core_libcbase.h" #include "pub_core_libcassert.h" @@ -68,10 +69,6 @@ #endif -/* Set this to 1 to enable debug printing for the - should-we-load-debuginfo-now? finite state machine. */ -#define DEBUG_FSM 0 - /* Set this to 1 to enable somewhat minimal debug printing for the debuginfo-epoch machinery. */ #define DEBUG_EPOCHS 0 @@ -443,27 +440,31 @@ static void free_DebugInfo ( DebugInfo* di ) */ static void discard_or_archive_DebugInfo ( DebugInfo* di ) { - const HChar* reason = "munmap"; - const Bool archive = VG_(clo_keep_debuginfo); + /* di->have_dinfo can be False when an object is mapped "ro" + and then unmapped before the debug info is loaded. + In other words, debugInfo_list might contain many di that have + no OS mappings, even if their fsm.maps still contain mappings. + Such (left over) mappings can overlap with real mappings. + Search for FSMMAPSNOTCLEANEDUP: below for more details. */ + /* If a di has no dinfo, we can discard even if VG_(clo_keep_debuginfo). */ + const Bool archive = VG_(clo_keep_debuginfo) && di->have_dinfo; DebugInfo** prev_next_ptr = &debugInfo_list; DebugInfo* curr = debugInfo_list; - /* It must be active! */ - vg_assert(is_DebugInfo_active(di)); + /* If di->have_dinfo, then it must be active! */ + vg_assert(!di->have_dinfo || is_DebugInfo_active(di)); while (curr) { if (curr == di) { /* Found it; (remove from list and free it), or archive it. */ - if (curr->have_dinfo - && (VG_(clo_verbosity) > 1 || VG_(clo_trace_redir))) - VG_(message)(Vg_DebugMsg, - "%s syms at %#lx-%#lx in %s due to %s()\n", - archive ? "Archiving" : "Discarding", - di->text_avma, - di->text_avma + di->text_size, - curr->fsm.filename ? curr->fsm.filename - : "???", - reason); + if (VG_(clo_verbosity) > 1 || VG_(clo_trace_redir)) + VG_(dmsg)("%s syms at %#lx-%#lx in %s (have_dinfo %d)\n", + archive ? "Archiving" : "Discarding", + di->text_avma, + di->text_avma + di->text_size, + curr->fsm.filename ? curr->fsm.filename + : "???", + curr->have_dinfo); vg_assert(*prev_next_ptr == curr); if (!archive) { *prev_next_ptr = curr->next; @@ -1021,7 +1022,7 @@ ULong VG_(di_notify_mmap)( Addr a, Bool allow_SkFileV, Int use_fd ) Int actual_fd, oflags; SysRes preadres; HChar buf1k[1024]; - Bool debug = (DEBUG_FSM != 0); + const Bool debug = VG_(debugLog_getLevel)() >= 3; SysRes statres; struct vg_stat statbuf; @@ -1034,11 +1035,11 @@ ULong VG_(di_notify_mmap)( Addr a, Bool allow_SkFileV, Int use_fd ) vg_assert(seg); if (debug) { - VG_(printf)("di_notify_mmap-0:\n"); - VG_(printf)("di_notify_mmap-1: %#lx-%#lx %c%c%c\n", - seg->start, seg->end, - seg->hasR ? 'r' : '-', - seg->hasW ? 'w' : '-',seg->hasX ? 'x' : '-' ); + VG_(dmsg)("di_notify_mmap-0:\n"); + VG_(dmsg)("di_notify_mmap-1: %#lx-%#lx %c%c%c\n", + seg->start, seg->end, + seg->hasR ? 'r' : '-', + seg->hasW ? 'w' : '-',seg->hasX ? 'x' : '-' ); } /* guaranteed by aspacemgr-linux.c, sane_NSegment() */ @@ -1064,7 +1065,7 @@ ULong VG_(di_notify_mmap)( Addr a, Bool allow_SkFileV, Int use_fd ) return 0; if (debug) - VG_(printf)("di_notify_mmap-2: %s\n", filename); + VG_(dmsg)("di_notify_mmap-2: %s\n", filename); /* Only try to read debug information from regular files. */ statres = VG_(stat)(filename, &statbuf); @@ -1167,9 +1168,9 @@ ULong VG_(di_notify_mmap)( Addr a, Bool allow_SkFileV, Int use_fd ) # endif if (debug) - VG_(printf)("di_notify_mmap-3: " - "is_rx_map %d, is_rw_map %d, is_ro_map %d\n", - (Int)is_rx_map, (Int)is_rw_map, (Int)is_ro_map); + VG_(dmsg)("di_notify_mmap-3: " + "is_rx_map %d, is_rw_map %d, is_ro_map %d\n", + (Int)is_rx_map, (Int)is_rw_map, (Int)is_ro_map); /* Ignore mappings with permissions we can't possibly be interested in. */ if (!(is_rx_map || is_rw_map || is_ro_map)) @@ -1253,15 +1254,15 @@ ULong VG_(di_notify_mmap)( Addr a, Bool allow_SkFileV, Int use_fd ) mjw/thh. */ if (di->have_dinfo) { if (debug) - VG_(printf)("di_notify_mmap-4x: " - "ignoring mapping because we already read debuginfo " - "for DebugInfo* %p\n", di); + VG_(dmsg)("di_notify_mmap-4x: " + "ignoring mapping because we already read debuginfo " + "for DebugInfo* %p\n", di); return 0; } if (debug) - VG_(printf)("di_notify_mmap-4: " - "noting details in DebugInfo* at %p\n", di); + VG_(dmsg)("di_notify_mmap-4: " + "noting details in DebugInfo* at %p\n", di); /* Note the details about the mapping. */ DebugInfoMapping map; @@ -1285,13 +1286,14 @@ ULong VG_(di_notify_mmap)( Addr a, Bool allow_SkFileV, Int use_fd ) already read debuginfo for this object. So let's do so now. Yee-ha! */ if (debug) - VG_(printf)("di_notify_mmap-5: " - "achieved accept state for %s\n", filename); + VG_(dmsg)("di_notify_mmap-5: " + "achieved accept state for %s\n", filename); return di_notify_ACHIEVE_ACCEPT_STATE ( di ); } else { - /* If we don't have an rx and rw mapping, or if we already have - debuginfo for this mapping for whatever reason, go no - further. */ + /* If we don't have an rx and rw mapping, go no further. */ + if (debug) + VG_(dmsg)("di_notify_mmap-6: " + "no dinfo loaded %s (no rx or no rw mapping)\n", filename); return 0; } } @@ -1336,16 +1338,16 @@ void VG_(di_notify_mprotect)( Addr a, SizeT len, UInt prot ) declaration of struct _DebugInfoFSM for details. */ void VG_(di_notify_vm_protect)( Addr a, SizeT len, UInt prot ) { - Bool debug = (DEBUG_FSM != 0); + const Bool debug = VG_(debugLog_getLevel)() >= 3; Bool r_ok = toBool(prot & VKI_PROT_READ); Bool w_ok = toBool(prot & VKI_PROT_WRITE); Bool x_ok = toBool(prot & VKI_PROT_EXEC); if (debug) { - VG_(printf)("di_notify_vm_protect-0:\n"); - VG_(printf)("di_notify_vm_protect-1: %#lx-%#lx %c%c%c\n", - a, a + len - 1, - r_ok ? 'r' : '-', w_ok ? 'w' : '-', x_ok ? 'x' : '-' ); + VG_(dmsg)("di_notify_vm_protect-0:\n"); + VG_(dmsg)("di_notify_vm_protect-1: %#lx-%#lx %c%c%c\n", + a, a + len - 1, + r_ok ? 'r' : '-', w_ok ? 'w' : '-', x_ok ? 'x' : '-' ); } Bool do_nothing = True; @@ -1354,8 +1356,8 @@ void VG_(di_notify_vm_protect)( Addr a, SizeT len, UInt prot ) # endif if (do_nothing /* wrong platform */) { if (debug) - VG_(printf)("di_notify_vm_protect-2: wrong platform, " - "doing nothing.\n"); + VG_(dmsg)("di_notify_vm_protect-2: wrong platform, " + "doing nothing.\n"); return; } @@ -1367,7 +1369,7 @@ void VG_(di_notify_vm_protect)( Addr a, SizeT len, UInt prot ) is found, conclude we're in an accept state and read debuginfo accordingly. */ if (debug) - VG_(printf)("di_notify_vm_protect-3: looking for existing DebugInfo*\n"); + VG_(dmsg)("di_notify_vm_protect-3: looking for existing DebugInfo*\n"); DebugInfo* di; DebugInfoMapping *map = NULL; Word i; @@ -1397,8 +1399,8 @@ void VG_(di_notify_vm_protect)( Addr a, SizeT len, UInt prot ) return; /* didn't find anything */ if (debug) - VG_(printf)("di_notify_vm_protect-4: found existing DebugInfo* at %p\n", - di); + VG_(dmsg)("di_notify_vm_protect-4: found existing DebugInfo* at %p\n", + di); /* Do the upgrade. Simply update the flags of the mapping and pretend we never saw the RO map at all. */ @@ -1419,7 +1421,7 @@ void VG_(di_notify_vm_protect)( Addr a, SizeT len, UInt prot ) /* Check if we're now in an accept state and read debuginfo. Finally. */ if (di->fsm.have_rx_map && di->fsm.have_rw_map && !di->have_dinfo) { if (debug) - VG_(printf)("di_notify_vm_protect-5: " + VG_(dmsg)("di_notify_vm_protect-5: " "achieved accept state for %s\n", di->fsm.filename); ULong di_handle __attribute__((unused)) = di_notify_ACHIEVE_ACCEPT_STATE( di );