Autodetection of packages that need visibility annotations

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

Autodetection of packages that need visibility annotations

Yuri Gribov
Hi all,

Linux shared libraries by default allow for runtime interposition of
symbols. This comes in handy for customization of malloc but might
also have undesired consequences. Firstly, it badly affects
performance of generated code as
1) compiler is not allowed to optimize (inline/clone/split) functions
in fear that they will be interposed
2) calls to intra-library functions have to go through PLT
3) ld.so symbol resolution takes longer time (due to larger dynamic
symbol tables)
Also interposition hurts code correctness as it increases the chance of
1) inadvertent symbol collisions (see Flameeys'
https://flameeyes.blog/2013/02/23/redundant-symbols/ and other related
posts on his blog)
2) clients relying on private symbols (and later breaking and
complaining when such symbols are removed)

Recommended way to fix interposition is to build libraries with
`-fvisibility=hidden` and use [GNU visibility
annotations](https://gcc.gnu.org/wiki/Visibility) to mark public
symbols. In effort to promote a wider use of those in open-source
projects, I've recently made a simple tool to help locate Debian
packages which would benefit the most from such annotations
(https://github.com/yugr/ShlibVisibilityChecker). The tool (based on
libclang) compares symbols exported from shlibs with those declared in
package headers and reports the difference i.e. exported symbols which
aren't present in headers. The idea is that such symbols are likely to
be internal library symbols and thus should be marked as hidden.

Here's an example usage for libacl1 package:

# # Needs to be run in chroot
# make clean all
# scripts/debiancheck libacl1
Binary symbols not in public interface of acl:
  __acl_extended_file
  __acl_from_xattr
  __acl_to_xattr
  __bss_start
  closed
  _edata
  _end
  _fini
  head
  high_water_alloc
  _init
  next_line
  num_dir_handles
  walk_tree
For a total of 14 (25%).

Tool, albeit heuristic and imprecise, turned out to be useful - it
could successfully process ~60% of packages I tried (main reason for
failures were missing #includes in packages' public headers which are
arguably package bugs). Some resulting upstream bugs are in
https://github.com/yugr/ShlibVisibilityChecker#tropheys

The main reason for writing this mail is that albeit I'd be happy to
do more visibility patching myself, Debian has 50K packages and it
would be infeasible for me, both in terms of computing power and
personal time to handle this task on my own (not to mention that
upstreams may be hesitant to take patches from unknown contributors).
Can I suggest that interested maintainers try the tool on packages
that they maintain and add visibility annotations if they turn out to
provide noticeable savings (or at least bug upstream projects to
enable visibility themselves)? I'd obviously be happy to fix any
arising bugs or add missing features to the tool.

-Yury Gribov

Reply | Threaded
Open this post in threaded view
|

Re: Autodetection of packages that need visibility annotations

Paul Wise via nm
On Tue, May 15, 2018 at 1:29 PM, Yuri Gribov wrote:

> In effort to promote a wider use of those in open-source
> projects, I've recently made a simple tool to help locate Debian
> packages which would benefit the most from such annotations
> (https://github.com/yugr/ShlibVisibilityChecker).

In case you would like to get this (or your other QA tools) into
Debian, I would be happy to sponsor you.

https://mentors.debian.net/intro-maintainers

--
bye,
pabs

https://wiki.debian.org/PaulWise

Reply | Threaded
Open this post in threaded view
|

Re: Autodetection of packages that need visibility annotations

Yuri Gribov
On Tue, May 15, 2018 at 7:28 AM, Paul Wise <[hidden email]> wrote:
> In case you would like to get this (or your other QA tools) into
> Debian, I would be happy to sponsor you.

Thanks Paul, that's very inspiring! I would be glad to work on
integration to Debian if community decides that tool(s) are useful.

-Y

Reply | Threaded
Open this post in threaded view
|

Re: Autodetection of packages that need visibility annotations

Andrey Rahmatullin-3
In reply to this post by Yuri Gribov
There is an interesting question about this: technically speaking,
removing exported symbols requires a soname bump. But one can say that
those symbols were never public and so they are not a part of ABI.
Fixing this upstream and bumping the soname should be easier rthough..

--
WBR, wRAR

signature.asc (911 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Autodetection of packages that need visibility annotations

Yuri Gribov
On Tue, May 15, 2018 at 9:22 AM, Andrey Rahmatullin <[hidden email]> wrote:
> There is an interesting question about this: technically speaking,
> removing exported symbols requires a soname bump. But one can say that
> those symbols were never public and so they are not a part of ABI.
> Fixing this upstream and bumping the soname should be easier rthough..

Right, TBH until now I haven't bothered with soname bumps when
submitting visibility-related patches to upstream...

-Y

Reply | Threaded
Open this post in threaded view
|

Re: Autodetection of packages that need visibility annotations

Simon McVittie-7
On Tue, 15 May 2018 at 10:44:27 +0100, Yuri Gribov wrote:
> On Tue, May 15, 2018 at 9:22 AM, Andrey Rahmatullin <[hidden email]> wrote:
> > There is an interesting question about this: technically speaking,
> > removing exported symbols requires a soname bump. But one can say that
> > those symbols were never public and so they are not a part of ABI.
> > Fixing this upstream and bumping the soname should be easier rthough..
>
> Right, TBH until now I haven't bothered with soname bumps when
> submitting visibility-related patches to upstream...

If the symbols aren't considered to be public (the upstream developer
should know) then you shouldn't bump the SONAME, to avoid unnecessary
ABI transitions and disruption.

    smcv

Reply | Threaded
Open this post in threaded view
|

Re: Autodetection of packages that need visibility annotations

Yuri Gribov
On Tue, May 15, 2018 at 10:56 AM, Simon McVittie <[hidden email]> wrote:

> On Tue, 15 May 2018 at 10:44:27 +0100, Yuri Gribov wrote:
>> On Tue, May 15, 2018 at 9:22 AM, Andrey Rahmatullin <[hidden email]> wrote:
>> > There is an interesting question about this: technically speaking,
>> > removing exported symbols requires a soname bump. But one can say that
>> > those symbols were never public and so they are not a part of ABI.
>> > Fixing this upstream and bumping the soname should be easier rthough..
>>
>> Right, TBH until now I haven't bothered with soname bumps when
>> submitting visibility-related patches to upstream...
>
> If the symbols aren't considered to be public (the upstream developer
> should know) then you shouldn't bump the SONAME, to avoid unnecessary
> ABI transitions and disruption.

As a side note, in one of my other projects I tried scanning reverse
package dependencies (recursively) to detect various symbol anomalies
but that required amounts of storage and compute power well beyond of
what a single workstation can provide...

-Y

Reply | Threaded
Open this post in threaded view
|

Re: Autodetection of packages that need visibility annotations

Paul Wise via nm
On Tue, May 15, 2018 at 10:58 PM, Yuri Gribov wrote:

> As a side note, in one of my other projects I tried scanning reverse
> package dependencies (recursively) to detect various symbol anomalies
> but that required amounts of storage and compute power well beyond of
> what a single workstation can provide...

The 'adequate' tool does some of that:

missing-symbol-version-information
symbol-size-mismatch
undefined-symbol

https://packages.debian.org/unstable/adequate

--
bye,
pabs

https://wiki.debian.org/PaulWise

Reply | Threaded
Open this post in threaded view
|

Re: Autodetection of packages that need visibility annotations

Yuri Gribov
On Wed, May 16, 2018 at 2:32 AM, Paul Wise <[hidden email]> wrote:
> The 'adequate' tool does some of that:
>
> missing-symbol-version-information
> symbol-size-mismatch
> undefined-symbol
>
> https://packages.debian.org/unstable/adequate

Thanks, that's a nice one (what a relief to read Perl rather than Python).

My work was different in that I had to scan all existing ELFs (in all
50K packages) which implies usage of database, parallel processing of
packages, etc.

-Y