Re: [PATCH] elf-reader: reclaim fd and mem before break

[Dodji Seketeli <dodji@seketeli.org>]

Hello Xiaole,

Xiaole He via Libabigail <libabigail@sourceware.org> a écrit:

> In 'src/abg-elf-reader.cc':
>
> /* src/abg-elf-reader.cc begin */
> 1  void
> 2  locate_alt_ctf_debug_info()
> 3  {
> 4      ...
> 5      for (const auto& path : rdr.debug_info_root_paths())
> 6      {
> 7          ...
> 8          int fd;
> 9          if ((fd = open(file_path.c_str(), O_RDONLY)) == -1)
> 10             continue;
> 11
> 12         ...
> 13         Elf *hdl;
> 14         if ((hdl = elf_begin(fd, ELF_C_READ, nullptr)) == nullptr)
> 15             ...
> 16
> 17         alt_ctf_section =
> 18             elf_helpers::find_section(hdl, ".ctf", SHT_PROGBITS);
> 19         break;
> 20
> 21         elf_end(hdl);
> 22         close(fd);
> 23     }
> 24     ...
> 25 }
> /* src/abg-elf-reader.cc end */
>
> The file descriptor 'fd' and the memory that 'hdl' pointed to can have
> a chance where they were only created but nerver been destroyed when
> above code reach the line 19. Thus cause the leakage of file descriptor
> and memory.

Good catch.

> This leakage problem had already occured on our system, and the problem
> finally cause process can not open any more file and complaint
> 'Errno 24: Too many open files'.

Of course.  Sorry about that.

>
> This patch fix above problem.
>
>         * src/abg-elf-reader.cc (locate_alt_ctf_debug_info):
>         reclaim fd and mem before break.

Thanks.

>
> Signed-off-by: Xiaole He <hexiaole@kylinos.cn>
> ---
>  src/abg-elf-reader.cc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/abg-elf-reader.cc b/src/abg-elf-reader.cc
> index c07f0655..979f0aae 100644
> --- a/src/abg-elf-reader.cc
> +++ b/src/abg-elf-reader.cc
> @@ -453,10 +453,10 @@ struct reader::priv
>  	  // unlikely .ctf was designed to be present in stripped file
>  	  alt_ctf_section =
>  	    elf_helpers::find_section(hdl, ".ctf", SHT_PROGBITS);
> -          break;
>  
>  	  elf_end(hdl);
>  	  close(fd);
> +          break;

Right, so I have amended the patch somewhat to break out of the loop
only if alt_ctf_section has been found.  Otherwise, the loop keeps going
until all the debug info paths have been explored.

Also, if an alt_ctf_section section is already available,
locate_alt_ctf_debug_info returns early.  The diff of my changes
(compared to your patch) is:

    diff --git a/src/abg-elf-reader.cc b/src/abg-elf-reader.cc
    index 979f0aae..656418e3 100644
    --- a/src/abg-elf-reader.cc
    +++ b/src/abg-elf-reader.cc
    @@ -420,6 +420,9 @@ struct reader::priv
       void
       locate_alt_ctf_debug_info()
       {
    +    if (alt_ctf_section)
    +      return;
    +
         Elf_Scn *section =
           elf_helpers::find_section(elf_handle,
                                    ".gnu_debuglink",
    @@ -456,7 +459,9 @@ struct reader::priv

              elf_end(hdl);
              close(fd);
    -          break;
    +
    +	  if (alt_ctf_section)
    +	    break;
            }
       }
 
The complete patch I am applying to the master branch is the one below.

Many thanks!

Cheers,

From 83bbc679e509047f171fa4db9faa0d05cd26a258 Mon Sep 17 00:00:00 2001
From: Xiaole He <hexiaole@kylinos.cn>
Date: Tue, 20 Dec 2022 21:06:34 +0800
Subject: [PATCH] elf-reader: reclaim fd and mem before break

In elf::reader::priv::locate_alt_ctf_debug_info from
src/abg-elf-reader.cc, the resources held by the hdl and fd variables
aren't necessary released because the control-flow gets out of the
loop too early.  This patch fixes the problem.

        * src/abg-elf-reader.cc
	(elf::reader::priv::locate_alt_ctf_debug_info): Reclaim fd and mem
	before break.  Also, do not try to locate the debug info it's
	already been located.

Signed-off-by: Xiaole He <hexiaole@kylinos.cn>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 src/abg-elf-reader.cc | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/abg-elf-reader.cc b/src/abg-elf-reader.cc
index c07f0655..656418e3 100644
--- a/src/abg-elf-reader.cc
+++ b/src/abg-elf-reader.cc
@@ -420,6 +420,9 @@ struct reader::priv
   void
   locate_alt_ctf_debug_info()
   {
+    if (alt_ctf_section)
+      return;
+
     Elf_Scn *section =
       elf_helpers::find_section(elf_handle,
 				".gnu_debuglink",
@@ -453,10 +456,12 @@ struct reader::priv
 	  // unlikely .ctf was designed to be present in stripped file
 	  alt_ctf_section =
 	    elf_helpers::find_section(hdl, ".ctf", SHT_PROGBITS);
-          break;
 
 	  elf_end(hdl);
 	  close(fd);
+
+	  if (alt_ctf_section)
+	    break;
 	}
   }
 
-- 
2.31.1


-- 
		Dodji