Re: Proposal: replacing esound with polypaudio in 2.10

[Lennart Poettering <mztabzr 0pointer de>]

On Thu, 28.10.04 21:26, Alan Cox (alan lxorguk ukuu org uk) wrote:

> On Iau, 2004-10-28 at 21:35, Lennart Poettering wrote:
> > > - It has no synchronization with X
> > Please explain what exactly you mean with this.
> 
> Firstly X has a synchronization extension secondly even without that you
> want the function "play this sound once the following X property
> changes". That helps you keep sync between the client and server when
> the X and sound are going down different sockets

The X Synchronization Extension seems simple enough. I will add
support for it a soon as I find the time for it. 

I must admit that my programming experience with X11 is rather
limited, in fact the only code I ever wrote that uses the xlibs
directly is the module-x11-bell for polypaudio which traps X11 bell
events and plays a sample from the sample cache.

> > they store the key in /tmp instead of $HOME, but where's the
> > difference?). The only real difference i see is that they use a MD5 auth,
> > instead of plain like esd and polypaudio. However, that's trivial to
> > fix.
> 
> I would suggest you perhaps stick the auth key (or a copy of it) as a
> property on the root window. Now your X authentication protections your
> audio authentication. No more insane hackery with thin clients as is the
> problem with both esd and arts. (Note - a client could do this at
> session start up if you want to keep X and the daemon apart)

That's a very good idea and simple to implement. There's already a
module connecting to X11 anyway (see above), so I can add this in
another module as well.

> > > - It has no way to pass audio descriptions for the hearing impaired
> > >   (who may want both audio and synchronized mark up).
> > 
> > What do you expect from a sound server to do with this data? 
> > I see no problem in adding a path for such meta data to be passed down
> > to the sound driver, if that makes you happy.
> 
> That would make sense - it's just another plugin after all.

You're right. Could you tell me what format this meta data should have?

anything more than just a bunch of tuples [timestamp, text]?

> > > It's not compatible with KDE while moving to arts would be.
> > Come on. Arts is dead
> 
> Where do KDE plan to go - to polypaudio - one audio for both would be a
> godsend.. 

I proposed adoption of polypaudio to the KDE people on the respective
mailing list, but they seem to be a little uncertain what to do. Some
believe that gstreamer would solve their needs for a sound server,
others that NMM or MAS would be solution. I personally don't consider
gstreamer being a "sound server", so the discussion was very
unconclusive.

In case the Gnome projects decides to adopt Polypaudio I would be in a much
stronger position for convining them to adopt it as well. ;-)
 
