* [PATCH] elf-reader: reclaim fd and mem before break
@ 2022-12-20 13:06 Xiaole He
2022-12-21 11:01 ` Dodji Seketeli
0 siblings, 1 reply; 3+ messages in thread
From: Xiaole He @ 2022-12-20 13:06 UTC (permalink / raw)
To: libabigail; +Cc: Xiaole He, Xiaole He
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(-)
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;
}
}
--
2.27.0
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] elf-reader: reclaim fd and mem before break
2022-12-20 13:06 [PATCH] elf-reader: reclaim fd and mem before break Xiaole He
@ 2022-12-21 11:01 ` Dodji Seketeli
2022-12-22 1:09 ` Xiaole He
0 siblings, 1 reply; 3+ messages in thread
From: Dodji Seketeli @ 2022-12-21 11:01 UTC (permalink / raw)
To: Xiaole He via Libabigail; +Cc: Xiaole He, Xiaole He
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re:Re: [PATCH] elf-reader: reclaim fd and mem before break
2022-12-21 11:01 ` Dodji Seketeli
@ 2022-12-22 1:09 ` Xiaole He
0 siblings, 0 replies; 3+ messages in thread
From: Xiaole He @ 2022-12-22 1:09 UTC (permalink / raw)
To: Dodji Seketeli; +Cc: Xiaole He via Libabigail, Xiaole He
[-- 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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-12-22 1:10 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-20 13:06 [PATCH] elf-reader: reclaim fd and mem before break Xiaole He
2022-12-21 11:01 ` Dodji Seketeli
2022-12-22 1:09 ` Xiaole He
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).