[V2] ctf: make libabigail::ctf_reader::read_corpus reentrant

  [Changes from V1:
- Make sure that the string table passed to libctf is the one related
  to the symbol table.]

The libctf call ctf_open is not reentrant.  This is because it uses
bfd_open (and other BFD calls) internally in order to fetch the
different bits of CTF from the ELF file.

This is unfortunate, as it makes libabigail::ctf_reader::read_corpus
non-reentrant.  We detected this problem thanks to one of the
libabigail test driver, that exercises tests in parallel using
threads.

Fortunately libctf provides an alternate way to decode CTF data, that
involves the user to provide the raw contents of the relevant ELF
sections (.ctf, the symtab, the string table) to ctf_arc_bufopen
call.

This patch changes the CTF reader in libabigail to use this
mechanism.  libelf is used in order to extract the contents of these
sections.

	* src/abg-ctf-reader.cc (class read_context): New attributes
	elf_handler, elf_fd, ctf_sect, symtab_sec and strtab_sect.
	(read_context): Do not read the CTF archive here.
	(slurp_elf_info): Adjust to use attributes instead of locals, and
	fetch the raw ELF section contents for libctf.
	(close_elf_handler): New function.
	(fill_ctf_section): Likewise.
	(read_corpus): Call open_elf_handler, close_elf_handler and build
	the CTF archive using ctf_arc_bufopen instead of ctf_open.

Signed-by: Jose E. Marchesi <jose.marchesi@oracle.com>
---
 src/abg-ctf-reader.cc | 143 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 112 insertions(+), 31 deletions(-)
  

ping

