Re: [PATCH] Fix de-initialization of elf::reader::priv

[Dodji Seketeli <dodji@seketeli.org>]

Hello Petr,

Petr Pavlu via Libabigail <libabigail@sourceware.org> a écrit:

> Add a destructor for elf::reader::priv which releases any allocated alt
> DWARF data and fix the initialize() method to fully reset the object.
>
> The latter fixes a crash observed when handling multiple files. For
> instance, when reading the Linux kernel tree, load_vmlinux_corpus()
> processes vmlinux and all modules. Member dwarf_handle was never reset
> after setting it for the first file which could later result in use of
> invalid DWARF data in dwarf::reader::build_die_parent_maps().
>
> 	* src/abg-elf-reader.cc (priv::~priv): Release alt debug
> 	information.
> 	(priv::initialize): Reset all members.
> 	(priv::clear_alt_dwarf_debug_info_data): New helper function.
>
> Fixes: 7bd69830 ("Make Front Ends first class citizens")

Thanks for the patch!

I have just edited slightly to fix what I think is a related dormant
issue in find_alt_dwarf_debug_info, which is that if the macro
LIBDW_HAS_DWARF_GETALT is not defined, then find_alt_dwarf_debug_info
doesn't return the proper file descriptor for the alternate DWARF debug
info file.  With this additional fix, clear_alt_dwarf_debug_info_data()
will find (and clear) the expected alternate DWARF debuginfo file
descriptor.

The diff of my change is:

    diff --git a/src/abg-elf-reader.cc b/src/abg-elf-reader.cc
    index 4814a70e..c07f0655 100644
    --- a/src/abg-elf-reader.cc
    +++ b/src/abg-elf-reader.cc
    @@ -225,10 +225,10 @@ find_alt_dwarf_debug_info(Dwfl_Module *elf_module,
           // If we reach this point it means we have found the path to the
           // alternate debuginfo file and it's in alt_file_path.  So let's
           // open it and read it.
    -      int fd = open(alt_file_path.c_str(), O_RDONLY);
    -      if (fd == -1)
    +      alt_fd = open(alt_file_path.c_str(), O_RDONLY);
    +      if (alt_fd == -1)
            return result;
    -      result = dwarf_begin(fd, DWARF_C_READ);
    +      result = dwarf_begin(alt_fd, DWARF_C_READ);

     #ifdef LIBDW_HAS_DWARF_GETALT
           Dwarf_Addr bias = 0;

Please find below the patch that I am applying to the master branch.

Thanks!

Cheers,

From 241c7fc7c1fb3b258815c800311f6d83869de87b Mon Sep 17 00:00:00 2001
From: Petr Pavlu <petr.pavlu@suse.com>
Date: Sun, 18 Dec 2022 20:33:40 +0100
Subject: [PATCH] Fix de-initialization of elf::reader::priv

This fixes 7bd69830 ("Make Front Ends first class citizens").

Add a destructor for elf::reader::priv which releases any allocated alt
DWARF data and fix the initialize() method to fully reset the object.

The latter fixes a crash observed when handling multiple files. For
instance, when reading the Linux kernel tree, load_vmlinux_corpus()
processes vmlinux and all modules. Member dwarf_handle was never reset
after setting it for the first file which could later result in use of
invalid DWARF data in dwarf::reader::build_die_parent_maps().

	* src/abg-elf-reader.cc (priv::~priv): Release alt debug
	information.
	(priv::initialize): Reset all members.
	(priv::clear_alt_dwarf_debug_info_data): New helper function.

Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 src/abg-elf-reader.cc | 44 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 39 insertions(+), 5 deletions(-)

diff --git a/src/abg-elf-reader.cc b/src/abg-elf-reader.cc
index 3b1b5803..c07f0655 100644
--- a/src/abg-elf-reader.cc
+++ b/src/abg-elf-reader.cc
@@ -225,10 +225,10 @@ find_alt_dwarf_debug_info(Dwfl_Module *elf_module,
       // If we reach this point it means we have found the path to the
       // alternate debuginfo file and it's in alt_file_path.  So let's
       // open it and read it.
-      int fd = open(alt_file_path.c_str(), O_RDONLY);
-      if (fd == -1)
+      alt_fd = open(alt_file_path.c_str(), O_RDONLY);
+      if (alt_fd == -1)
 	return result;
-      result = dwarf_begin(fd, DWARF_C_READ);
+      result = dwarf_begin(alt_fd, DWARF_C_READ);
 
 #ifdef LIBDW_HAS_DWARF_GETALT
       Dwarf_Addr bias = 0;
@@ -281,6 +281,11 @@ struct reader::priv
     initialize(debug_info_roots);
   }
 
+  ~priv()
+  {
+    clear_alt_dwarf_debug_info_data();
+  }
+
   /// Reset the private data of @elf elf::reader.
   ///
   /// @param debug_info_roots the vector of new directories where to
@@ -288,11 +293,23 @@ struct reader::priv
   void
   initialize(const vector<char**>& debug_info_roots)
   {
-    debug_info_root_paths = debug_info_roots;
+    clear_alt_dwarf_debug_info_data();
+
+    elf_handle = nullptr;
+    symtab_section = nullptr;
+    elf_architecture.clear();
+    dt_needed.clear();
     symt.reset();
+    debug_info_root_paths = debug_info_roots;
+    offline_callbacks = {};
     dwfl_handle.reset();
     elf_module = nullptr;
-    elf_handle = nullptr;
+    dwarf_handle = nullptr;
+    alt_dwarf_handle = nullptr;
+    alt_dwarf_path.clear();
+    alt_dwarf_fd = 0;
+    ctf_section = nullptr;
+    alt_ctf_section = nullptr;
   }
 
   /// Setup the necessary plumbing to open the ELF file and find all
@@ -348,6 +365,23 @@ struct reader::priv
     return result;
   }
 
+  /// Clear the resources related to the alternate DWARF data.
+  void
+  clear_alt_dwarf_debug_info_data()
+  {
+    if (alt_dwarf_fd)
+      {
+        if (alt_dwarf_handle)
+          {
+            dwarf_end(alt_dwarf_handle);
+            alt_dwarf_handle = nullptr;
+          }
+        close(alt_dwarf_fd);
+        alt_dwarf_fd = 0;
+      }
+    alt_dwarf_path.clear();
+  }
+
   /// Locate the DWARF debug info in the ELF file.
   ///
   /// This also knows how to locate split debug info.
-- 
2.31.1


-- 
		Dodji