Re: Proposal: replacing esound with polypaudio in 2.10

[Alan Cox <alan lxorguk ukuu org uk>]

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

> > - Its security model is broken (keys in home directory)
> Come on. Your beloved arts does it the same way

My "beloved" arts. If you think I'm an arts fan you are mistaken, it
merely sucks a lot less.

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

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

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

Some bits of it are a lot saner, eg the client library seems to work
rather like other gnomish code

 x = pa_client_alloc(...)
 pa_client_free(x)

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

Comments would be wonderful, who calls what, what are the assumptions,
what data is user provided. You want to be able to look at any variable
and easily be able to see what size it is if its an array (you do that
well generally and pass lengths too), what range it has, where did it
come from.

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

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"]

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 use access - which is pointless because the files can always change
between the access call and any other.

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

Other issues:
- You use snprintf in the signal handler - thats not guaranteed safe I
believe
- Your write to the_pipe[1] could block or fail in the signal_handler
code. 
- 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])
- 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
- No ipv6 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
- Trusts getaddrinfo response for address length. You need to validate
it is sane(eg 4 byte for IP)
- Should chdir the daemon out of the users dir so if it is left around
  it doesnt lock down automounts
- 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 ?

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.

Alan