[1/2] symtab-reader: Remove an over-agressive assertion

  In symtab::load, the symtab reader walks the symbol table and records
each relation "symbol <-> address".
So, the relation "foo <-> address-of-foo" is going to be recorded.
The relation "foo.cfi <-> address-of-foo.cfi" is going to be recorded
as well.

But then, because the symbol foo.cfi has a special meaning, in the
realm of "control flow integrity", the relation "foo.cfi <->
address-of-foo.cfi" (as well as all the *.cfi <-> address-of*.cfi
relations) is going to be recorded (again but) in a particular way by
calling symtab::add_alternative_address_lookups.

The problem is that in, symtab::add_alternative_address_lookups there
is an assert that (wrongly) assumes that the relation foo.cfi <->
address-of-foo.cfi is being seen for the first time.  This is wrong
because the loop in symtab::load that records all the "symbol <->
address" relations has seen and recorded this foo.cfi <->
address-of-foo.cfi relation once already.

This patch removes that assert so that the kernel referred to in the bug
report of PR26646, as mentioned in
https://sourceware.org/bugzilla/show_bug.cgi?id=26646#c5, can be
processed by abidw without crashing.

	* src/abg-symtab-reader.cc
	(symtab::add_alternative_address_lookups): Remove over-aggressive
	assert.

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

Hi.

On Mon, 7 Feb 2022 at 11:10, Dodji Seketeli <dodji@redhat.com> wrote:
>
>
> In symtab::load, the symtab reader walks the symbol table and records
> each relation "symbol <-> address".
> So, the relation "foo <-> address-of-foo" is going to be recorded.
> The relation "foo.cfi <-> address-of-foo.cfi" is going to be recorded
> as well.
>
> But then, because the symbol foo.cfi has a special meaning, in the
> realm of "control flow integrity", the relation "foo.cfi <->
> address-of-foo.cfi" (as well as all the *.cfi <-> address-of*.cfi
> relations) is going to be recorded (again but) in a particular way by
> calling symtab::add_alternative_address_lookups.
>
> The problem is that in, symtab::add_alternative_address_lookups there
> is an assert that (wrongly) assumes that the relation foo.cfi <->
> address-of-foo.cfi is being seen for the first time.  This is wrong
> because the loop in symtab::load that records all the "symbol <->
> address" relations has seen and recorded this foo.cfi <->
> address-of-foo.cfi relation once already.
>
> This patch removes that assert so that the kernel referred to in the bug
> report of PR26646, as mentioned in
> https://sourceware.org/bugzilla/show_bug.cgi?id=26646#c5, can be
> processed by abidw without crashing.
>
>         * src/abg-symtab-reader.cc
>         (symtab::add_alternative_address_lookups): Remove over-aggressive
>         assert.
>
> Signed-off-by: Dodji Seketeli <dodji@redhat.com>

Reviewed-by: Giuliano Procida <gprocida@google.com>

I didn't see a better solution and you've written up a proper analysis
in the commit message. Thanks!

Giuliano.

> ---
>  src/abg-symtab-reader.cc | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/src/abg-symtab-reader.cc b/src/abg-symtab-reader.cc
> index 78dec36d..b42ce87d 100644
> --- a/src/abg-symtab-reader.cc
> +++ b/src/abg-symtab-reader.cc
> @@ -651,9 +651,7 @@ symtab::add_alternative_address_lookups(Elf* elf_handle)
>                                                              symbol_sptr);
>                 }
>
> -             const auto result =
> -                 addr_symbol_map_.emplace(symbol_value, symbol_sptr);
> -             ABG_ASSERT(result.second);
> +             addr_symbol_map_.emplace(symbol_value, symbol_sptr);
>             }
>         }
>      }
> --
> 2.35.0.rc2
>
>
>
> --
>                 Dodji
>
  

Giuliano Procida via Libabigail <libabigail@sourceware.org> a écrit:

> Hi.
>
> On Mon, 7 Feb 2022 at 11:10, Dodji Seketeli <dodji@redhat.com> wrote:
>>
>>
>> In symtab::load, the symtab reader walks the symbol table and records
>> each relation "symbol <-> address".
>> So, the relation "foo <-> address-of-foo" is going to be recorded.
>> The relation "foo.cfi <-> address-of-foo.cfi" is going to be recorded
>> as well.
>>
>> But then, because the symbol foo.cfi has a special meaning, in the
>> realm of "control flow integrity", the relation "foo.cfi <->
>> address-of-foo.cfi" (as well as all the *.cfi <-> address-of*.cfi
>> relations) is going to be recorded (again but) in a particular way by
>> calling symtab::add_alternative_address_lookups.
>>
>> The problem is that in, symtab::add_alternative_address_lookups there
>> is an assert that (wrongly) assumes that the relation foo.cfi <->
>> address-of-foo.cfi is being seen for the first time.  This is wrong
>> because the loop in symtab::load that records all the "symbol <->
>> address" relations has seen and recorded this foo.cfi <->
>> address-of-foo.cfi relation once already.
>>
>> This patch removes that assert so that the kernel referred to in the bug
>> report of PR26646, as mentioned in
>> https://sourceware.org/bugzilla/show_bug.cgi?id=26646#c5, can be
>> processed by abidw without crashing.
>>
>>         * src/abg-symtab-reader.cc
>>         (symtab::add_alternative_address_lookups): Remove over-aggressive
>>         assert.
>>
>> Signed-off-by: Dodji Seketeli <dodji@redhat.com>
>
> Reviewed-by: Giuliano Procida <gprocida@google.com>
>
> I didn't see a better solution and you've written up a proper analysis
> in the commit message. Thanks!

Thanks!

Applied to master.

[...]

Cheers,