Re: [pulseaudio-discuss] incomplete IPv6 support in pa_socket_client_new_string()
Tanu Kaskinen wrote: On Thu, 2014-11-06 at 22:26 -0600, Hajime Fujita wrote: Tanu Kaskinen wrote: On Wed, 2014-11-05 at 22:29 -0600, Hajime Fujita wrote: Hi Tanu, Thank you for your comment and sorry for the late reply. Tanu Kaskinen wrote: 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. Do you have any justification behind this? Yes, sorry for leaving it out from my answer. The justification: I think you can't actually implement suggestion b, because there's no way you can figure out in pa_socket_client_new_string() whether 1234::1234:1234 contains only an IP address or an IP address and a port. Well my justification for option b was: if the caller wants to pass addr:port style string to pa_socket_client_new_string(), addr must be surrounded by brackets for the very reason you described. So, if pa_socket_client_new_string() sees a string like 1234::1234:1234, I thought it would be safe to interpret it as an IPv6 address alone, not including port. Ok, now I understand (and looking back, I feel I should have understood already from the first mail...). I think pa_parse_address() should be changed so that 1234::1234:1234 is parsed as an IP address, not IP address + port. That way the problem is fixed for all callers of pa_parse_address(), not only for pa_socket_client_new_string(). Thanks, this makes sense. I'll send a patch for this. Best, Hajime ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] incomplete IPv6 support in pa_socket_client_new_string()
On Thu, 2014-11-06 at 22:26 -0600, Hajime Fujita wrote: Tanu Kaskinen wrote: On Wed, 2014-11-05 at 22:29 -0600, Hajime Fujita wrote: Hi Tanu, Thank you for your comment and sorry for the late reply. Tanu Kaskinen wrote: 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. Do you have any justification behind this? Yes, sorry for leaving it out from my answer. The justification: I think you can't actually implement suggestion b, because there's no way you can figure out in pa_socket_client_new_string() whether 1234::1234:1234 contains only an IP address or an IP address and a port. Well my justification for option b was: if the caller wants to pass addr:port style string to pa_socket_client_new_string(), addr must be surrounded by brackets for the very reason you described. So, if pa_socket_client_new_string() sees a string like 1234::1234:1234, I thought it would be safe to interpret it as an IPv6 address alone, not including port. Ok, now I understand (and looking back, I feel I should have understood already from the first mail...). I think pa_parse_address() should be changed so that 1234::1234:1234 is parsed as an IP address, not IP address + port. That way the problem is fixed for all callers of pa_parse_address(), not only for pa_socket_client_new_string(). -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] incomplete IPv6 support in pa_socket_client_new_string()
Tanu Kaskinen wrote: On Wed, 2014-11-05 at 22:29 -0600, Hajime Fujita wrote: Hi Tanu, Thank you for your comment and sorry for the late reply. Tanu Kaskinen wrote: 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. Do you have any justification behind this? Yes, sorry for leaving it out from my answer. The justification: I think you can't actually implement suggestion b, because there's no way you can figure out in pa_socket_client_new_string() whether 1234::1234:1234 contains only an IP address or an IP address and a port. Well my justification for option b was: if the caller wants to pass addr:port style string to pa_socket_client_new_string(), addr must be surrounded by brackets for the very reason you described. So, if pa_socket_client_new_string() sees a string like 1234::1234:1234, I thought it would be safe to interpret it as an IPv6 address alone, not including port. Thanks, Hajime ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] incomplete IPv6 support in pa_socket_client_new_string()
Hi Tanu, Thank you for your comment and sorry for the late reply. Tanu Kaskinen wrote: 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. Do you have any justification behind this? Thanks, Hajime ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] incomplete IPv6 support in pa_socket_client_new_string()
On Wed, 2014-11-05 at 22:29 -0600, Hajime Fujita wrote: Hi Tanu, Thank you for your comment and sorry for the late reply. Tanu Kaskinen wrote: 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. Do you have any justification behind this? Yes, sorry for leaving it out from my answer. The justification: I think you can't actually implement suggestion b, because there's no way you can figure out in pa_socket_client_new_string() whether 1234::1234:1234 contains only an IP address or an IP address and a port. -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] incomplete IPv6 support in pa_socket_client_new_string()
On Fri, 2014-10-31 at 18:07 -0600, Glenn Golden wrote: 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. I once checked this from Wikipedia: the DNS specification allows any characters, so from that perspective there's no bug. The root DNS servers that manage the Internet only allow letters, digits and hyphens, however. I think it would make sense to restrict the domain names similarly in PulseAudio, so the bug that you reported is valid, even though in theory such validation may cause breakage on private networks with crazy names. -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] incomplete IPv6 support in pa_socket_client_new_string()
On Tue, 2014-10-28 at 06:10 -0600, Glenn Golden wrote: Tanu Kaskinen tanu.kaski...@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? 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)). 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? -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] incomplete IPv6 support in pa_socket_client_new_string()
Tanu Kaskinen tanu.kaski...@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.kaski...@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.
___
Re: [pulseaudio-discuss] incomplete IPv6 support in pa_socket_client_new_string()
Tanu Kaskinen tanu.kaski...@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 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. Regards, Glenn ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
