Re: [PATCH] ld.so: Support moving versioned symbols between sonames [BZ #24741]

[Florian Weimer <fw at deneb dot enyo dot de>]

* Carlos O'Donell:

> I have dropped Ulrich's text here in our wiki:

Uhm, I don't see a permission notice in the original document.
Did you ask Ulrich?

I don't think we should block this patch on the outcome of that
discussion.

(I would definitely prefer a text file in Git, by the way.)

> An audit of _dl_name_match_p() which is predominantly used for this purpose
> shows callers in dl-load.c, dl-lookup.c, dl-misc.c, dl-version.c, and rtld.c.
>
> I found one questionable assert in dl-lookup.c
>
>   99   if (version != NULL)
>  100     {
>  101       if (__glibc_unlikely (verstab == NULL))
>  102         {
>  103           /* We need a versioned symbol but haven't found any.  If
>  104              this is the object which is referenced in the verneed
>  105              entry it is a bug in the library since a symbol must
>  106              not simply disappear.
>  107 
>  108              It would also be a bug in the object since it means that
>  109              the list of required versions is incomplete and so the
>  110              tests in dl-version.c haven't found a problem.*/
>  111           assert (version->filename == NULL
>  112                   || ! _dl_name_match_p (version->filename, map));
>  113 
>  114           /* Otherwise we accept the symbol.  */
>
> Why would we assert here?

I think it's a corrupt object, where the symbol uses an index that's
outside the symbol table range.  For example, this code in _dl_fixup
doesn't do any range checks:

      if (l->l_info[VERSYMIDX (DT_VERSYM)] != NULL)
	{
	  const ElfW(Half) *vernum =
	    (const void *) D_PTR (l, l_info[VERSYMIDX (DT_VERSYM)]);
	  ElfW(Half) ndx = vernum[ELFW(R_SYM) (reloc->r_info)] & 0x7fff;
	  version = &l->l_versions[ndx];
	  if (version->hash == 0)
	    version = NULL;
	}

That can of course segfault, or give you bad data, leading to
segfaults or asserts later.  I believe the assert you quoted would
fire if by accident we got data that looked like a genuine version
reference, thus implying that the soname needs to have version
information.  (dl-version.c should verify that l_versions references
are actually matched by the listed objects, so I don't see how you can
get the assert without some out-of-bounds lookup.)

I don't think we can write a valid test that hits that assert.

> Can we reasonably handle this error?
> If it is the object referenced in verneed then version->filename != NULL
> and so the assert triggers? That seems overly restrictive, and I expect
> we don't get here because earlier dl-version.c checks fail. I'm not asking
> you to cleanup this code, but it does beg the next question.

> What happens if you move the last versioned symbol out of a library?

You need to ensure that the symbol has the required version definition
records.  I think with BFD ld, you may need to keep an arbitrarily
named versioned symbol under that particular version to retain it
under a non-weak version.

> Are you allowed to stop using the version script in the library that
> has zero versioned symbols? I think the answer is "No." Because we
> still check that all libraries provide the version sets that they
> had at link time? Is that still OK? 

Yes, I think that's acceptable.  We might want to fix ld eventually to
avoid the need for dummy symbol definitions, but I don't think that's
immediatelly necessary.

>> +   This file is part of the GNU C Library.
>> +
>> +   The GNU C Library is free software; you can redistribute it and/or
>> +   modify it under the terms of the GNU Lesser General Public
>> +   License as published by the Free Software Foundation; either
>> +   version 2.1 of the License, or (at your option) any later version.
>> +
>> +   The GNU C Library is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +   Lesser General Public License for more details.
>> +
>> +   You should have received a copy of the GNU Lesser General Public
>> +   License along with the GNU C Library; if not, see
>> +   <http://www.gnu.org/licenses/>.  */
>> +
>> +/* Dummy function to add the required symbol version.  */
>> +void
>> +other_function (void)
>> +{
>> +}
>
> OK. Ah, so you do need a dummy function to preserve the required version
> set or we still get another error?

I didn't even think about it and just added the symbol.  I tried now,
and BFD ld starts producing weak symbol definitions, altering behavior
rather drastically, if you don't do that.  We definitely don't want to
do that for glibc because we do not have the link module/run-time
module separation there, and I think it's valuable to align the test
to that (i.e. not have weak version definitions in the run-time
module).

> I expect the answer is "Yes" and that we're not entirely willing to
> remove this requirement. We're happy to move symbols, but what about
> keeping the original symbol set intact? Would we relax that in a
> subsequent patch?

No, I think we should keep that check by default.  It's perhaps less
important now that many binaries use BIND_NOW, but it's the check that
prevents people from running programs with older glibc versions that
do not provide the required symbol set.

We should really add an option to disable the symbol matching in
dl-version.c, so that you can LD_PRELOAD completely new symbol
versions, but that has quite different applications.

Carlos, this is yet another patch review where you write

  Reviewed-by: Carlos O'Donell <carlos@redhat.com>

with a condition that is difficult to fulfill.  (Sometimes, you also
request non-trivial changes which would potentially invalidate the
review.)  I think it's easier for patch submitters if you delay
approval until you are satisfied with the patch submissions and all
your preconditions have been met.