* [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).