> [Changes from V1:
> - Make sure that the string table passed to libctf is the one related
>   to the symbol table.]
>
> The libctf call ctf_open is not reentrant.  This is because it uses
> bfd_open (and other BFD calls) internally in order to fetch the
> different bits of CTF from the ELF file.
>
> This is unfortunate, as it makes libabigail::ctf_reader::read_corpus
> non-reentrant.  We detected this problem thanks to one of the
> libabigail test driver, that exercises tests in parallel using
> threads.
>
> Fortunately libctf provides an alternate way to decode CTF data, that
> involves the user to provide the raw contents of the relevant ELF
> sections (.ctf, the symtab, the string table) to ctf_arc_bufopen
> call.
>
> This patch changes the CTF reader in libabigail to use this
> mechanism.  libelf is used in order to extract the contents of these
> sections.
>
> 	* src/abg-ctf-reader.cc (class read_context): New attributes
> 	elf_handler, elf_fd, ctf_sect, symtab_sec and strtab_sect.
> 	(read_context): Do not read the CTF archive here.
> 	(slurp_elf_info): Adjust to use attributes instead of locals, and
> 	fetch the raw ELF section contents for libctf.
> 	(close_elf_handler): New function.
> 	(fill_ctf_section): Likewise.
> 	(read_corpus): Call open_elf_handler, close_elf_handler and build
> 	the CTF archive using ctf_arc_bufopen instead of ctf_open.
>
> Signed-by: Jose E. Marchesi <jose.marchesi@oracle.com>
> ---
>  src/abg-ctf-reader.cc | 143 +++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 112 insertions(+), 31 deletions(-)
>
> diff --git a/src/abg-ctf-reader.cc b/src/abg-ctf-reader.cc
> index 1c41ea02..d107fac8 100644
> --- a/src/abg-ctf-reader.cc
> +++ b/src/abg-ctf-reader.cc
> @@ -58,9 +58,20 @@ public:
>    /// is used to reuse already generated types.
>    unordered_map<ctf_id_t,type_base_sptr> types_map;
>  
> +  /// libelf handler for the ELF file from which we read the CTF data,
> +  /// and the corresponding file descriptor.
> +  Elf *elf_handler;
> +  int elf_fd;
> +
>    /// The symtab read from the ELF file.
>    symtab_reader::symtab_sptr symtab;
>  
> +  /// Raw contents of several sections from the ELF file.  These are
> +  /// used by libctf.
> +  ctf_sect_t ctf_sect;
> +  ctf_sect_t symtab_sect;
> +  ctf_sect_t strtab_sect;
> +
>    /// Associate a given CTF type ID with a given libabigail IR type.
>    void add_type(ctf_id_t ctf_type, type_base_sptr type)
>    {
> @@ -93,15 +104,12 @@ public:
>    /// @param elf_path the path to the ELF file.
>    read_context(string elf_path, ir::environment *env)
>    {
> -    int err;
> -
>      types_map.clear();
>      filename = elf_path;
>      ir_env = env;
> -    ctfa = ctf_open(filename.c_str(), NULL /* BFD target */, &err);
> -
> -    if (ctfa == NULL)
> -      fprintf(stderr, "cannot open %s: %s\n", filename.c_str(), ctf_errmsg(err));
> +    elf_handler = NULL;
> +    elf_fd = -1;
> +    ctfa = NULL;
>    }
>  
>    /// Destructor of the @ref read_context type.
> @@ -939,50 +947,115 @@ process_ctf_archive(read_context *ctxt, corpus_sptr corp)
>  
>  }
>  
> -/// Slurp certain information from the ELF file described by a given
> -/// read context and install it in a libabigail corpus.
> -///
> -/// @param ctxt the read context
> -/// @param corp the libabigail corpus in which to install the info.
> +/// Open the ELF file described by the given read context.
>  ///
> -/// @return 0 if there is an error.
> +/// @param ctxt the read context.
> +/// @return 0 if the ELF file can't be opened.
>  /// @return 1 otherwise.
>  
>  static int
> -slurp_elf_info(read_context *ctxt, corpus_sptr corp)
> +open_elf_handler (read_context *ctxt)
>  {
>    /* libelf requires to negotiate/set the version of ELF.  */
>    if (elf_version(EV_CURRENT) == EV_NONE)
>      return 0;
>  
>    /* Open an ELF handler.  */
> -  int elf_fd = open(ctxt->filename.c_str(), O_RDONLY);
> -  if (elf_fd == -1)
> +  ctxt->elf_fd = open(ctxt->filename.c_str(), O_RDONLY);
> +  if (ctxt->elf_fd == -1)
>      return 0;
>  
> -  Elf *elf_handler = elf_begin(elf_fd, ELF_C_READ, NULL);
> -  if (elf_handler == NULL)
> +  ctxt->elf_handler = elf_begin(ctxt->elf_fd, ELF_C_READ, NULL);
> +  if (ctxt->elf_handler == NULL)
>      {
>        fprintf(stderr, "cannot open %s: %s\n",
>                 ctxt->filename.c_str(), elf_errmsg(elf_errno()));
> -      close(elf_fd);
> +      close(ctxt->elf_fd);
>        return 0;
>      }
>  
> +  return 1;
> +}
> +
> +/// Close the ELF file described by the given read context.
> +///
> +/// @param ctxt the read context.
> +
> +static void
> +close_elf_handler (read_context *ctxt)
> +{
> +  /* Finish the ELF handler and close the associated file.  */
> +  elf_end(ctxt->elf_handler);
> +  close(ctxt->elf_fd);
> +}
> +
> +/// Fill a CTF section description with the information in a given ELF
> +/// section.
> +///
> +/// @param ctxt the read context.
> +/// @param elf_section the ELF section from which to get.
> +/// @param ctf_section the CTF section to fill with the raw data.
> +
> +static void
> +fill_ctf_section(read_context *ctxt, Elf_Scn *elf_section, ctf_sect_t *ctf_section)
> +{
> +  GElf_Shdr section_header_mem, *section_header;
> +  Elf_Data *section_data;
> +
> +  section_header = gelf_getshdr(elf_section, &section_header_mem);
> +  section_data = elf_getdata(elf_section, 0);
> +
> +  ABG_ASSERT (section_header != NULL);
> +  ABG_ASSERT (section_data != NULL);
> +
> +  ctf_section->cts_name = ""; /* This is not actually used by libctf.  */
> +  ctf_section->cts_data = (char *) section_data->d_buf;
> +  ctf_section->cts_size = section_data->d_size;
> +  ctf_section->cts_entsize = section_header->sh_entsize;
> +}
> +
> +/// Slurp certain information from the ELF file described by a given
> +/// read context and install it in a libabigail corpus.
> +///
> +/// @param ctxt the read context
> +/// @param corp the libabigail corpus in which to install the info.
> +///
> +/// @return 0 if there is an error.
> +/// @return 1 otherwise.
> +
> +static int
> +slurp_elf_info(read_context *ctxt, corpus_sptr corp)
> +{
>    /* Set the ELF architecture.  */
>    GElf_Ehdr eh_mem;
> -  GElf_Ehdr *ehdr = gelf_getehdr(elf_handler, &eh_mem);
> +  GElf_Ehdr *ehdr = gelf_getehdr(ctxt->elf_handler, &eh_mem);
>    corp->set_architecture_name(elf_helpers::e_machine_to_string(ehdr->e_machine));
>  
>    /* Read the symtab from the ELF file and set it in the corpus.  */
>    ctxt->symtab =
> -    symtab_reader::symtab::load(elf_handler, ctxt->ir_env,
> +    symtab_reader::symtab::load(ctxt->elf_handler, ctxt->ir_env,
>                                  0 /* No suppressions.  */);
>    corp->set_symtab(ctxt->symtab);
>  
> -  /* Finish the ELF handler and close the associated file.  */
> -  elf_end(elf_handler);
> -  close(elf_fd);
> +  /* Get the raw ELF section contents for libctf.  The .ctf section
> +     and the symtab are straightforward enough.  */
> +  Elf_Scn *ctf_scn = elf_helpers::find_section(ctxt->elf_handler, ".ctf", SHT_PROGBITS);
> +  Elf_Scn *symtab_scn = elf_helpers::find_symbol_table_section(ctxt->elf_handler);
> +
> +  if (ctf_scn == NULL || symtab_scn == NULL)
> +    return 0;
> +
> +  /* The string table that libctf expects is the one related to the
> +     symbol table by virtue of sh_link.  */
> +  GElf_Shdr symtab_shdr_mem, *symtab_shdr = gelf_getshdr(symtab_scn, &symtab_shdr_mem);
> +  Elf_Scn *strtab_scn = elf_getscn(ctxt->elf_handler, symtab_shdr->sh_link);
> +
> +  if (strtab_scn == NULL)
> +    return 0;
> +
> +  fill_ctf_section(ctxt, ctf_scn, &ctxt->ctf_sect);
> +  fill_ctf_section(ctxt, symtab_scn, &ctxt->symtab_sect);
> +  fill_ctf_section(ctxt, strtab_scn, &ctxt->strtab_sect);
>  
>    return 1;
>  }
> @@ -1012,19 +1085,27 @@ read_corpus(read_context *ctxt)
>    corpus_sptr corp
>      = std::make_shared<corpus>(ctxt->ir_env, ctxt->filename);
>  
> +  /* Open the ELF file.  */
> +  if (!open_elf_handler(ctxt))
> +    return corp;
> +
>    /* Set some properties of the corpus first.  */
>    corp->set_origin(corpus::CTF_ORIGIN);
>    if (!slurp_elf_info(ctxt, corp))
>      return corp;
>  
> -  /* Get out now if no CTF debug info is found.  */
> -  if (ctxt->ctfa == NULL)
> -    return corp;
> -
> -  /* Process the CTF archive in the read context, if any.  Information
> -     about the types, variables, functions, etc contained in the
> -     archive are added to the given corpus.  */
> -  process_ctf_archive(ctxt, corp);
> +  /* Build the cfta from the contents of the relevant ELF sections,
> +     and process the CTF archive in the read context, if any.
> +     Information about the types, variables, functions, etc contained
> +     in the archive are added to the given corpus.  */
> +  int errp;
> +  ctxt->ctfa = ctf_arc_bufopen(&ctxt->ctf_sect, &ctxt->symtab_sect,
> +                               &ctxt->strtab_sect, &errp);
> +  if (ctxt->ctfa != NULL)
> +    process_ctf_archive(ctxt, corp);
> +
> +  /* Cleanup and return.  */
> +  close_elf_handler(ctxt);
>    return corp;
>  }
  

