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

[Fangrui Song <maskray@google.com>]

On 2022-05-02, Florian Weimer wrote:
>* 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?

VER_NDX_GLOBAL (1) is handled the same way as versions with index >= 2,
in _dl_check_map_versions in dl-version.c. I think l_versions[1] is
typically filled in due to VER_FLG_BASE in DT_VERDEF (.gnu.version_d).

An object may have DT_VERDEF but not DT_VERNEED. In that case, the field
for VER_NDX_GLOBAL are all zeroes.

>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)?

When playing around this yesterday, I have triggered the assert failure
but I don't remember how I did.

       assert (version->filename == NULL
               || ! _dl_name_match_p (version->filename, map));