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

[Carlos O'Donell <carlos at redhat dot com>]

On 6/27/19 5:12 PM, Florian Weimer wrote:
> * 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 have asked Ulrich for permission to use this text under a license
which would allow us to carry it as documentation for how GNU symbol
versioning is implemented.

I don't have a reply yet.

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

I can agree to that if we agree to the need for documentation and
that we'll update it to follow the changes we make.

Agreed?

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

Perfect, let's do that instead then.

I'm removing the wiki page then.

>> 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.

No worries then.

>> 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.

That's what I thought.

>> 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.

Agreed.
 
>>> +   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).

Agreed. What you have is OK.

>> 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.

Right.

> 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.

OK.

> 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.

I'm trying to avoid a full-day delay due to review and TZ, and get
the patch through more quickly, therefore I try to provide my preconditions
for accepting the patch.

Your patch is OK for master as-is, and I won't block it on updating
documentation. I expect to update the docs when we have them ready for
committing to git.

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

-- 
Cheers,
Carlos.