Bug 55180 - daemon shouldn't depend on libpulse
Summary: daemon shouldn't depend on libpulse
Status: RESOLVED MOVED
Alias: None
Product: PulseAudio
Classification: Unclassified
Component: build-system (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: pulseaudio-bugs
QA Contact: pulseaudio-bugs
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-21 08:13 UTC by Pierre Ossman
Modified: 2018-07-30 09:35 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
pulse-split.patch (9.64 KB, patch)
2012-09-21 08:32 UTC, Pierre Ossman
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pierre Ossman 2012-09-21 08:13:31 UTC
The pulseaudio binary (and a bunch of other stuff), has an incorrect dependency on libpulse. This was introduced in commit df6e38bfd (yes I know it's ancient. But that was still after the last time I was involved in the project, so I didn't spot it until now):

commit df6e38bfd226db442783e8a1cb6af0f656139765
Author: Lennart Poettering <lennart@poettering.net>
Date:   Tue Oct 21 23:55:33 2008 +0200

    temporary commit to allow flameeyes a look

Seem like the commit wasn't terribly temporary. :)


The so-files are supposed to be divided as such:

 - lubpulsecore : Stuff that _only_ the server needs

 - libpulse : Stuff that _only_ the client needs

 - libpulsecommon : Stuff that _both_ the client and the server needs

With the server depending on libpulse, libpulsecommon is entirely redundant and might as well be merged into libpulse.

Or we go back to the old system, reducing the stuff loaded by the server.
Comment 1 Pierre Ossman 2012-09-21 08:32:13 UTC
Created attachment 67482 [details] [review]
pulse-split.patch

Suggested patch that splits things back up again. Only mildly tested.
Comment 2 Peter Åstrand 2013-03-28 14:47:27 UTC
If there's anything we can do to make it easier to integrate this work, please let us know. Would a git pull request help?
Comment 3 Tanu Kaskinen 2013-03-29 12:39:59 UTC
Sorry for ignoring this.

The patch moves a bunch of client API stuff to libpulsecommon. That is not acceptable, because that would require applications to link to libpulsecommon. libpulsecommon contains functions that are used by the daemon and libpulse, but which must not be visible to applications. It's not possible to allow applications to link to libpulsecommon without also making all the internal stuff visible to them.
Comment 4 Pierre Ossman 2013-03-29 19:30:58 UTC
Applications are already linking against libpulsecommon (via libpulse), so they are already seeing everything that's in there. So this patch does not make anything visible that wasn't already.

I'm afraid the only way to hide symbols is to not export them at all. Which would mean they would have to be inside libpulse. But that also means that the server also could not access them either, so it would have to have a separate copy in libpulsecore. I'm not opposed to that model though, and it probably wouldn't require that much work.
Comment 5 Tanu Kaskinen 2013-04-01 07:30:37 UTC
(In reply to comment #4)
> Applications are already linking against libpulsecommon (via libpulse), so
> they are already seeing everything that's in there.

Applications can't use the functions in libpulsecommon without linking to it directly, and they should not link to it directly.

> I'm afraid the only way to hide symbols is to not export them at all. Which
> would mean they would have to be inside libpulse. But that also means that
> the server also could not access them either, so it would have to have a
> separate copy in libpulsecore. I'm not opposed to that model though, and it
> probably wouldn't require that much work.

Having separate copies of the common functionality in libpulse and libpulsecore sounds reasonable to me. It would somewhat increase the size on disk and memory, but that doesn't sound too bad to me. libpulsecommon could still exist as a static library (included in libpulse and libpulsecore) to avoid compiling things twice. To see if others have different views, I think it would be a good idea to bring up this topic on the mailing list, if you're going to work on this.

BTW, are you having some practical problems with the current setup, or do you want to fix this only because it's ugly to link the daemon to libpulse?
Comment 6 David Henningsson 2013-04-04 09:22:45 UTC
It looks like the meaning of these libraries have changed over time.

Previous meaning is the one Pierre says: libpulse for client, libpulsecore for server, libpulsecommon for both.

Current meaning is: libpulse contain public API functions for clients. libpulsecommon contains other functions internally used by libpulse, pulseaudio and libpulsecore.

If we need to bring back the old meaning without losing the new meaning, it sounds like we would need to split libpulsecommon into libpulsecommon-public and libpulsecommon-hidden?
Comment 7 David Henningsson 2013-04-04 09:30:58 UTC
To comment on Pierre's "clients can see libpulsecommon symbols anyway", could you clarify:

The versioning of libpulse should stay stable, so that clients do not need recompiling (always linking against libpulse.so.0).

libpulsecommon symbols we want to add, remove, change as we wish between PulseAudio versions, because there are plenty of internal functions used in both client and server, that change over time with the internals.

If clients start to link directly to libpulsecommon, we would be forced to make libpulsecommon symbols more stable in order not to break the client ABI. This sounds like a major drawback to me.

Am I missing something?
Comment 8 Pierre Ossman 2013-04-12 06:46:57 UTC
(In reply to comment #5)
> 
> Applications can't use the functions in libpulsecommon without linking to it
> directly, and they should not link to it directly.
> 

Actually they can. A short primer on ELF dynamic linking:

When you create an ELF binary, two lists gets put in the result:

 a) A list of missing symbols. E.g. malloc() and pa_simple_new().

 b) A list of _suggested_ libraries where these symbols can be found.

These two lists are not connected in any way. There is no information about which symbol comes from where, or even if all libraries are needed or sufficient.

At runtime the system will load all the libraries in list b). It will also load all libraries that they depend on. So libpulse.so is in b), but libpulsecommon.so will also get loaded in round 2 because libpulse.so depends on it.