> > > I would like to see the zillions of wrappers that make the code hard to
> > > follow removed, all the pointer to point passing and argument poking
> > > around cleaned up into proper objects and some commenting.
> > 
> > Execuse me? Please elaborate! "zillions of wrappers"? "pointers to
> > point"? "argument poking around"? What do you mean?
> 
> Gems like this
> 
> int pa_create_io_events(snd_pcm_t *pcm_handle, struct pa_mainloop_api* m,
>                         struct pa_io_event ***io_events, 
>                         unsigned *n_io_events, 
>                          void (*cb)(struct pa_mainloop_api*a, struct pa_io_event *e, int fd, enum pa_io_event_flags events, void *userdata), void *userdata) {
> 
> This makes it hard to read, and hard to audit because you don't have a
> clear flow of information change. Eg the code in that function breaks if
> n_io_events is very large, but its very hard to verify the value
> range. 

Tsss. You picked an interesting piece of code, really. Would
commenting the function suffice your needs? 

The code you mention is ugly, I know, but this is mostly due to the
fact that it implements the link of the ALSA API to the main loop
implementation of polypaudio - and the ALSA API isn't the prettiest
API itself.

> > > I tried to audit it and its currently near unauditable. To me if the
> > > code isn't easily auditable as a daemon then that alone is a
> > > showstopper.
> > 
> > What does it need to make it auditable to you? 
> 
> You need to be able to see where data came from and whether it has
> already been validated. That is the big thing. Also using existing code
> where possible - eg I know g_string is secure, its been audited about
> five times. I've no idea if pa_strbuf_printf is without doing a complete
> analysis of it (and indeed it will do spectacularly bad things
> fed 2Gb of input). So because its new code I have to look at it,
> identify any weaknesses (eg this 2Gb one is a clone of a fixed glib
> weakness) then go back and verify the users are safe  eg there is no
> single strbuf_printf(..) that is passed user data without "%s" properly
> used.

I didn't make use of glib for polypaudio because I want both KDE and
Gnome to adopt polypaudio. As you probably know the KDE people don't
like glib very much.

pa_strbuf uses snprintf internally. So it fails under the same conditions
as snprintf.

Wherever possible I did not implement anything which was already
existant in the libc. But due to the fact, that I do not link against
glib, I have to reimplement some stuff which exists in glib.

> > In the same way you can replace the current, basic mixing
> > implementation of polypaudio. So, what's your point?
> 
> That would be a good idea if people decide polypaudio is the way to go
> and would fix a lot of the other technical issues about the audio
> mixing. (and fair is fair audio mixing, volume and rate adaption are
> not trivial, they just look it which is why esd rate adaption sounds
> like singing down a drainpipe)

For resampling polypaudio relies on libsamplerate (aka "Secret Rabbit
Code"), which contains a few different interpolators.
 
> Other bits I noticed - you use setuid()/setuid() to drop privileges.
> When available setresuid() is a lot safer because the semantics of
> dropping the saved setuid (the 3rd copy) are interesting in portable
> code [read "old unix is broken new unix fixed it"]

I changed that now. Thanks for the tip.

> Where you getenv() for things like /home remember that rules out keeping
> the sound daemon running across multiple users. You also want to use
> getpwnam() to fallback if $HOME is not set.

You can specify all options of polypaudio on the command line or
specify a non-standard configuration file if you like. Polypaudio just
looks for the configuration script in $HOME first, in case it is not
told to do otherwise. The auth cookie and socket file can be specified
on the respective module's arguments line. So there is really no
limitation to run multiple polypaudio instances as the same user
concurrently or have a single running system wide.

> You use access - which is pointless because the files can always change
> between the access call and any other.

Yes, you're right on that, but only when locating the configuration
file to use. So it ain't a security hole. Who edits his configuration
files in the same time as he starts polypaudio anyway?

> I'm curious why you use pa_* functions where glib functions exist - did
> the project just start out independant ?

Polypaudio doesn't depend on glib. For the reasons, see above.

Polypaudio comes with a glib main loop adapter for easily embedding
the client library into GTK applications.

> Other issues:
> - You use snprintf in the signal handler - thats not guaranteed safe I
> believe

As far as I know (f)printf() is not safe since it accesses a FILE
object, but snprintf should be safe.

> - Your write to the_pipe[1] could block or fail in the signal_handler
> code. 

No. It can't, due to the fact that the pipe is a nonblocking
pipe. Since I ignore the return value of write() signals may be lost,
but that would mean that more than 2000 signals have been caught but
not delivered yet. 

> - pa_get_binary_name is as you note not portable so would want curing
>   (actually its unfixable it might be deleted/renamed before you ask
>    you just have argv[0])

That function is used in the libao driver only. Since that is a plugin
for a library I have no access to argv[]. The routine is used for
sending a pretty stream name to the daemon so that user may identify
his streams easily. If that name cannot be read, it is simple set to
"libao". Short: it's just usef for eyecandy and is otherwise useless.

> - What stops esd connection_write from expanding the buffer until we
>   run out of ram - eg because a connection we are recording from stalls
>   for two hours due to network failure or use of ^Z in the client

connection_write() is not called in that case. connection_write() is
used only for control messages which are very unlikely to overflow.

What the user can, however, is writing thousands of control messages to
the server without reading the server's replies. That would make the
server to allocated more and more memory.

I'll fix this, thanks, for the tip!

> - No ipv6 support

I have no IPv6 setup here, but I surely accept patches providing IPv5
support. ;-)

> - Uses TCP_NODELAY - thats probably wrong for streaming audio, very
> wrong in that situation. TCP_CORK is the ideal API but its linux
> only

I do believe that NODELAY is a good idea in this case. Why? It lowers the
latency of network transmissions. Polypaudio usually writes very large
blocks to the sockets and it usually takes some time to get the next
block (i.e. this is controlled by the soundcard's crystal), so most of
the time there's no point in waiting for more data.

One of my most important test cases is video playback of mplayer over
a network. It works extraordinarily well, with perfect lip
synchronization when using TCP_NODELAY. (I must admit that I didn't
test it without... ;-))

Most of the time I have more interest in low and stable latancies in
favor of throughput. That's why I enabled NODELAY.

> - Trusts getaddrinfo response for address length. You need to validate
> it is sane(eg 4 byte for IP)

Hmpf. I always trust what the libc/kernel tells me. 

> - Should chdir the daemon out of the users dir so if it is left around
>   it doesnt lock down automounts

I changed that. Thanks.

> - Not sure if Im just missing where the code is but I don't see code
> handling the case of me just making 1000 connections and not
> authenticating - nothing seems to bump stale unauth connections ?

Yes. You're right. 

> And please - I'm not trying to lobby for arts, I want a sound daemon
> that does cool stuff, securely. Something that is maintainable for all
> us poor distribution schmucks. If the world wants polypaudio then I'll
> carry on auditing it further because it'll need auditing for our vendor
> use so it might as well get done in advance.

You convinced to go over the code once again and make sure that every
resource a networked client has access to is limited to certain
ranges. ;-)

You're very welcome in giving more tips like these above.

Thank you,
-- 
name { Lennart Poettering } loc { Hamburg - Germany }
mail { mzft (at) 0pointer (dot) de } gpg { 1A015CC4 }  
www { http://0pointer.de/lennart/ } icq# { 11060553 }