[pulseaudio-discuss] incomplete IPv6 support in pa_socket_client_new_string()

[Glenn Golden]

Tanu Kaskinen <tanu.kaskinen at linux.intel.com> [2014-10-31 14:03:56 +0200]:
> On Tue, 2014-10-28 at 06:10 -0600, Glenn Golden wrote:
> > Tanu Kaskinen <tanu.kaskinen at linux.intel.com> [2014-10-28 11:20:08 +0200]:
> > > On Tue, 2014-10-21 at 00:49 -0500, Hajime Fujita wrote:
> > > > Hello,
> > > > 
> > > > I'm currently working on IPv6 support for the raop module [1].
> > > > 
> > > > During the work I found a potential issue in 
> > > > pa_socket_client_new_string() (in pulsecore/socket-client.c) as pointed 
> > > > out in [1].
> > > > 
> > > > It does not work as expected when an IPv6 address string like 
> > > > "2001:db8::1" is passed as the "name" parameter. This is because the 
> > > > name parameter is passed to pa_parse_address(), which thinks the last 
> > > > colon as a separator between hostname (or address) and a port number. To 
> > > > prevent pa_parse_address() from doing this, an IPv6 address must be 
> > > > bracketed with "[]" (e.g. "[2001:db8::1]").
> > > > 
> > > > I'm wondering what would be the best solution for this situation.
> > > > a. Modify callers: callers of pa_socket_client_new_string() must add 
> > > > brackets to IPv6 addresses.
> > > > b. Modify pa_socket_client_new_string(): if an IPv6 address is given as 
> > > > the name parameter, it will be bracketed before being passed to 
> > > > pa_parse_address().
> > > > 
> > > > Any opinions or suggestions?
> > > 
> > > I think it makes more sense to fix this in the callers of
> > > pa_socket_client_new_string(). It would be good to also add a comment in
> > > socket-client.h saying that the name parameter can include also the port
> > > using the usual syntax, and for that reason IPv6 addresses must be
> > > enclosed in brackets.
> > > 
> > 
> > This is kind of a vague comment/question, but I'll ask anyway: Is there any
> > potential here for simplification/standardization/enforcement of a canonical
> > syntax for PA server addressing to be used throughout the PA libs and at
> > the user level?
> > 
> > The above bracketed IPv6 syntax seems (naively to me, anyway) to perhaps be 
> > an superfluous variation on the user-level server string syntax described in
> > 
> >     http://www.freedesktop.org/wiki/Software/PulseAudio/Documentation/User/ServerStrings
> 
> In what way is it superfluous?
>

It isn't. My comment was superfluous. :)

I had assumed -- incorrectly, now that I've looked at the source -- that
pa_parse_address() accepted only a bare host spec (e.g. "some.host.com" or
numerical equivalent) along with a port number as an additional parameter.
So I thought (wrongly) that it did not accept the leading {tcp, tcp4, tcp6}
prefix that is discussed in the Wiki "Server strings" page.  But it does
accept those prefixes, so my comment about "superfluous variation" was
misguided.  Evidently, that Wiki format (optionally including the prefix)
_is_ the canonical addressing form used throughout the PA libs, at least under
the assumption that pa_parse_address() is the core routine used for all address
parsing.

>
> The bracketed syntax for IPv6 to separate the IP address from the port is
> standard (and should be mentioned in the parsing rules on that wiki page too,
> if the parser in PulseAudio checks whether the IP address is bracketed
> (I didn't check whether that's the case, but I'd expect so)).
> 

The Wiki writeup does not go into any detail on the syntax for separting port
number from IP address, it simply says

    "If the string starts with tcp6: a similar rule applies, but this
     time for IPv6"

thus assuming that the reader is familiar with the standard syntax for IPv6,
which includes brackets only when necessary, not always.  (No argument with
that assumption, just mentioning it.) 


> > Additionally, judging from actual PA behavior (5.0), the libs are presently
> > not performing any syntax checking on user-supplied server strings anyway,
> > e.g. see
> > 
> >     https://bugs.freedesktop.org/show_bug.cgi?id=83657
> > 
> > What I'm getting at: Since this issue looks like it's getting some attention
> > now, might it make sense to think about a cleanup that enforces a single
> > unified syntax for both user- and API-internal server specification strings,
> > and hence the possibility of a single routine that validates them (hence
> > contributing to a future fix for the above ticket)?
> > 
> > Again, this comment may not even make sense due to my unfamiliarity with the
> > big picture, just trying to supply a useful thought, since Hajime had asked
> > for opinions and suggestions.
> 
> As far as I can tell, the issue that Hajime had was not about PulseAudio
> server strings but addresses used to refer to a remote RAOP server.
> Given that, do you still think there would be need for some unification?
> 

See above. My unification comment was misguided.

However, regarding syntax validation: Judging from pactl's behavior in the
above ticket (83657) pa_parse_address() either isn't doing a very careful
job of validation, or the error return is being ignored by pactl. It (pactl)
acccepts some pretty nonsensical server specification strings without
complaining.