linux, client: set module name from DT_SONAME

3e56ef9d changed dump_syms to set the module name from DT_SONAME
expecting that clients were already using DT_SONAME when it was
present. The Breakpad client previously only used DT_SONAME as the name
for a module if it detected that it was likely mapped from a zip file.
This patch updates the Breakpad Linux client to always use the
DT_SONAME in minidumps if it's present.

Also included are changes to address comments that were missed from
that review.

Bug: 1016924
Change-Id: I4aae8c05e6793d4b0598049a8964ddd4cb0c6194
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/1889231
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

2 files changed, 32 insertions, 24 deletions

diff --git a/src/client/linux/minidump_writer/linux_dumper.cc b/src/client/linux/minidump_writer/linux_dumper.cc
index 5653133e..1112035b 100644
--- a/src/client/linux/minidump_writer/linux_dumper.cc
+++ b/src/client/linux/minidump_writer/linux_dumper.cc
@@ -473,19 +473,26 @@ void LinuxDumper::GetMappingEffectiveNameAndPath(const MappingInfo& mapping,
                                                  size_t file_name_size) {
   my_strlcpy(file_path, mapping.name, file_path_size);
 
-  // If an executable is mapped from a non-zero offset, this is likely because
-  // the executable was loaded directly from inside an archive file (e.g., an
-  // apk on Android). We try to find the name of the shared object (SONAME) by
-  // looking in the file for ELF sections.
-  bool mapped_from_archive = false;
-  if (mapping.exec && mapping.offset != 0) {
-    mapped_from_archive =
-        ElfFileSoName(*this, mapping, file_name, file_name_size);
+  // Tools such as minidump_stackwalk use the name of the module to look up
+  // symbols produced by dump_syms. dump_syms will prefer to use a module's
+  // DT_SONAME as the module name, if one exists, and will fall back to the
+  // filesystem name of the module.
+
+  // Just use the filesystem name if no SONAME is present.
+  if (!ElfFileSoName(*this, mapping, file_name, file_name_size)) {
+    //   file_path := /path/to/libname.so
+    //   file_name := libname.so
+    const char* basename = my_strrchr(file_path, '/');
+    basename = basename == NULL ? file_path : (basename + 1);
+    my_strlcpy(file_name, basename, file_name_size);
+    return;
   }
 
-  if (mapped_from_archive) {
-    // Some tools (e.g., stackwalk) extract the basename from the pathname. In
-    // this case, we append the file_name to the mapped archive path as follows:
+  if (mapping.exec && mapping.offset != 0) {
+    // If an executable is mapped from a non-zero offset, this is likely because
+    // the executable was loaded directly from inside an archive file (e.g., an
+    // apk on Android).
+    // In this case, we append the file_name to the mapped archive path:
     //   file_name := libname.so
     //   file_path := /path/to/ARCHIVE.APK/libname.so
     if (my_strlen(file_path) + 1 + my_strlen(file_name) < file_path_size) {
@@ -493,12 +500,15 @@ void LinuxDumper::GetMappingEffectiveNameAndPath(const MappingInfo& mapping,
       my_strlcat(file_path, file_name, file_path_size);
     }
   } else {
-    // Common case:
-    //   file_path := /path/to/libname.so
-    //   file_name := libname.so
-    const char* basename = my_strrchr(file_path, '/');
-    basename = basename == NULL ? file_path : (basename + 1);
-    my_strlcpy(file_name, basename, file_name_size);
+    // Otherwise, replace the basename with the SONAME.
+    char* basename = const_cast<char*>(my_strrchr(file_path, '/'));
+    if (basename) {
+      my_strlcpy(basename + 1, file_name,
+                 file_path_size - my_strlen(file_path) +
+                     my_strlen(basename + 1));
+    } else {
+      my_strlcpy(file_path, file_name, file_path_size);
+    }
   }
 }
 
diff --git a/src/common/linux/dump_symbols.cc b/src/common/linux/dump_symbols.cc
index e561ad94..0eea2b54 100644
--- a/src/common/linux/dump_symbols.cc
+++ b/src/common/linux/dump_symbols.cc
@@ -963,13 +963,11 @@ bool InitModuleForElfClass(const typename ElfClass::Ehdr* elf_header,
     return false;
   }
 
-  string name;
-  char name_buf[NAME_MAX];
-  memset(name_buf, 0, sizeof(name_buf));
-  name = google_breakpad::ElfFileSoNameFromMappedFile(elf_header, name_buf,
-                                                      sizeof(name_buf))
-             ? name_buf
-             : google_breakpad::BaseName(obj_filename);
+  char name_buf[NAME_MAX] = {};
+  std::string name = google_breakpad::ElfFileSoNameFromMappedFile(
+                         elf_header, name_buf, sizeof(name_buf))
+                         ? name_buf
+                         : google_breakpad::BaseName(obj_filename);
 
   // Add an extra "0" at the end.  PDB files on Windows have an 'age'
   // number appended to the end of the file identifier; this isn't