After this is done, it will start resolving symbols. libpulse.so gets priority because it was loaded first, but it will not refrain from also looking in libpulsecommon.so for symbols.


In other words; an application will see all the symbols of your library, _plus_ all the symbols of each and every library that gets loaded as a dependency.


The only fool-proof way of hiding symbols is to put everything in a single .so file and completely avoid putting those symbols in the export list. Once you have two .so files you've already lost.

The more common approach is to not try to prevent misuse, but to encourage proper use. Do this by documenting. No one is going to run objdump on your .so files to figure out the API. They will be reading your headers and documentation. So use those files to clearly separate public from private stuff.

> Having separate copies of the common functionality in libpulse and
> libpulsecore sounds reasonable to me. It would somewhat increase the size on
> disk and memory, but that doesn't sound too bad to me. libpulsecommon could
> still exist as a static library (included in libpulse and libpulsecore) to
> avoid compiling things twice. To see if others have different views, I think
> it would be a good idea to bring up this topic on the mailing list, if
> you're going to work on this.

Not entirely sure libtool will let us to the right thing here. Static libraries usually don't get compiled with -fPIC. And getting an entire .a file linked is generally a bit pesky. I don't think compile time is that much of a problem?

> 
> BTW, are you having some practical problems with the current setup, or do
> you want to fix this only because it's ugly to link the daemon to libpulse?

Sorry, it was a while since I did this so I've forgotten the details. :/
The primary issue was reducing the amount of stuff we needed to ship though.
Comment 9 Pierre Ossman 2015-07-02 11:18:39 UTC
This has been lingering a while, but I'd like to get this sorted out. :)

As I see it, there are three options:

1. Keep disk and runtime size small

This is the original approach, and what my earlier patch does. I.e.:

 - Clients use libpulse and libpulsecommon
 - Servers use libpulsecore and libpulsecommon

IOW, nothing the client needs in libpulsecore and nothing the server needs in libpulse.

2. Keep disk size small, and less confusing structure

 - Clients use libpulse
 - Servers use libpulse and libpulsecore

Basically the same as 1, just that libpulsecommon is merged with libpulse. Nothing will really change for clients, but servers will be forced to load a slightly larger library. 

(And only shipping the server still means shipping some client stuff)

3. Protect API

 - Clients use libpulse
 - Servers use libpulsecore

Here we duplicate code between libpulse and libpulsecore, but gain the ability to lock down the exported symbols in libpulse so that no one abuses the API.



I can be prepared to implement this. We just need to decide on which one. :)
3 seemed to be the preferred from David at least?
Comment 10 GitLab Migration User 2018-07-30 09:35:12 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/pulseaudio/pulseaudio/issues/35.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.