"Jose E. Marchesi via Libabigail" <libabigail@sourceware.org> a écrit:

> [Changes from V1:
> - Make sure that the string table passed to libctf is the one related
>   to the symbol table.]


Pffft.

I mistakenly applied the V1 patch to master, sorry about that.

So I am providing below a "differential" patch that would bring master
to the level of this V2 patch.

Would you mind reviewing it, please, (and sign it off) so that I can
apply it master as well?

Thanks, and sorry again.

Cheers,

From b3e9ff53879939a3e0d4092ecd23cbd39393c268 Mon Sep 17 00:00:00 2001
From: Dodji Seketeli <dodji@redhat.com>
Date: Tue, 9 Nov 2021 11:24:16 +0100
Subject: [PATCH] ctf-reader: Use the string table related to the symtab

There can be several string tables in the ELF file.  The one we want
to use is the one related to the symbol table, referred to by its
sh_link property.

This patch ensures that.

	* src/abg-ctf-reader.cc (slurp_elf_info): Use the string table
	related to the symbol table.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 src/abg-ctf-reader.cc | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/abg-ctf-reader.cc b/src/abg-ctf-reader.cc
index 51fb2ed9..33642821 100644
--- a/src/abg-ctf-reader.cc
+++ b/src/abg-ctf-reader.cc
@@ -1037,12 +1037,20 @@ slurp_elf_info(read_context *ctxt, corpus_sptr corp)
                                 0 /* No suppressions.  */);
   corp->set_symtab(ctxt->symtab);
 
