VG_(di_notify_mmap): once we've read debuginfo for an object, ignore all further...

Once we've read debuginfo for an object, ignore all further mappings.  If we
don't do that, applications that mmap in their own objects to inspect them for
whatever reason, will cause "irrelevant" mappings to be recorded in the
object's fsm.maps table.  This can lead to serious problems later on.

This has become necessary because 64aa729bfae71561505a40c12755bd6b55bb3061 of
Thu Jul 12 2018 (the fix for bug 395682) started recording readonly segments
in the fsm.maps table, where before they were ignored.

diff --git a/coregrind/m_debuginfo/debuginfo.c b/coregrind/m_debuginfo/debuginfo.c
index c36d4980486c14b797681350b864fc1d39a7ac24..55c05cbe06fd58af02fe02cb22ceab8dadf8c4c2 100644 (file)
--- a/coregrind/m_debuginfo/debuginfo.c
+++ b/coregrind/m_debuginfo/debuginfo.c
@@ -1200,6 +1200,32 @@ ULong VG_(di_notify_mmap)( Addr a, Bool allow_SkFileV, Int use_fd )
    di = find_or_create_DebugInfo_for( filename );
    vg_assert(di);
 
+   /* Ignore all mappings for this filename once we've read debuginfo for it.
+      This avoids the confusion of picking up "irrelevant" mappings in
+      applications which mmap their objects outside of ld.so, for example
+      Firefox's Gecko profiler.
+
+      What happens in that case is: the application maps the object "ro" for
+      whatever reason.  We record the mapping di->fsm.maps.  The application
+      later unmaps the object.  However, the mapping is not removed from
+      di->fsm.maps.  Later, when some other (unrelated) object is mapped (via
+      ld.so) into that address space, we first unload any debuginfo that has a
+      mapping intersecting that area.  That means we will end up incorrectly
+      unloading debuginfo for the object with the "irrelevant" mappings.  This
+      causes various problems, not least because it can unload the debuginfo
+      for libc.so and so cause malloc intercepts to become un-intercepted.
+
+      This fix assumes that all mappings made once we've read debuginfo for
+      an object are irrelevant.  I think that's OK, but need to check with
+      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);
+      return 0;
+   }
+
    if (debug)
       VG_(printf)("di_notify_mmap-4: "
                   "noting details in DebugInfo* at %p\n", di);
@@ -1220,7 +1246,8 @@ ULong VG_(di_notify_mmap)( Addr a, Bool allow_SkFileV, Int use_fd )
    di->fsm.have_ro_map |= is_ro_map;
 
    /* So, finally, are we in an accept state? */
-   if (di->fsm.have_rx_map && di->fsm.have_rw_map && !di->have_dinfo) {
+   vg_assert(!di->have_dinfo);
+   if (di->fsm.have_rx_map && di->fsm.have_rw_map) {
       /* Ok, so, finally, we found what we need, and we haven't
          already read debuginfo for this object.  So let's do so now.
          Yee-ha! */