From 8e1d13bd11f0064b39896450e15e9d924866c478 Mon Sep 17 00:00:00 2001 From: Paul Floyd Date: Thu, 28 Dec 2023 10:27:18 +0100 Subject: [PATCH] Bug 479041 - Executables without RW sections do not trigger debuginfo reading The main change is to not assume that there is always 1 and only 1 RW segment. Now the rw segment count is obtained from the macho segments. I've had to make several changes to remove asserts that checked that there is always 1 or more rw segments. I don't think that this really affects 'normal' C and C++ compiled binaries. There is one exp-bbv testcase (x86/million) that is written in assembler and was failing until I removed all of the asserts. There's still a bit more work to do. 1. Handle fat binaries - are these still a thing (with "apple silicon")? 2. Use a dynamically sized buffer for the segments rather than just 4k. --- coregrind/m_debuginfo/debuginfo.c | 22 ++++++++++---------- coregrind/m_debuginfo/priv_readmacho.h | 4 ++-- coregrind/m_debuginfo/readelf.c | 2 -- coregrind/m_debuginfo/readmacho.c | 28 +++++++++++++++++++++----- coregrind/m_debuginfo/storage.c | 2 +- tests/filter_stderr_basic.in | 3 +++ 6 files changed, 40 insertions(+), 21 deletions(-) diff --git a/coregrind/m_debuginfo/debuginfo.c b/coregrind/m_debuginfo/debuginfo.c index eed134be85..e4450afc47 100644 --- a/coregrind/m_debuginfo/debuginfo.c +++ b/coregrind/m_debuginfo/debuginfo.c @@ -701,7 +701,6 @@ static void check_CFSI_related_invariants ( const DebugInfo* di ) been successfully read. And that shouldn't happen until we have both a r-x and rw- mapping for the object. Hence: */ vg_assert(di->fsm.have_rx_map); - vg_assert(di->fsm.rw_map_count); for (i = 0; i < VG_(sizeXA)(di->fsm.maps); i++) { const DebugInfoMapping* map = VG_(indexXA)(di->fsm.maps, i); /* We are interested in r-x mappings only */ @@ -1171,7 +1170,11 @@ ULong VG_(di_notify_mmap)( Addr a, Bool allow_SkFileV, Int use_fd ) Int actual_fd, oflags; #if defined(VGO_darwin) SysRes preadres; - HChar buf1k[1024]; + // @todo PJF make this dynamic + // that probably means reading the sizeofcmds from the mach_header then + // allocating enough space for it + // and then one day maybe doing something for fat binaries + HChar buf4k[4096]; #else Bool elf_ok; #endif @@ -1341,7 +1344,7 @@ ULong VG_(di_notify_mmap)( Addr a, Bool allow_SkFileV, Int use_fd ) #if defined(VGO_darwin) /* Peer at the first few bytes of the file, to see if it is an ELF */ /* object file. Ignore the file if we do not have read permission. */ - VG_(memset)(buf1k, 0, sizeof(buf1k)); + VG_(memset)(buf4k, 0, sizeof(buf4k)); #endif oflags = VKI_O_RDONLY; @@ -1368,7 +1371,7 @@ ULong VG_(di_notify_mmap)( Addr a, Bool allow_SkFileV, Int use_fd ) } #if defined(VGO_darwin) - preadres = VG_(pread)( actual_fd, buf1k, sizeof(buf1k), 0 ); + preadres = VG_(pread)( actual_fd, buf4k, sizeof(buf4k), 0 ); if (use_fd == -1) { VG_(close)( actual_fd ); } @@ -1383,6 +1386,10 @@ ULong VG_(di_notify_mmap)( Addr a, Bool allow_SkFileV, Int use_fd ) if (sr_Res(preadres) == 0) return 0; vg_assert(sr_Res(preadres) > 0 && sr_Res(preadres) <= sizeof(buf1k) ); + + if (!ML_(is_macho_object_file)( buf1k, (SizeT)sr_Res(preadres) )) + return 0; + rw_load_count = 1; #endif /* We're only interested in mappings of object files. */ @@ -1400,12 +1407,6 @@ ULong VG_(di_notify_mmap)( Addr a, Bool allow_SkFileV, Int use_fd ) return 0; } -# elif defined(VGO_darwin) - if (!ML_(is_macho_object_file)( buf1k, (SizeT)sr_Res(preadres) )) - return 0; - rw_load_count = 1; -# else -# error "unknown OS" # endif /* See if we have a DebugInfo for this filename. If not, @@ -1466,7 +1467,6 @@ ULong VG_(di_notify_mmap)( Addr a, Bool allow_SkFileV, Int use_fd ) /* So, finally, are we in an accept state? */ vg_assert(!di->have_dinfo); if (di->fsm.have_rx_map && - rw_load_count >= 1 && di->fsm.rw_map_count == rw_load_count) { /* Ok, so, finally, we found what we need, and we haven't already read debuginfo for this object. So let's do so now. diff --git a/coregrind/m_debuginfo/priv_readmacho.h b/coregrind/m_debuginfo/priv_readmacho.h index 6b6425a63a..b5172878ce 100644 --- a/coregrind/m_debuginfo/priv_readmacho.h +++ b/coregrind/m_debuginfo/priv_readmacho.h @@ -34,8 +34,8 @@ #include "pub_core_debuginfo.h" // DebugInfo /* Identify a Mach-O object file by peering at the first few bytes of - it. */ -extern Bool ML_(is_macho_object_file)( const void* buf, SizeT size ); + it. Also count the number of RW segements. */ +extern Bool ML_(check_macho_and_get_rw_loads)( const void* buf, SizeT size, Int* rw_loads ); /* The central function for reading Mach-O debug info. For the object/exe specified by the DebugInfo, find Mach-O sections, then read diff --git a/coregrind/m_debuginfo/readelf.c b/coregrind/m_debuginfo/readelf.c index 16b5340a53..3b986bb48b 100644 --- a/coregrind/m_debuginfo/readelf.c +++ b/coregrind/m_debuginfo/readelf.c @@ -1959,7 +1959,6 @@ Bool ML_(read_elf_object) ( struct _DebugInfo* di ) vg_assert(di); vg_assert(di->fsm.have_rx_map == True); - vg_assert(di->fsm.rw_map_count >= 1); vg_assert(di->have_dinfo == False); vg_assert(di->fsm.filename); vg_assert(!di->symtab); @@ -1990,7 +1989,6 @@ Bool ML_(read_elf_object) ( struct _DebugInfo* di ) vg_assert(VG_IS_PAGE_ALIGNED(map->avma)); } vg_assert(has_nonempty_rx); - vg_assert(has_nonempty_rw); } /* ---------------------------------------------------------- diff --git a/coregrind/m_debuginfo/readmacho.c b/coregrind/m_debuginfo/readmacho.c index 33cc037b57..e93bae7941 100644 --- a/coregrind/m_debuginfo/readmacho.c +++ b/coregrind/m_debuginfo/readmacho.c @@ -93,7 +93,7 @@ memory that falls entirely inside the primary image. */ -Bool ML_(is_macho_object_file)( const void* buf, SizeT szB ) +Bool ML_(check_macho_and_get_rw_loads)( const void* buf, SizeT szB, Int* rw_loads ) { /* (JRS: the Mach-O headers might not be in this mapped data, because we only mapped a page for this initial check, @@ -108,19 +108,39 @@ Bool ML_(is_macho_object_file)( const void* buf, SizeT szB ) can to establish whether or not we're looking at something sane. */ + /* @todo PJF change function signature to pass in file handle + Read MACH_HEADER to determine sizeofcommands + Allocate a dynamic buffer for the commands. */ + const struct fat_header* fh_be = buf; const struct MACH_HEADER* mh = buf; vg_assert(buf); + vg_assert(rw_loads); if (szB < sizeof(struct fat_header)) return False; - if (VG_(ntohl)(fh_be->magic) == FAT_MAGIC) + if (VG_(ntohl)(fh_be->magic) == FAT_MAGIC) { + // @todo PJF not yet handled, previous behaviour was to assume that the count is 1 + *rw_loads = 1; return True; + } if (szB < sizeof(struct MACH_HEADER)) return False; - if (mh->magic == MAGIC) + if (mh->magic == MAGIC) { + const struct load_command* lc = (const struct load_command*)((const char*)buf + sizeof(struct MACH_HEADER)); + for (unsigned int i = 0U; i < mh->ncmds; ++i) { + if (lc->cmd == LC_SEGMENT_CMD) { + const struct SEGMENT_COMMAND* sc = (const struct SEGMENT_COMMAND*)lc; + if (sc->initprot == 3) { + ++*rw_loads; + } + } + const char* tmp = (const char*)lc + lc->cmdsize; + lc = (const struct load_command*)tmp; + } return True; + } return False; } @@ -714,7 +734,6 @@ Bool ML_(read_macho_debug_info)( struct _DebugInfo* di ) /* This should be ensured by our caller (that we're in the accept state). */ vg_assert(di->fsm.have_rx_map); - vg_assert(di->fsm.rw_map_count); for (i = 0; i < VG_(sizeXA)(di->fsm.maps); i++) { const DebugInfoMapping* map = VG_(indexXA)(di->fsm.maps, i); @@ -726,7 +745,6 @@ Bool ML_(read_macho_debug_info)( struct _DebugInfo* di ) break; } vg_assert(rx_map); - vg_assert(rw_map); if (VG_(clo_verbosity) > 1) VG_(message)(Vg_DebugMsg, diff --git a/coregrind/m_debuginfo/storage.c b/coregrind/m_debuginfo/storage.c index 961d767b8a..148de6f17e 100644 --- a/coregrind/m_debuginfo/storage.c +++ b/coregrind/m_debuginfo/storage.c @@ -611,7 +611,7 @@ void ML_(addLineInfo) ( struct _DebugInfo* di, /* Rule out ones which are completely outside the r-x mapped area. See "Comment_Regarding_Text_Range_Checks" elsewhere in this file for background and rationale. */ - vg_assert(di->fsm.have_rx_map && di->fsm.rw_map_count); + vg_assert(di->fsm.have_rx_map); if (ML_(find_rx_mapping)(di, this, this + size - 1) == NULL) { if (0) VG_(message)(Vg_DebugMsg, diff --git a/tests/filter_stderr_basic.in b/tests/filter_stderr_basic.in index db83e9f366..14ca362f27 100755 --- a/tests/filter_stderr_basic.in +++ b/tests/filter_stderr_basic.in @@ -74,3 +74,6 @@ $SED '/warning: the debug information found in "[^"]*" does not match/d' | # Suppress warnings from Dwarf reader $SED '/warning: evaluate_Dwarf3_Expr: unhandled DW_OP_/d' + +# Suppress Darwin dyld errors +$SED '/^used_suppression:.*OSX.*dyld.*default.supp:*/d' -- 2.43.5