-  /* Get the raw ELF section contents for libctf.  */
+  /* Get the raw ELF section contents for libctf.  The .ctf section
+     and the symtab are straightforward enough.  */
   Elf_Scn *ctf_scn = elf_helpers::find_section(ctxt->elf_handler, ".ctf", SHT_PROGBITS);
   Elf_Scn *symtab_scn = elf_helpers::find_symbol_table_section(ctxt->elf_handler);
-  Elf_Scn *strtab_scn = elf_helpers::find_section(ctxt->elf_handler, SHT_STRTAB);
 
-  if (ctf_scn == NULL || symtab_scn == NULL || strtab_scn == NULL)
+  if (ctf_scn == NULL || symtab_scn == NULL)
+    return 0;
+
+  /* The string table that libctf expects is the one related to the
+     symbol table by virtue of sh_link.  */
+  GElf_Shdr symtab_shdr_mem, *symtab_shdr = gelf_getshdr(symtab_scn, &symtab_shdr_mem);
+  Elf_Scn *strtab_scn = elf_getscn(ctxt->elf_handler, symtab_shdr->sh_link);
+
+  if (strtab_scn == NULL)
     return 0;
 
   fill_ctf_section(ctxt, ctf_scn, &ctxt->ctf_sect);
  

> "Jose E. Marchesi via Libabigail" <libabigail@sourceware.org> a écrit:
>
>> [Changes from V1:
>> - Make sure that the string table passed to libctf is the one related
>>   to the symbol table.]
>
>
> Pffft.
>
> I mistakenly applied the V1 patch to master, sorry about that.

Nah it is my fault.  I should have sent the right version straight away
:)

> So I am providing below a "differential" patch that would bring master
> to the level of this V2 patch.
>
> Would you mind reviewing it, please, (and sign it off) so that I can
> apply it master as well?

LGTM.

>
> Thanks, and sorry again.
>
> Cheers,
>
> From b3e9ff53879939a3e0d4092ecd23cbd39393c268 Mon Sep 17 00:00:00 2001
> From: Dodji Seketeli <dodji@redhat.com>
> Date: Tue, 9 Nov 2021 11:24:16 +0100
> Subject: [PATCH] ctf-reader: Use the string table related to the symtab
>
> There can be several string tables in the ELF file.  The one we want
> to use is the one related to the symbol table, referred to by its
> sh_link property.
>
> This patch ensures that.
>
> 	* src/abg-ctf-reader.cc (slurp_elf_info): Use the string table
> 	related to the symbol table.
>
> Signed-off-by: Dodji Seketeli <dodji@redhat.com>

Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com>

> ---
>  src/abg-ctf-reader.cc | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/src/abg-ctf-reader.cc b/src/abg-ctf-reader.cc
> index 51fb2ed9..33642821 100644
> --- a/src/abg-ctf-reader.cc
> +++ b/src/abg-ctf-reader.cc
> @@ -1037,12 +1037,20 @@ slurp_elf_info(read_context *ctxt, corpus_sptr corp)
>                                  0 /* No suppressions.  */);
>    corp->set_symtab(ctxt->symtab);
>  
> -  /* Get the raw ELF section contents for libctf.  */
> +  /* Get the raw ELF section contents for libctf.  The .ctf section
> +     and the symtab are straightforward enough.  */
>    Elf_Scn *ctf_scn = elf_helpers::find_section(ctxt->elf_handler, ".ctf", SHT_PROGBITS);
>    Elf_Scn *symtab_scn = elf_helpers::find_symbol_table_section(ctxt->elf_handler);
> -  Elf_Scn *strtab_scn = elf_helpers::find_section(ctxt->elf_handler, SHT_STRTAB);
>  
> -  if (ctf_scn == NULL || symtab_scn == NULL || strtab_scn == NULL)
> +  if (ctf_scn == NULL || symtab_scn == NULL)
> +    return 0;
> +
> +  /* The string table that libctf expects is the one related to the
> +     symbol table by virtue of sh_link.  */
> +  GElf_Shdr symtab_shdr_mem, *symtab_shdr = gelf_getshdr(symtab_scn, &symtab_shdr_mem);
> +  Elf_Scn *strtab_scn = elf_getscn(ctxt->elf_handler, symtab_shdr->sh_link);
> +
> +  if (strtab_scn == NULL)
>      return 0;
>  
>    fill_ctf_section(ctxt, ctf_scn, &ctxt->ctf_sect);
> -- 
>
> 2.32.0
  

