Re: [PATCH] elf: Simplify version test when searching a versioned symbol

[Florian Weimer <fweimer@redhat.com>]

* Fangrui Song:

> On 2022-05-01, Fangrui Song wrote:
>>---
>> elf/dl-lookup.c | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>
>>diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c
>>index 989b073e4f..3ad6c95d79 100644
>>--- a/elf/dl-lookup.c
>>+++ b/elf/dl-lookup.c
>>@@ -110,14 +110,14 @@ check_match (const char *const undef_name,
>> 	}
>>       else
>> 	{
>>-	  /* We can match the version information or use the
>>-	     default one if it is not hidden.  */
>>-	  ElfW(Half) ndx = verstab[symidx] & 0x7fff;
>>+	  /* When the version does not match, allow VER_NDX_GLOBAL fallback when
>>+	     resolving relocations (version->hidden==0).  Don't bother with the
>>+	     check done by the linker: VER_NDX_GLOBAL symbol cannot be hidden.
>>+	     */
>>+	  ElfW (Half) ndx = verstab[symidx] & 0x7fff;
>> 	  if ((map->l_versions[ndx].hash != version->hash
>> 	       || strcmp (map->l_versions[ndx].name, version->name))
>>-	      && (version->hidden || map->l_versions[ndx].hash
>>-		  || (verstab[symidx] & 0x8000)))
>>-	    /* It's not the version we want.  */
>>+	      && (version->hidden || ndx != VER_NDX_GLOBAL))
>> 	    return NULL;
>> 	}
>>     }
>> -- 2.36.0.464.gb9c8b46e94-goog
>
> The existing code has a bug.
>
> If a has foo@v1 referencing b.so. If I rebuild b.so and change foo@v1 to
> foo VER_NDX_GLOBAL,
>
> `strcmp (map->l_versions[ndx].name, version->name)` may trigger a null
> pointer dereference:
>
>   (rr) p map->l_versions[1]
>   $7 = {name = 0x0, hash = 0, hidden = 0, filename = 0x0}
>
> This can be fixed with `!map->l_versions[ndx].name || strcmp (map->l_versions[ndx].name, version->name)`

Hmm.  How do we handle VER_NDX_GLOBAL in the dynamic linker?

The if branch has an assert.  I think it fires if we drop symbol
versioning completely.  I think we should report a proper error for
that.

Should the switch to VER_NDX_GLOBAL also result in an error
(eventually)?

Thanks,
Florian