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

["Xiaole He" <hexiaole1994@126.com>]

[-- Attachment #1: Type: text/plain, Size: 4887 bytes --]

Copy that, thank you for reviewing and amending, looks great!

















At 2022-12-21 19:01:44, "Dodji Seketeli" <dodji@seketeli.org> wrote:
>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