Actually... I think it would be better to put the logic below in a
function in elf_helpers instead.  Let me chant a patch quickly...

>> "Jose E. Marchesi via Libabigail" <libabigail@sourceware.org> a écrit:
>>
>>> [Changes from V1:
>>> - Make sure that the string table passed to libctf is the one related
>>>   to the symbol table.]
>>
>>
>> Pffft.
>>
>> I mistakenly applied the V1 patch to master, sorry about that.
>
> Nah it is my fault.  I should have sent the right version straight away
> :)
>
>> So I am providing below a "differential" patch that would bring master
>> to the level of this V2 patch.
>>
>> Would you mind reviewing it, please, (and sign it off) so that I can
>> apply it master as well?
>
> LGTM.
>
>>
>> Thanks, and sorry again.
>>
>> Cheers,
>>
>> From b3e9ff53879939a3e0d4092ecd23cbd39393c268 Mon Sep 17 00:00:00 2001
>> From: Dodji Seketeli <dodji@redhat.com>
>> Date: Tue, 9 Nov 2021 11:24:16 +0100
>> Subject: [PATCH] ctf-reader: Use the string table related to the symtab
>>
>> There can be several string tables in the ELF file.  The one we want
>> to use is the one related to the symbol table, referred to by its
>> sh_link property.
>>
>> This patch ensures that.
>>
>> 	* src/abg-ctf-reader.cc (slurp_elf_info): Use the string table
>> 	related to the symbol table.
>>
>> Signed-off-by: Dodji Seketeli <dodji@redhat.com>
>
> Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com>
>
>> ---
>>  src/abg-ctf-reader.cc | 14 +++++++++++---
>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/abg-ctf-reader.cc b/src/abg-ctf-reader.cc
>> index 51fb2ed9..33642821 100644
>> --- a/src/abg-ctf-reader.cc
>> +++ b/src/abg-ctf-reader.cc
>> @@ -1037,12 +1037,20 @@ slurp_elf_info(read_context *ctxt, corpus_sptr corp)
>>                                  0 /* No suppressions.  */);
>>    corp->set_symtab(ctxt->symtab);
>>  
>> -  /* Get the raw ELF section contents for libctf.  */
>> +  /* Get the raw ELF section contents for libctf.  The .ctf section
>> +     and the symtab are straightforward enough.  */
>>    Elf_Scn *ctf_scn = elf_helpers::find_section(ctxt->elf_handler, ".ctf", SHT_PROGBITS);
>>    Elf_Scn *symtab_scn = elf_helpers::find_symbol_table_section(ctxt->elf_handler);
>> -  Elf_Scn *strtab_scn = elf_helpers::find_section(ctxt->elf_handler, SHT_STRTAB);
>>  
>> -  if (ctf_scn == NULL || symtab_scn == NULL || strtab_scn == NULL)
>> +  if (ctf_scn == NULL || symtab_scn == NULL)
>> +    return 0;
>> +
>> +  /* The string table that libctf expects is the one related to the
>> +     symbol table by virtue of sh_link.  */
>> +  GElf_Shdr symtab_shdr_mem, *symtab_shdr = gelf_getshdr(symtab_scn, &symtab_shdr_mem);
>> +  Elf_Scn *strtab_scn = elf_getscn(ctxt->elf_handler, symtab_shdr->sh_link);
>> +
>> +  if (strtab_scn == NULL)
>>      return 0;
>>  
>>    fill_ctf_section(ctxt, ctf_scn, &ctxt->ctf_sect);
>> -- 
>>
>> 2.32.0