elf: Simplify version test when searching a versioned symbol
Checks
Context |
Check |
Description |
dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
dj/TryBot-32bit |
success
|
Build for i686
|
Commit Message
---
elf/dl-lookup.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
Comments
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)`
* 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
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));
@@ -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;
}
}