[PATCH] elf: Fix handling of symbol versions which hash to zero (bug 29190)

[Florian Weimer]

* Adhemerval Zanella Netto:

> This triggers some warnings with make:
>
> Makefile:3410: warning: overriding recipe for target '/mnt/projects/glibc/build/aarch64-linux-gnu/elf/tst-version-hash-zero-mod.so'
> ../Makerules:722: warning: ignoring old recipe for target '/mnt/projects/glibc/build/aarch64-linux-gnu/elf/tst-version-hash-zero-mod.so'
> Makefile:3420: warning: overriding recipe for target '/mnt/projects/glibc/build/aarch64-linux-gnu/elf/tst-version-hash-zero-linkmod.so'
> ../Makerules:722: warning: ignoring old recipe for target '/mnt/projects/glibc/build/aarch64-linux-gnu/elf/tst-version-hash-zero-linkmod.so'
> Makefile:3410: warning: overriding recipe for target '/mnt/projects/glibc/build/aarch64-linux-gnu/elf/tst-version-hash-zero-mod.so'
> ../Makerules:722: warning: ignoring old recipe for target '/mnt/projects/glibc/build/aarch64-linux-gnu/elf/tst-version-hash-zero-mod.so'
> Makefile:3420: warning: overriding recipe for target '/mnt/projects/glibc/build/aarch64-linux-gnu/elf/tst-version-hash-zero-linkmod.so'
> ../Makerules:722: warning: ignoring old recipe for target '/mnt/projects/glibc/build/aarch64-linux-gnu/elf/tst-version-hash-zero-linkmod.so'
>
> Would be possible to the version script with LDFLAGS-* rule?

It will lose the dependency and miss some required rebuilds.  But I
think it may be the only way to do this without warnings in the current
build system.

>> diff --git a/elf/dl-version.c b/elf/dl-version.c
>> index d414bd1e18..2fbf4942b9 100644
>> --- a/elf/dl-version.c
>> +++ b/elf/dl-version.c
>> @@ -357,6 +357,13 @@ _dl_check_map_versions (struct link_map *map, int verbose, int trace_mode)
>>  	      ent = (ElfW(Verdef) *) ((char *) ent + ent->vd_next);
>>  	    }
>>  	}
>> +
>> +      /* The empty string has ELF hash zero.  This avoids a NULL check
>> +	 before the version string comparison in check_match in
>> +	 dl-lookup.c.  */
>> +      for (unsigned int i = 0; i < map->l_nversions; ++i)
>> +	if (map->l_versions[i].name == NULL)
>> +	  map->l_versions[i].name = "";
>>      }
>>  
>>    /* When there is a DT_VERNEED entry with libc.so on DT_NEEDED, issue
>
> Would the test trigger this? I tried to comment this out and at least
> with valgrind tst-version-hash-zero does not trigger any invalid
> memory access.

This can only happen if there is a gap in the version indexes, so
testing this would require binary patching.  We could error out in this
case, I think.  Maybe as a separate change?

>> diff --git a/elf/tst-version-hash-zero.c b/elf/tst-version-hash-zero.c
>> new file mode 100644
>> index 0000000000..ad391bf202
>> --- /dev/null
>> +++ b/elf/tst-version-hash-zero.c
>> @@ -0,0 +1,55 @@

>> +  /* tst-version-hash-zero-refmod.so references
>> +     global_variable@@OTHER_VERSION and is expected to fail to load.
>> +     dlvsym sets the hidden flag during lookup.  Relocation does not,
>> +     so this exercises a different failure case.  */
>> +  TEST_VERIFY (dlopen ("tst-version-hash-zero-refmod.so", RTLD_NOW) == NULL);
>> +  const char *message = dlerror ();
>> +  if (strstr (message,
>
> This is strictly invalid is dlopen does not fail (and the below test is
> not TEST_VERIFY_EXIT). Maybe use 'message == NULL || ...' here.

I'll switch to TEST_VERIFY_EXIT.

Thanks,
Florian