Bug 55180

Created attachment 67482 [details] [review]
pulse-split.patch

Suggested patch that splits things back up again. Only mildly tested.

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?

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.

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.

(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?

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?

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?

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

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?

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