diff mbox series

elf-reader: reclaim fd and mem before break

Message ID	20221220130634.9693-1-hexiaole1994@126.com
State	New
Headers	DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org EEEE03858421 To: libabigail@sourceware.org Cc: Xiaole He <hexiaole1994@126.com>, Xiaole He <hexiaole@kylinos.cn> Subject: [PATCH] elf-reader: reclaim fd and mem before break Date: Tue, 20 Dec 2022 21:06:34 +0800 Message-Id: <20221220130634.9693-1-hexiaole1994@126.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: list From: Xiaole He via Libabigail <libabigail@sourceware.org> Reply-To: Xiaole He <hexiaole1994@126.com> Errors-To: libabigail-bounces+patchwork=sourceware.org@sourceware.org Sender: "Libabigail" <libabigail-bounces+patchwork=sourceware.org@sourceware.org>
Series	elf-reader: reclaim fd and mem before break \| elf-reader: reclaim fd and mem before break

Commit Message

Xiaole He Dec. 20, 2022, 1:06 p.m. UTC

  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.

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'.

This patch fix above problem.

        * src/abg-elf-reader.cc (locate_alt_ctf_debug_info):
        reclaim fd and mem before break.

Signed-off-by: Xiaole He <hexiaole@kylinos.cn>
---
 src/abg-elf-reader.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Dodji Seketeli Dec. 21, 2022, 11:01 a.m. UTC | #1

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;
 	}
   }

Xiaole He Dec. 22, 2022, 1:09 a.m. UTC | #2

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

diff mbox series

Patch

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;
 	}
   }