Bug 61922 - Fix IPv6 support on windows implementation
Summary: Fix IPv6 support on windows implementation
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: All Windows (All)
: medium normal
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2013-03-06 21:51 UTC by Ralf Habacker
Modified: 2018-03-20 12:31 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Detect ipv6 support and handler related error cases (3.01 KB, patch)
2015-01-31 12:42 UTC, Ralf Habacker
Details | Splinter Review
Fix IPV6 support in _dbus_get_peer_pid_from_tcp_handle() on Windows (5.22 KB, patch)
2017-12-02 00:50 UTC, Ralf Habacker
Details | Splinter Review
Fix dbus-daemon startup on Windows in case no family is specified and ipv6 is disabled (7.31 KB, patch)
2018-02-14 13:18 UTC, Ralf Habacker
Details | Splinter Review
Let dbus-daemon be more descriptive in case of ipv6 listen errors on Windows (1.94 KB, patch)
2018-02-14 13:18 UTC, Ralf Habacker
Details | Splinter Review
Fix msvc runtime check failure "The variable 'has_ipv6' is being used without being initialized (821 bytes, patch)
2018-03-07 15:43 UTC, Ralf Habacker
Details | Splinter Review
Fix "failed to bind socket: Cannot assign requested address" on linux with ipv6 disabled (3.20 KB, patch)
2018-03-08 08:21 UTC, Ralf Habacker
Details | Splinter Review
Fix dbus-daemon startup on Windows in case no family is specified and ipv6 is disable (7.32 KB, patch)
2018-03-08 08:25 UTC, Ralf Habacker
Details | Splinter Review
Fix "failed to bind socket: Cannot assign requested address" on linux with ipv6 disabled (3.20 KB, patch)
2018-03-08 08:43 UTC, Ralf Habacker
Details | Splinter Review
Let dbus-daemon be more descriptive in case of ipv6 listen errors on Windows (2.32 KB, patch)
2018-03-08 08:43 UTC, Ralf Habacker
Details | Splinter Review
[1/7] _dbus_append_address_from_socket: Make control flow clearer (3.70 KB, patch)
2018-03-08 14:44 UTC, Simon McVittie
Details | Splinter Review
[2/7] _dbus_append_address_from_socket: Correct misleading error message (967 bytes, patch)
2018-03-08 14:44 UTC, Simon McVittie
Details | Splinter Review
[3/7] _dbus_append_address_from_socket: Factor out inet_sockaddr_to_string (4.69 KB, patch)
2018-03-08 14:46 UTC, Simon McVittie
Details | Splinter Review
[4/7] _dbus_listen_tcp_socket: Set more specific IP-related errors (3.73 KB, patch)
2018-03-08 14:47 UTC, Simon McVittie
Details | Splinter Review
[5/7] _dbus_listen_tcp_socket: Treat bind() failures as non-fatal (7.56 KB, patch)
2018-03-08 14:49 UTC, Simon McVittie
Details | Splinter Review
[6/7] _dbus_connect_tcp_socket_with_nonce: Always goto out (2.56 KB, patch)
2018-03-08 14:49 UTC, Simon McVittie
Details | Splinter Review
[7/7] _dbus_connect_tcp_socket_with_nonce: Combine all connect() errors (2.70 KB, patch)
2018-03-08 14:50 UTC, Simon McVittie
Details | Splinter Review
Keep Windows implementation of _dbus_listen_tcp_socket in sync with unix (21.79 KB, patch)
2018-03-08 17:52 UTC, Ralf Habacker
Details | Splinter Review
Windows _dbus_listen_tcp_socket: Treat bind() failures as non-fatal (8.14 KB, patch)
2018-03-08 23:36 UTC, Ralf Habacker
Details | Splinter Review
Windows _dbus_connect_tcp_socket_with_nonce: Combine all connect() errors (6.14 KB, patch)
2018-03-08 23:36 UTC, Ralf Habacker
Details | Splinter Review
On Windows use the same error code for 'Unknown address family' errors in _dbus_connect_tcp_socket_with_nonce() as on unix (934 bytes, patch)
2018-03-08 23:37 UTC, Ralf Habacker
Details | Splinter Review
_dbus_get_low_level_socket_errno: Add (2.85 KB, patch)
2018-03-09 10:39 UTC, Ralf Habacker
Details | Splinter Review
Windows: Target Windows Vista or higher (1.97 KB, patch)
2018-03-09 10:39 UTC, Ralf Habacker
Details | Splinter Review
_dbus_append_address_from_socket: Factor out inet_sockaddr_to_string (6.89 KB, patch)
2018-03-09 10:40 UTC, Ralf Habacker
Details | Splinter Review
sysdeps-unix: Set more specific IP-related errors when listening (4.85 KB, patch)
2018-03-09 10:40 UTC, Ralf Habacker
Details | Splinter Review
sysdeps-unix: Treat bind() failures as non-fatal (8.55 KB, patch)
2018-03-09 10:40 UTC, Ralf Habacker
Details | Splinter Review
Unix _dbus_connect_tcp_socket_with_nonce: Always goto out (2.61 KB, patch)
2018-03-09 10:40 UTC, Ralf Habacker
Details | Splinter Review
Unix _dbus_connect_tcp_socket_with_nonce: Combine all connect() errors (2.86 KB, patch)
2018-03-09 10:40 UTC, Ralf Habacker
Details | Splinter Review
Windows _dbus_listen_tcp_socket: Treat bind() failures as non-fatal (7.51 KB, patch)
2018-03-10 13:21 UTC, Ralf Habacker
Details | Splinter Review
Windows _dbus_connect_tcp_socket_with_nonce: Combine all connect() errors (4.78 KB, patch)
2018-03-10 13:21 UTC, Ralf Habacker
Details | Splinter Review
Windows _dbus_connect_tcp_socket_with_nonce: Simplify initialization of variable noncefileStr (1.22 KB, patch)
2018-03-10 13:21 UTC, Ralf Habacker
Details | Splinter Review
Windows _dbus_connect_tcp_socket_with_nonce: Keep error code for 'Unknown address family' in sync with unix (919 bytes, patch)
2018-03-10 13:21 UTC, Ralf Habacker
Details | Splinter Review
Windows _dbus_listen_tcp_socket: Use AF_UNSPEC if family is unspecified (1.02 KB, patch)
2018-03-10 13:21 UTC, Ralf Habacker
Details | Splinter Review
Windows _dbus_listen_tcp_socket: Set socket options SO_REUSEADDR and TCP_NODELAY (1.97 KB, patch)
2018-03-10 13:21 UTC, Ralf Habacker
Details | Splinter Review
DBusSocket helpers: Declare as _DBUS_GNUC_WARN_UNUSED_RESULT (1.83 KB, patch)
2018-03-12 12:45 UTC, Simon McVittie
Details | Splinter Review
Windows _dbus_connect_tcp_socket_with_nonce: Combine all connect() errors (4.75 KB, patch)
2018-03-12 14:05 UTC, Ralf Habacker
Details | Splinter Review
Remove obsolate call to _dbus_string_free() in unix variant of _dbus_connect_tcp_socket_with_nonce (974 bytes, patch)
2018-03-12 14:29 UTC, Ralf Habacker
Details | Splinter Review
Windows _dbus_connect_tcp_socket_with_nonce: Simplify initialization of variable noncefileStr (1.29 KB, patch)
2018-03-12 14:30 UTC, Ralf Habacker
Details | Splinter Review
Windows _dbus_listen_tcp_socket: Set socket options SO_REUSEADDR and TCP_NODELAY (2.01 KB, patch)
2018-03-12 14:47 UTC, Ralf Habacker
Details | Splinter Review
Fix using uninitialized value "name" in _dbus_combine_tcp_errors (840 bytes, patch)
2018-03-20 07:06 UTC, Ralf Habacker
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Ralf Habacker 2013-03-06 21:51:53 UTC
Testing the ipv6 implementing for bug 61787 and bug 54445 with a tcp listen address with family=ipv6 

tcp:host=localhost,port=35038,family=ipv6,guid=8fdd21febd9e7e1c7844d2395137b8a5

raises the following fatal error on dbus-daemon: 

Failed to start message bus: Failed to bind socket "localhost:35038": Invalid argument
Comment 1 Ralf Habacker 2015-01-31 11:54:13 UTC
As documentated at https://bugs.freedesktop.org/show_bug.cgi?id=87999#c42 this error message indicates that the system does not support ipv6.
Comment 2 Ralf Habacker 2015-01-31 12:42:37 UTC
Created attachment 112995 [details] [review]
Detect ipv6 support and handler related error cases
Comment 3 Ralf Habacker 2015-01-31 12:44:52 UTC
_dbus_get_peer_pid_from_tcp_handle (int handle) also needs to be fixed, in which  is

    _dbus_verbose ("FIXME [61922]: IPV6 support not working on windows\n");
Comment 4 Simon McVittie 2017-11-15 13:09:32 UTC
Comment on attachment 112995 [details] [review]
Detect ipv6 support and handler related error cases

Review of attachment 112995 [details] [review]:
-----------------------------------------------------------------

Sorry for the much-delayed review here.

The commit message is rather long for a one-liner, and there's no real justification of why this extra complexity is necessary on Windows but not on Unix. It's entirely possible that Windows does need this - but if so, we should explain why, so that future maintainers won't have to reverse-engineer it.

The approach we're using here works on Unix, and Winsock is basically a copy of the BSD sockets API (much like the sockets interface in every other OS), so I'd like to see some sort of explanation (either in the commit message or a comment or both) of what is going on here.

https://chris.beams.io/posts/git-commit/ is a useful guide to commit messages.

Please check the review comments about the callers of _dbus_has_ipv6() before spending time fixing the issues in _dbus_has_ipv6() itself - in a way, the ideal solution for this would be for _dbus_has_ipv6() to not exist at all.

::: dbus/dbus-sysdeps-win.c
@@ +670,5 @@
>  
>  /**
> + * @returns #FALSE if no ipv6 support has been found
> + */
> +dbus_bool_t _dbus_has_ipv6()

static dbus_bool_t
_dbus_has_ipv6 (void)

Coding style: space before "(" throughout.

@@ +673,5 @@
> + */
> +dbus_bool_t _dbus_has_ipv6()
> +{
> +    static dbus_bool_t first = TRUE;
> +    static dbus_bool_t has_ipv6 = TRUE;

I'm concerned that this pattern isn't thread-safe: two threads could be in _dbus_has_ipv6() simultaneously, and there is no guarantee that the compiler or CPU won't reorder the write to first and the write to has_ipv6.

I think what we really want here is the equivalent of GLib's g_once_init_enter()/g_once_init_leave().

I think it would be OK if we used atomic accesses to a single variable:

/* TRUE, FALSE, or -1 if not known yet */
static long cached_result = -1;
struct addrinfo hints;
struct addrinfo *ai = NULL;
SOCKET temp = INVALID_SOCKET;
dbus_bool_t has_ipv6;

/* This is not atomic, but if we fall through to the slow path
 * in two threads in parallel, it's fairly harmless */
if (cached_result != -1)
  return cached_result;

... do the work to set has_ipv6 correctly ...

/* Atomic version of "cached_result = has_ipv6" */
InterlockedExchange (&cached_result, has_ipv6);

return has_ipv6;

@@ +679,5 @@
> +    if(first)
> +      {
> +        struct addrinfo hints;
> +        struct addrinfo *ai = 0;
> +        int temp = INVALID_SOCKET;

Shouldn't this be a SOCKET rather than an int, because SOCKET can be wider than 32 bits on 64-bit Windows?

@@ +1555,4 @@
>    _DBUS_ZERO (hints);
>  
>    if (!family)
> +    hints.ai_family = _dbus_has_ipv6() ? AF_UNSPEC : AF_INET;

I don't think this is really necessary. If AF_UNSPEC has worked until now, please keep using it. (See longer explanation below, in the listening code path)

@@ +1565,5 @@
> +      else
> +        {
> +          dbus_set_error (error,
> +                          DBUS_ERROR_INVALID_ARGS,
> +                          "No ipv6 support available", "");

If the user specifically asked for IPv6, wouldn't it be simpler to set hints.ai_family = AF_INET6, carry on anyway, and get a failure later? (Given that we need to handle later failures in any case.)

I think this will give warnings about the printf string not having a %s in it. Just remove the ', ""' part.

@@ +1715,4 @@
>    _DBUS_ZERO (hints);
>  
>    if (!family)
> +    hints.ai_family = _dbus_has_ipv6() ? AF_UNSPEC : AF_INET;

According to Bug #87999, AF_UNSPEC is not OK to use here on at least some systems (commit 91dbd2e); but I'm not entirely sure that I understand why that was necessary any more. With hindsight, I should have insisted on that commit having a comment explaining why.

If the reasoning on Bug #87999 was wrong (or the root cause of the failure was fixed later) and it is actually OK to use AF_UNSPEC on systems that have IPv6, then it should be OK everywhere. AF_UNSPEC means "accept any IP protocol". If the machine doesn't support IPv6 then AF_UNSPEC is effectively the same as AF_INET, or if the machine does support IPv6 then AF_UNSPEC means "accept both IPv4 and IPv6", making it nearly always the right thing to use.

Or if Windows doesn't have the same behaviour in this respect as the BSD sockets API, such that my reasoning is wrong and putting AF_INET in the hints breaks name-resolution on IPv4-only machines, then I think that definitely deserves a comment.

@@ +1719,5 @@
>    else if (!strcmp(family, "ipv4"))
>      hints.ai_family = AF_INET;
>    else if (!strcmp(family, "ipv6"))
> +    {
> +      if (_dbus_has_ipv6())

Same as for the connecting case: it would seem simpler to carry on trying to use AF_INET6 and let it fail later
Comment 5 Ralf Habacker 2017-12-01 23:45:44 UTC
I retested without this patch and disabled ipv6 support on several platforms:

openSUSE 42.2 Linux:

echo 1 > /proc/sys/net/ipv6/conf/lo/disable_ipv6
bin/dbus-daemon --config-file=bus/session.conf --address=tcp:port=1234,family=ipv6    --print-address

Failed to start message bus: Failed to bind socket "localhost:1234": Cannot assign requested address


Windows 7 and windows 10:

(disabled ipv6 in related network connection settings)

bin/dbus-daemon --config-file=bus/session.conf --address=tcp:port=1234,family=ipv6    --print-address
warning: Failed to start message bus: Failed to lookup host/port: "127.0.0.1:1234": Host not found (11001)

I guess the initial test has been done on Windows XP, which did not returned an explicit error code.
Comment 6 Ralf Habacker 2017-12-02 00:50:45 UTC
Created attachment 135874 [details] [review]
Fix IPV6 support in _dbus_get_peer_pid_from_tcp_handle() on Windows

Bug:
Comment 7 Ralf Habacker 2018-02-14 12:40:49 UTC
(In reply to Simon McVittie from comment #4)

> If the reasoning on Bug #87999 was wrong (or the root cause of the failure
> was fixed later) and it is actually OK to use AF_UNSPEC on systems that have
> IPv6, then it should be OK everywhere. AF_UNSPEC means "accept any IP
> protocol". If the machine doesn't support IPv6 then AF_UNSPEC is effectively
> the same as AF_INET, 

I rechecked this with recent dbus build from master on linux and did run on a system with ipv4 and ipv6 enabled:

DBUS_VERBOSE=1 bin/dbus-daemon --config-file=bus/session.conf --address tcp:host=localhost,port=60001 

netstat -alnpt | grep 60001  returns

tcp        0      0 127.0.0.1:60001         0.0.0.0:*               LISTEN      19652/dbus-daemon   
tcp        0      0 ::1:60001               :::*                    LISTEN      19652/dbus-daemon   

which means AF_UNSPEC let dbus-daemon listen on ipv4 and ipv6. 

disabling ipv6 with 

sudo su - 
echo 1 > /proc/sys/net/ipv6/conf/lo/disable_ipv6
exit

and restarting dbus-daemon with 

DBUS_VERBOSE=1 bin/dbus-daemon --config-file=bus/session.conf --address tcp:host=localhost,port=60001 

returns 

dbus-daemon[xxxx]: Failed to start message bus: Failed to bind socket "localhost:60001": Cannot assign requested address

which means the usage of AF_UNSPEC breaks dbus-daemon start if ipv6 is not enabled.

> or if the machine does support IPv6 then AF_UNSPEC
> means "accept both IPv4 and IPv6", making it nearly always the right thing
> to use.
no see above.
Comment 8 Ralf Habacker 2018-02-14 13:18:12 UTC
Created attachment 137355 [details] [review]
Fix dbus-daemon startup on Windows in case no family is specified and ipv6 is disabled

Not specifing a socket family on a tcp listen address on Windows let dbus-daemon
uses AF_UNPEC family, which results into a fatal error on trying to listen on
ipv6, regardless if ipv6 is enabled or not.

To avoid this issue dbus-daemon now checks at startup once if ipv6 is supported
and avoids to listen on ipv6 if not present.

- rebased
- fixed mentioned issues
- split of setting AF_... in calls to separate patch
Comment 9 Ralf Habacker 2018-02-14 13:18:32 UTC
Created attachment 137356 [details] [review]
Let dbus-daemon be more descriptive in case of ipv6 listen errors on Windows

In case the host does not have ipv6 support and no family is specified
in a tcp listen address dbus daemon prints out a cryptic message 'Failed
to bind socket "localhost:xxx": Invalid argument', which is changed to
'Failed to start message bus: No ipv6 support available' with this patch.
Comment 10 Ralf Habacker 2018-03-07 15:43:45 UTC
Created attachment 137863 [details] [review]
Fix msvc runtime check failure "The variable 'has_ipv6' is being used without being initialized
Comment 11 Simon McVittie 2018-03-07 18:13:22 UTC
(In reply to Ralf Habacker from comment #7)
> I rechecked this with recent dbus build from master on linux
...
> disabling ipv6 with 
> 
> sudo su - 
> echo 1 > /proc/sys/net/ipv6/conf/lo/disable_ipv6
> exit
> 
> and restarting dbus-daemon with 
> 
> DBUS_VERBOSE=1 bin/dbus-daemon --config-file=bus/session.conf --address
> tcp:host=localhost,port=60001 
> 
> returns 
> 
> dbus-daemon[xxxx]: Failed to start message bus: Failed to bind socket
> "localhost:60001": Cannot assign requested address

This is an interesting failure mode. Looking at this with "strace -e trace=%network" on Linux, the problem seems to be that if you disable IPv6 in this way, the resolver will still resolve "localhost" to both 127.0.0.1 (IPv4) and ::1 (IPv6); but then we're not actually allowed to bind to ::1.

We bind to 127.0.0.1:0 ("127.0.0.1, don't care which port") successfully, and the kernel allocates 127.0.0.1:34427 for us:

socket(AF_INET, SOCK_STREAM|SOCK_CLOEXEC, IPPROTO_IP) = 4
setsockopt(4, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
setsockopt(4, SOL_TCP, TCP_NODELAY, [1], 4) = 0
bind(4, {sa_family=AF_INET, sin_port=htons(0), sin_addr=inet_addr("127.0.0.1")}, 16) = 0
listen(4, 30)                           = 0
getsockname(4, {sa_family=AF_INET, sin_port=htons(34427), sin_addr=inet_addr("127.0.0.1")}, [128->16]) = 0

Then we `goto redo_lookup_with_port` (see https://bugs.freedesktop.org/show_bug.cgi?id=87999#c33 if, like me, you need a reminder of how this stuff is meant to work). Binding to 127.0.0.1:34427 fails with EADDRINUSE, but that's OK, we expected that:

socket(AF_INET, SOCK_STREAM|SOCK_CLOEXEC, IPPROTO_IP) = 5
setsockopt(5, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
setsockopt(5, SOL_TCP, TCP_NODELAY, [1], 4) = 0
bind(5, {sa_family=AF_INET, sin_port=htons(34427), sin_addr=inet_addr("127.0.0.1")}, 16) = -1 EADDRINUSE (Address already in use)

Binding to [::1]:34427 also fails, but this time with EADDRNOTAVAIL:

socket(AF_INET6, SOCK_STREAM|SOCK_CLOEXEC, IPPROTO_IP) = 5
setsockopt(5, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
setsockopt(5, SOL_TCP, TCP_NODELAY, [1], 4) = 0
bind(5, {sa_family=AF_INET6, sin6_port=htons(34427), inet_pton(AF_INET6, "::1", &sin6_addr), sin6_flowinfo=htonl(0), sin6_scope_id=0}, 28) = -1 EADDRNOTAVAIL (Cannot assign requested address)

and unlike EADDRINUSE, we don't cope gracefully with that.

Maybe we should - that would hopefully fix the AF_UNSPEC case.
Comment 12 Ralf Habacker 2018-03-08 08:21:33 UTC
Created attachment 137882 [details] [review]
Fix "failed to bind socket: Cannot assign requested address" on linux with ipv6 disabled
Comment 13 Ralf Habacker 2018-03-08 08:25:43 UTC
Created attachment 137883 [details] [review]
Fix dbus-daemon startup on Windows in case no family is specified and ipv6 is disable
Comment 14 Ralf Habacker 2018-03-08 08:26:40 UTC
Comment on attachment 137883 [details] [review]
Fix dbus-daemon startup on Windows in case no family is specified and ipv6 is disable

merged in attachment 137863 [details] [review]
Comment 15 Ralf Habacker 2018-03-08 08:37:49 UTC
Comment on attachment 137356 [details] [review]
Let dbus-daemon be more descriptive in case of ipv6 listen errors on Windows

Review of attachment 137356 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-sysdeps-win.c
@@ +1756,5 @@
> +          hints.ai_family = AF_INET6;
> +      else
> +        {
> +          dbus_set_error (error,
> +                          DBUS_ERROR_INVALID_ARGS,

On unix issues with address family are returned as DBUS_ERROR_BAD_ADDRESS error. It would  probably better to use also here and above
Comment 16 Ralf Habacker 2018-03-08 08:39:35 UTC
Comment on attachment 137882 [details] [review]
Fix "failed to bind socket: Cannot assign requested address" on linux with ipv6 disabled

Review of attachment 137882 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-sysdeps-unix.c
@@ +1416,5 @@
> +          hints.ai_family = AF_INET6;
> +      else
> +        {
> +          dbus_set_error (error,
> +                          DBUS_ERROR_INVALID_ARGS,

better DBUS_ERROR_BAD_ADDRESS ?

@@ +1545,5 @@
> +          hints.ai_family = AF_INET6;
> +      else
> +        {
> +          dbus_set_error (error,
> +                          DBUS_ERROR_INVALID_ARGS,

see above
Comment 17 Ralf Habacker 2018-03-08 08:43:06 UTC
Created attachment 137884 [details] [review]
Fix "failed to bind socket: Cannot assign requested address" on linux with ipv6 disabled

- use DBUS_ERROR_BAD_ADDRESS to keep in sync with other address family related error codes
Comment 18 Ralf Habacker 2018-03-08 08:43:45 UTC
Created attachment 137885 [details] [review]
Let dbus-daemon be more descriptive in case of ipv6 listen errors on Windows

- Use DBUS_ERROR_BAD_ADDRESS
Comment 19 Simon McVittie 2018-03-08 13:15:00 UTC
Comment on attachment 137884 [details] [review]
Fix "failed to bind socket: Cannot assign requested address" on linux with ipv6 disabled

Review of attachment 137884 [details] [review]:
-----------------------------------------------------------------

My high-level concern with this approach (which I think is the same approach you've been using on Windows on this bug?) is that it doesn't actually tell you whether "IPv6 is disabled", because to some extent there's no such thing. In your test instructions on Linux, you mention setting a sysctl to disable IPv6 *on the loopback interface*, lo, and what _dbus_has_ipv6() is actually telling you is whether ::1 is resolvable and a socket can be bound to ::1 - but that says nothing about whether IPv6 is enabled on interfaces other than lo. If IPv6 is disabled on the loopback interface, then we could still have working IPv6 on eth0.

I think it might be better to keep using AF_UNSPEC, and implement it more like what we do for EADDRINUSE (Bug #87999): if a hostname (typically "localhost") resolves to multiple IP addresses (typically 127.0.0.1 and ::1), and we are able to listen on at least one of them, then treat that as success; but if none of the possible IP addresses turns out to have been available, fail.

This does introduce a bit of extra complexity because ideally we'd track the individual errors for all the addresses, then if we fail to listen on *every* address that we resolved, combine the errors somehow to avoid being misleading - a bit like the way _dbus_read_local_machine_uuid() in dbus/dbus-sysdeps-unix.c reports errors, but with a variable-length array or list instead of hard-coding that there were exactly two attempts.

I started trying to write a patch for this, but found that we were already producing slightly misleading error messages, because we say something like "localhost:" (using the arguments of the function) instead of converting tmp->ai_addr to a string and using that (which would give us the address that we *actually* failed to listen on, something like "127.0.0.1:12345"). I'll try to finish that today.
Comment 20 Simon McVittie 2018-03-08 14:44:12 UTC
Created attachment 137891 [details] [review]
[1/7] _dbus_append_address_from_socket: Make control flow clearer

In the current implementation, we can reach the "err" label either
because we ran out of memory, or because we got a socket error from
getsockname() or inet_ntop(), and we blindly assume that running out
of memory will set ENOMEM.

However, that isn't actually true: when we are simulating OOM in
dbus_malloc(), the fake OOM doesn't set ENOMEM. Handle the OOM
condition explicitly instead.

In the AF_UNIX case, the break statement is no longer reached at all,
so leave a comment there. In the AF_INET and AF_INET6 cases, if
inet_ntop() fails, explicitly goto err instead of using break
(this has the same practical effect) to make it clearer that we are
going to the "socket error" code path.
Comment 21 Simon McVittie 2018-03-08 14:44:39 UTC
Created attachment 137892 [details] [review]
[2/7] _dbus_append_address_from_socket: Correct misleading error message

If what actually failed was reading the address from the socket,
we might as well say so.
Comment 22 Simon McVittie 2018-03-08 14:46:15 UTC
Created attachment 137893 [details] [review]
[3/7] _dbus_append_address_from_socket: Factor out inet_sockaddr_to_string

---

You might be able to use this in Windows too, if Windows has inet_ntop(). If so, it could be moved to dbus-sysdeps.c, with a smaller #ifdef for EAFNOSUPPORT vs. WSAEAFNOSUPPORT.
Comment 23 Simon McVittie 2018-03-08 14:47:30 UTC
Created attachment 137894 [details] [review]
[4/7] _dbus_listen_tcp_socket: Set more specific IP-related  errors

When we have resolved a hostname/port pair to a list of IPv4 or IPv6
addresses, if we are unable to listen on a a specific one of those
addresses, we should report which one.

When IPv6 is disabled for the loopback interface, this changes the
diagnostic from:

Failed to bind socket "localhost:1234": Cannot assign requested address

to the more informative

Failed to bind socket "::1" port 1234: Cannot assign requested address

---

As with Attachment #137893 [details], you might be able to use this in Windows too.
Comment 24 Simon McVittie 2018-03-08 14:49:26 UTC
Created attachment 137895 [details] [review]
[5/7] _dbus_listen_tcp_socket: Treat bind() failures as non-fatal

When we use AF_UNSPEC, we are likely to get multiple addresses back
from getaddrinfo(), and perhaps we won't be able to use them all.
Give that failure mode, or any other bind() failure, the same treatment
as EADDRINUSE failures here and all connect() failures in
_dbus_connect_tcp_socket_with_nonce(): if any address succeeds, then
the overall operation succeeds, but if all of them fail, then the
overall operation fails.

I've made combine_tcp_errors() generic enough that in principle
_dbus_connect_tcp_socket_with_nonce() could use it too, although that
isn't implemented yet.

---

This is an alternative approach to solving the same bug as Attachment #137884 [details].

combine_tcp_errors() is generic, and could be moved to dbus-sysdeps.c as _dbus_combine_tcp_errors() if the corresponding Windows code path wants it (which it probably does).
Comment 25 Simon McVittie 2018-03-08 14:49:40 UTC
Created attachment 137896 [details] [review]
[6/7] _dbus_connect_tcp_socket_with_nonce: Always goto out

This will make it easier to do single-exit-path cleanup.
Comment 26 Simon McVittie 2018-03-08 14:50:56 UTC
Created attachment 137897 [details] [review]
[7/7] _dbus_connect_tcp_socket_with_nonce: Combine all connect() errors

Previously, we took the errno from the most recent connect() error,
and used a more generic diagnostic message.

---

This and Attachment #137896 [details] are very much optional - they improve diagnostics but don't actually solve a bug.
Comment 27 Ralf Habacker 2018-03-08 15:06:23 UTC
(In reply to Simon McVittie from comment #22)

> You might be able to use this in Windows too, if Windows has inet_ntop(). 
it has, see https://msdn.microsoft.com/de-de/library/windows/desktop/cc805843(v=vs.85).aspx
Comment 28 Simon McVittie 2018-03-08 15:34:55 UTC
(In reply to Ralf Habacker from comment #27)
> (In reply to Simon McVittie from comment #22)
> 
> > You might be able to use this in Windows too, if Windows has inet_ntop(). 
> it has, see
> https://msdn.microsoft.com/de-de/library/windows/desktop/cc805843(v=vs.85).
> aspx

OK, great. If you like the approach those patches take, please try something similar on the Windows side: either write Windows-specific functions that behave a bit like my Unix-specific functions, or make my Unix-specific functions more portable, move them to dbus-sysdeps.c and share them between the implementations (whichever seems less ugly).

The branch is at https://github.com/smcv/dbus/commits/bind-error-61922 if you find that more convenient.
Comment 29 Ralf Habacker 2018-03-08 17:52:09 UTC
Created attachment 137907 [details] [review]
Keep Windows implementation of _dbus_listen_tcp_socket in sync with unix

- move inet_sockaddr_to_string, set_error_with_inet_sockaddr and combine_tcp_error
  to dbus-sysdeps.c to be usable on Windows
- prefix the mentioned function with _dbus to indicate private api
- use mentioned functions in dbus_listen_tcp_socket
Comment 30 Simon McVittie 2018-03-08 18:04:08 UTC
Comment on attachment 137907 [details] [review]
Keep Windows implementation of _dbus_listen_tcp_socket in sync with unix

Review of attachment 137907 [details] [review]:
-----------------------------------------------------------------

::: cmake/CMakeLists.txt
@@ +171,5 @@
>  endif()
>  
> +# set Windows Vista as mimimun requirement
> +if(WIN32 AND NOT WINCE)
> +    add_definitions(-D_WIN32_WINNT=0x0600)

I think bumping the minimum Windows version up to Vista should be a separate commit, affecting both CMake and Autotools, with the current _WIN32_WINNT definition removed from dbus-sysdeps-win.c - it's probably best to do it in a way that applies to absolutely everything.

For Autotools, you can get it defined in config.h with:

AC_DEFINE([_WIN32_WINNT], [0x0600], [Define to the minimum supported Windows version (0x0600 represents Vista)])

in an appropriate place - perhaps like this pseudo-patch?

     if test "$dbus_wince" = yes; then
       AC_DEFINE(DBUS_WINCE,1,[Defined if we run on a W32 CE API based system])
       AC_DEFINE(_WIN32_WCE, 0x0502, [Defined to get newer W32 CE APIs])
+    else
+      AC_DEFINE([_WIN32_WINNT], [0x0600], [Define to the minimum supported Windows version (0x0600 represents Vista)])
     fi

For CMake, you could either keep it like you've done here, or add it to config.h.cmake to be consistent with Autotools.
Comment 31 Simon McVittie 2018-03-08 18:28:27 UTC
Comment on attachment 137907 [details] [review]
Keep Windows implementation of _dbus_listen_tcp_socket in sync with unix

Review of attachment 137907 [details] [review]:
-----------------------------------------------------------------

This is looking good, just a few minor issues. It looks better to me than the approach with _dbus_have_ipv6().

I'd prefer the first line of the commit message to be the important part for fixing the bug, which is

sysdeps-win: Treat bind() failures as non-fatal 

or

Windows _dbus_listen_tcp_socket: Treat bind() failures as non-fatal 

or something like that. That's the functional change here - the rest is refactoring to support it (and could be in separate commits, or not, as you prefer).

I should perhaps rebase my patch series to use "sysdeps-unix:" as their topic prefix, to emphasize that the Windows side isn't fixed until later.

::: dbus/dbus-sysdeps-win.c
@@ +1719,4 @@
>        if (bind (fd.sock, (struct sockaddr*) tmp->ai_addr, tmp->ai_addrlen) == SOCKET_ERROR)
>          {
>            DBUS_SOCKET_SET_ERRNO ();
> +          saved_errno = errno;

If you're not using errno directly any more (and I don't think you are), it seems like it would make more sense to replace

  DBUS_SOCKET_SET_ERRNO (); /* essentially: errno = WSAGetLastError(); */
  saved_errno = errno;

with

  saved_errno = WSAGetLastError ();

which is arguably what we should have been doing in all the Windows sockets code anyway.

@@ +1729,5 @@
> +           * - If saved_errno is EADDRINUSE after we
> +           *   "goto redo_lookup_with_port" after binding a port on one of the
> +           *   possible addresses, we will try to bind that same port on
> +           *   every address, including the same address again for a second
> +           *   time, which will fail with EADDRINUSE.

WSAEADDRINUSE (twice)

@@ +1733,5 @@
> +           *   time, which will fail with EADDRINUSE.
> +           *
> +           * - If saved_errno is EADDRINUSE, it might be because binding to
> +           *   an IPv6 address implicitly binds to a corresponding IPv4
> +           *   address or vice versa (e.g. Linux with bindv6only=0).

Windows is very much not Linux :-)

(Does this case exist on Windows?)

@@ +1738,5 @@
> +           *
> +           * - If saved_errno is EADDRNOTAVAIL when we asked for family
> +           *   AF_UNSPEC, it might be because IPv6 is disabled for this
> +           *   particular interface (e.g. Linux with
> +           *   /proc/sys/net/ipv6/conf/lo/disable_ipv6).

This would be better if it was more like:

If saved_errno is WSAWHATEVER when we asked for family AF_UNSPEC, it might be because getaddrinfo() returned IPv6 addresses but IPv6
is disabled on this machine.

(With whichever error is appropriate replacing WSAWHATEVER)

@@ +1856,5 @@
>  
>    *fds_p = listen_fd;
>  
> +  /* This list might be non-empty even on success, because we might be
> +   * ignoring EADDRINUSE or EADDRNOTAVAIL */

WSAEADDRINUSE, etc.

::: dbus/dbus-sysdeps.c
@@ +795,5 @@
> +_dbus_inet_sockaddr_to_string (const void *sockaddr_pointer,
> +                         size_t len,
> +                         char *string,
> +                         size_t string_len,
> +                         const char **family_name,

Please indent continuation lines with enough spaces to match up with the first argument, or reduce their indentation to 4 spaces if you prefer that style

@@ +815,5 @@
> +
> +  switch (addr.sa.sa_family)
> +    {
> +      case AF_INET:
> +        if (inet_ntop (AF_INET, &addr.ipv4.sin_addr, string, string_len) != NULL)

Always nice to see Windows implementing standard APIs :-)

@@ +844,5 @@
> +        return FALSE;
> +#endif
> +
> +      default:
> +        errno = EAFNOSUPPORT;

Do you need

#ifdef DBUS_WIN
  _dbus_win_set_errno (WSAEAFNOSUPPORT);
#else
  errno = EAFNOSUPPORT;
#endif

or is this form OK?

I should probably have made this function raise a DBusError "properly" instead of documenting that it sets errno.

@@ +864,5 @@
> +_dbus_set_error_with_inet_sockaddr (DBusError *error,
> +                              const void *sockaddr_pointer,
> +                              socklen_t len,
> +                              const char *description,
> +                              int saved_errno)

Same indentation comment as above

@@ +891,5 @@
> +_dbus_combine_tcp_errors (DBusList **sources,
> +                    const char *summary,
> +                    const char *host,
> +                    const char *port,
> +                    DBusError *dest)

And again

::: dbus/dbus-sysdeps.h
@@ +38,5 @@
>  #include <inttypes.h>
>  #endif
>  
> +#ifdef HAVE_UNISTD_H
> +#include <unistd.h>

Hmm, is this for socklen_t? If it is, please use <sys/socket.h> instead (#ifdef DBUS_UNIX), or (perhaps better) change the function's socklen_t argument to be a size_t.

@@ +696,4 @@
>  void            _dbus_daemon_report_reloaded               (void);
>  void            _dbus_daemon_report_stopping               (void);
>  
> +DBUS_PRIVATE_EXPORT

These three functions don't need to be decorated with DBUS_PRIVATE_EXPORT (unless they're going to be used from outside libdbus, but I think that would be wrong, because higher-level components like dbus-daemon should use higher-level abstractions).
Comment 32 Simon McVittie 2018-03-08 19:24:57 UTC
(In reply to Simon McVittie from comment #30)
> I think bumping the minimum Windows version up to Vista should be a separate
> commit, affecting both CMake and Autotools, with the current _WIN32_WINNT
> definition removed from dbus-sysdeps-win.c - it's probably best to do it in
> a way that applies to absolutely everything.

I tried doing this myself in <https://github.com/smcv/dbus/commits/bind-error-61922-experimental>. Have a look and see what you think?

I'm not sure whether I prefer the -experimental version or what I attached here, and I haven't tested any of this in real Windows.

(In reply to Simon McVittie from comment #31)
> I should probably have made this function raise a DBusError "properly"
> instead of documenting that it sets errno.

I did that on my experimental branch. This needed a new function _dbus_get_low_level_socket_errno() which expands to either errno or WSAGetLastError(). If you like this version, it would be good to start changing some of the Windows sockets functions from "copy WSAGetLastError() into errno, then copy it out again to get a DBusError" to the more logical "go directly from WSAGetLastError() to a DBusError".

(Unfortunately, some of our socket interfaces rely on errno, and in particular the ability to save and restore errno - gradually converting them to DBusError would probably be good for clarity, if there isn't some reason why we can't.)

> Hmm, is this for socklen_t? If it is, please use <sys/socket.h> instead
> (#ifdef DBUS_UNIX), or (perhaps better) change the function's socklen_t
> argument to be a size_t.

I used a size_t on my experimental branch. I think that's probably the best way.
Comment 33 Simon McVittie 2018-03-08 19:32:59 UTC
(In reply to Simon McVittie from comment #31)
> I should probably have made this function raise a DBusError "properly"
> instead of documenting that it sets errno.

Thinking about this some more: if you keep the API of _dbus_inet_sockaddr_to_string() as "sets errno", then on Windows it definitely needs to copy WSAGetLastError() into errno when inet_ntop() fails. Otherwise it will be breaking the guarantee that errno is set to something useful.

So if we want _dbus_inet_sockaddr_to_string() to be shared between all platforms, we definitely have to either add _dbus_get_low_level_socket_errno(), or make DBUS_SOCKET_SET_ERRNO exist on all platforms (it would have its current definition on Windows and do nothing on Unix).

The other possibility would be to give that function (but perhaps not the other two) two near-duplicate implementations on -unix and -win, and use DBUS_SOCKET_SET_ERRNO whenever inet_ntop() fails on Windows.

It's a difficult balancing act: Winsock is close enough to the Unix socket API that a separate Windows implementation of everything seems like wasteful code duplication, but is also different enough that using a shared implementation would mean a lot of #ifdef DBUS_WIN. I'm not sure what the right balance is.
Comment 34 Ralf Habacker 2018-03-08 23:24:19 UTC
(In reply to Simon McVittie from comment #32)
> (In reply to Simon McVittie from comment #30)
> > I think bumping the minimum Windows version up to Vista should be a separate
> > commit, affecting both CMake and Autotools, with the current _WIN32_WINNT
> > definition removed from dbus-sysdeps-win.c - it's probably best to do it in
> > a way that applies to absolutely everything.
sure
> 
> I tried doing this myself in
> <https://github.com/smcv/dbus/commits/bind-error-61922-experimental>. Have a
> look and see what you think?

looks good - I'm going to refactor the windows patches to this.

> I used a size_t on my experimental branch. I think that's probably the best
> way.
yes
Comment 35 Ralf Habacker 2018-03-08 23:36:23 UTC
Created attachment 137917 [details] [review]
Windows _dbus_listen_tcp_socket: Treat bind() failures as non-fatal

Based on experimental branch
Comment 36 Ralf Habacker 2018-03-08 23:36:43 UTC
Created attachment 137918 [details] [review]
Windows _dbus_connect_tcp_socket_with_nonce: Combine all connect() errors

Previously, we took the errno from the most recent connect() error,
and used a more generic diagnostic message.

based on experimental branch
Comment 37 Ralf Habacker 2018-03-08 23:37:03 UTC
Created attachment 137919 [details] [review]
On Windows use the same error code for 'Unknown address family' errors in _dbus_connect_tcp_socket_with_nonce() as on unix

based on experimental branch
Comment 38 Ralf Habacker 2018-03-09 07:14:46 UTC
(In reply to Ralf Habacker from comment #34)
> (In reply to Simon McVittie from comment #32)
> > (In reply to Simon McVittie from comment #30)
> > > I think bumping the minimum Windows version up to Vista should be a separate
> > > commit, affecting both CMake and Autotools, with the current _WIN32_WINNT
> > > definition removed from dbus-sysdeps-win.c - it's probably best to do it in
> > > a way that applies to absolutely everything.
> sure
> > 
> > I tried doing this myself in
> > <https://github.com/smcv/dbus/commits/bind-error-61922-experimental>. Have a
> > look and see what you think?
> 
> looks good - I'm going to refactor the windows patches to this.
> 
> > I used a size_t on my experimental branch. I think that's probably the best
> > way.
> yes

Comparing both branches it turns out that on the experimental branch these commits are the same as compared to the other branch 

1df2d70d _dbus_append_address_from_socket: Make control flow clearer
6ffc6d1f _dbus_append_address_from_socket: Correct misleading error message

these are new and provides the required api adjustment 

425d2a5f _dbus_get_low_level_socket_errno: Add
8d22386f Windows: Target Windows Vista or higher

and these are rebased including having tcp helper functions located in dbus-sysdeps.c, which is definitly good

61f04a7b _dbus_append_address_from_socket: Factor out inet_sockaddr_to_string
8d78ebcb sysdeps-unix: Set more specific IP-related errors when listening
e5a48a7c sysdeps-unix: Treat bind() failures as non-fatal
9f867ead Unix _dbus_connect_tcp_socket_with_nonce: Always goto out
dfd025a8 Unix _dbus_connect_tcp_socket_with_nonce: Combine all connect() errors

attachment 137917 [details] [review]  and 137918 are on top of these commits. 

Please add/update the related commits to this bug for reviewing
Comment 39 Ralf Habacker 2018-03-09 10:39:36 UTC
Created attachment 137927 [details] [review]
_dbus_get_low_level_socket_errno: Add
Comment 40 Ralf Habacker 2018-03-09 10:39:50 UTC
Created attachment 137928 [details] [review]
Windows: Target Windows Vista or higher

This will give us the RFC-2553 inet_ntop() interface.
Windows Vista extended security support ended in 2017, but we don't
actually need anything from versions newer than Vista yet.

Loosely based on part of a patch by Ralf Habacker.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 41 Ralf Habacker 2018-03-09 10:40:00 UTC
Created attachment 137929 [details] [review]
_dbus_append_address_from_socket: Factor out inet_sockaddr_to_string

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 42 Ralf Habacker 2018-03-09 10:40:06 UTC
Created attachment 137930 [details] [review]
sysdeps-unix: Set more specific IP-related errors when listening

When we have resolved a hostname/port pair to a list of IPv4 or IPv6
addresses, if we are unable to listen on a a specific one of those
addresses, we should report which one.

When IPv6 is disabled for the loopback interface, this changes the
diagnostic from:

Failed to bind socket "localhost:1234": Cannot assign requested address

to the more informative

Failed to bind socket "::1" port 1234: Cannot assign requested address

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 43 Ralf Habacker 2018-03-09 10:40:22 UTC
Created attachment 137931 [details] [review]
sysdeps-unix: Treat bind() failures as non-fatal

When we use AF_UNSPEC, we are likely to get multiple addresses back
from getaddrinfo(), and perhaps we won't be able to use them all.
Give that failure mode, or any other bind() failure, the same treatment
as EADDRINUSE failures here and all connect() failures in
_dbus_connect_tcp_socket_with_nonce(): if any address succeeds, then
the overall operation succeeds, but if all of them fail, then the
overall operation fails.

I've made combine_tcp_errors() generic enough that in principle
_dbus_connect_tcp_socket_with_nonce() could use it too, although that
isn't implemented yet.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 44 Ralf Habacker 2018-03-09 10:40:37 UTC
Created attachment 137932 [details] [review]
Unix _dbus_connect_tcp_socket_with_nonce: Always goto out

This will make it easier to do single-exit-path cleanup.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 45 Ralf Habacker 2018-03-09 10:40:47 UTC
Created attachment 137933 [details] [review]
Unix _dbus_connect_tcp_socket_with_nonce: Combine all connect() errors

Previously, we took the errno from the most recent connect() error,
and used a more generic diagnostic message.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 46 Ralf Habacker 2018-03-09 10:44:12 UTC
Comment on attachment 137891 [details] [review]
[1/7] _dbus_append_address_from_socket: Make control flow clearer

Review of attachment 137891 [details] [review]:
-----------------------------------------------------------------

looks good
Comment 47 Ralf Habacker 2018-03-09 10:44:37 UTC
Comment on attachment 137892 [details] [review]
[2/7] _dbus_append_address_from_socket: Correct misleading error message

Review of attachment 137892 [details] [review]:
-----------------------------------------------------------------

looks good
Comment 48 Ralf Habacker 2018-03-09 10:44:58 UTC
Comment on attachment 137927 [details] [review]
_dbus_get_low_level_socket_errno: Add

Review of attachment 137927 [details] [review]:
-----------------------------------------------------------------

looks good
Comment 49 Ralf Habacker 2018-03-09 10:46:06 UTC
Comment on attachment 137928 [details] [review]
Windows: Target Windows Vista or higher

Review of attachment 137928 [details] [review]:
-----------------------------------------------------------------

looks good
Comment 50 Ralf Habacker 2018-03-09 10:47:48 UTC
Comment on attachment 137929 [details] [review]
_dbus_append_address_from_socket: Factor out inet_sockaddr_to_string

Review of attachment 137929 [details] [review]:
-----------------------------------------------------------------

looks good
Comment 51 Ralf Habacker 2018-03-09 10:49:07 UTC
Comment on attachment 137930 [details] [review]
sysdeps-unix: Set more specific IP-related errors when listening

Review of attachment 137930 [details] [review]:
-----------------------------------------------------------------

looks good
Comment 52 Ralf Habacker 2018-03-09 10:51:58 UTC
Comment on attachment 137931 [details] [review]
sysdeps-unix: Treat bind() failures as non-fatal

Review of attachment 137931 [details] [review]:
-----------------------------------------------------------------

> combine_tcp_errors() 
This is now named _dbus_combine_tcp_errors()

otherwise looks good
Comment 53 Ralf Habacker 2018-03-09 10:57:14 UTC
Comment on attachment 137932 [details] [review]
Unix _dbus_connect_tcp_socket_with_nonce: Always goto out

Review of attachment 137932 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-sysdeps-unix.c
@@ +1421,5 @@
>        dbus_set_error (error,
>                        _dbus_error_from_errno (saved_errno),
>                        "Failed to connect to socket \"%s:%s\" %s",
>                        host, port, _dbus_strerror(saved_errno));
> +      goto out;

looks good  - dbus socket is already invalid here so there is not need to call dbus_socket_invalidate ()

@@ +1452,1 @@
>    return fd;

Without this patch the function returns in error case with the result of the call to _dbus_socket_get_invalid (). Is it made sure, that this is now also the case ?
Comment 54 Ralf Habacker 2018-03-09 10:58:15 UTC
Comment on attachment 137933 [details] [review]
Unix _dbus_connect_tcp_socket_with_nonce: Combine all connect() errors

Review of attachment 137933 [details] [review]:
-----------------------------------------------------------------

looks good
Comment 55 Ralf Habacker 2018-03-09 11:19:46 UTC
Some test results on Windows:

1. starting two dbus-daemon using address - "tcp:host=localhost,port=12345,family=ipv6"

The second one prints out: dbus-daemon[4468]: warning: Failed to start message bus: Failed to bind socket "::1" port 12345: Address already in use

2. starting two dbus-daemon using address - "tcp:host=localhost,port=12345,family=ipv4"

The second one prints out: dbus-daemon[3160]: warning: Failed to start message bus: Failed to bind socket "127.0.0.1" port 12345: Address already in use

3. starting two dbus-daemon using address "tcp:host=localhost,port=12345"

The second one prints out:  dbus-daemon[5784]: warning: Failed to start message bus: Failed to bind to "localhost":12345 (Failed to bind socket "::1" port 12345: Address already in use; Failed to bind socket "127.0.0.1" port 12345: Address already in use)

2. starting a dbus-daemon using address "tcp:host=localhost,port=12345,family=ipv4" and a second one with "tcp:host=localhost,port=12345"

The second daemon starts and adding --print-address to the command line dumps out "tcp:host=localhost,port=12345,guid=db45fd121f57825bc970205d5aa26aab" -> the address do not contains the family and requires that libdbus knows that it should contain to ipv6 first.
Comment 56 Simon McVittie 2018-03-09 12:16:45 UTC
(In reply to Ralf Habacker from comment #55)
> Some test results on Windows:
> 
> 1. starting two dbus-daemon using address -
> "tcp:host=localhost,port=12345,family=ipv6"
> 
> The second one prints out: dbus-daemon[4468]: warning: Failed to start
> message bus: Failed to bind socket "::1" port 12345: Address already in use

This seems good.

> 2. starting two dbus-daemon using address -
> "tcp:host=localhost,port=12345,family=ipv4"
> 
> The second one prints out: dbus-daemon[3160]: warning: Failed to start
> message bus: Failed to bind socket "127.0.0.1" port 12345: Address already
> in use

This seems good.

> 3. starting two dbus-daemon using address "tcp:host=localhost,port=12345"
> 
> The second one prints out:  dbus-daemon[5784]: warning: Failed to start
> message bus: Failed to bind to "localhost":12345 (Failed to bind socket
> "::1" port 12345: Address already in use; Failed to bind socket "127.0.0.1"
> port 12345: Address already in use)

This seems good. Now we're reporting a more specific (if slightly repetitive) error.

> 2. starting a dbus-daemon using address
> "tcp:host=localhost,port=12345,family=ipv4" and a second one with
> "tcp:host=localhost,port=12345"
> 
> The second daemon starts and adding --print-address to the command line
> dumps out
> "tcp:host=localhost,port=12345,guid=db45fd121f57825bc970205d5aa26aab" -> the
> address do not contains the family and requires that libdbus knows that it
> should contain to ipv6 first.

This isn't ideal behaviour, but I'm not sure what the ideal behaviour would be. Should the second dbus-daemon fail to start because the address it wanted to listen on is "partially occupied" (I think this would probably be really annoying, or even impossible, to implement without reintroducing Bug #87999), or should _dbus_server_new_for_tcp_socket() have a special case to add family= to the address if it's called with family == NULL but all the sockets it gets back from _dbus_listen_tcp_socket() are the same family (which would produce family=ipv6 here), or what?

I think the behaviour as implemented here is tolerable, because it's a really weird corner case anyway: now that AF_UNSPEC is working correctly, I don't really see a good reason to specify family= in listening addresses.
Comment 57 Simon McVittie 2018-03-09 12:23:37 UTC
(In reply to Ralf Habacker from comment #53)
> Comment on attachment 137932 [details] [review]
> Unix _dbus_connect_tcp_socket_with_nonce: Always goto out
> 
> @@ +1452,1 @@
> >    return fd;
> 
> Without this patch the function returns in error case with the result of the
> call to _dbus_socket_get_invalid (). Is it made sure, that this is now also
> the case ?

That was my intention, which is why I added all the _dbus_socket_invalidate() calls. Please check the implementation after this patch is applied to make sure I've got it right?

You should be able to see that every time we "goto out", either there is a _dbus_socket_invalidate() call immediately before, or we are doing the "goto out" statement *because* the socket is already invalid (which means { .fd = -1 } on Unix) so there is no need to call _dbus_socket_invalidate() again.

(In fact, I think it's probably enough to check the diff rather than the patched code, because there can't have been any valid "goto out" calls before the patch that added the "out" label, which is this one.)
Comment 58 Simon McVittie 2018-03-09 12:29:07 UTC
Comment on attachment 137891 [details] [review]
[1/7] _dbus_append_address_from_socket: Make control flow clearer

Applied, thanks
Comment 59 Simon McVittie 2018-03-09 12:29:21 UTC
Comment on attachment 137892 [details] [review]
[2/7] _dbus_append_address_from_socket: Correct misleading error message

Applied, thanks
Comment 60 Simon McVittie 2018-03-09 12:29:35 UTC
Comment on attachment 137927 [details] [review]
_dbus_get_low_level_socket_errno: Add

Applied, thanks
Comment 61 Simon McVittie 2018-03-09 12:29:49 UTC
Comment on attachment 137928 [details] [review]
Windows: Target Windows Vista or higher

Applied, thanks
Comment 62 Simon McVittie 2018-03-09 12:30:01 UTC
Comment on attachment 137929 [details] [review]
_dbus_append_address_from_socket: Factor out inet_sockaddr_to_string

Applied, thanks
Comment 63 Simon McVittie 2018-03-09 12:30:18 UTC
Comment on attachment 137930 [details] [review]
sysdeps-unix: Set more specific IP-related errors when listening

Applied, thanks
Comment 64 Simon McVittie 2018-03-09 12:30:32 UTC
Comment on attachment 137931 [details] [review]
sysdeps-unix: Treat bind() failures as non-fatal

Applied, thanks
Comment 65 Simon McVittie 2018-03-09 12:47:24 UTC
Comment on attachment 137917 [details] [review]
Windows _dbus_listen_tcp_socket: Treat bind() failures as non-fatal

Review of attachment 137917 [details] [review]:
-----------------------------------------------------------------

Looks good, just some minor comments

::: dbus/dbus-sysdeps-win.c
@@ +1670,4 @@
>    _DBUS_ZERO (hints);
>  
>    if (!family)
> +    hints.ai_family = AF_UNSPEC;

I'd slightly prefer this to be a separate commit after this one, with a commit message something like this:

"""
Windows _dbus_listen_tcp_socket: Use AF_UNSPEC if family is unspecified

Now that we cope with getting WSAEADDRNOTAVAIL when trying to listen on ::1 with IPv6 disabled, and gracefully fall back to only listening on 127.0.0.1, we can use AF_UNSPEC like the corresponding Unix code does.
"""

Or if you'd prefer to keep it as one commit, please mention why we can now change back to AF_UNSPEC in the long commit message.

Justification: Not using AF_UNSPEC here was a workaround for a bug - essentially the same bug that I fixed on the Unix side with Attachment #137931 [details]. The rest of this commit is a proper fix for that bug - basically Attachment #137931 [details], but for Windows. When we've fixed that bug properly, we can remove the workaround and use AF_UNSPEC again.

@@ +1714,2 @@
>                           "Failed to open socket: %s",
> +                         _dbus_strerror (saved_errno));

Nice! Yes, this is how I hoped you'd be able to use _dbus_get_low_level_socket_errno(). (Or since this is Windows-specific code anyway, you could inline the call to WSAGetLastError(), whichever you think is clearer - generic sockets code in dbus-sysdeps.c is the only place where we *have to* use _dbus_get_low_level_socket_errno().)

If you want to clean up sysdeps-win a bit more by doing the equivalent of this in other places, I'd be happy to review separate commits. Maybe one day DBUS_SOCKET_SET_ERRNO() can go away completely.

@@ +1716,5 @@
>            goto failed;
>          }
>        _DBUS_ASSERT_ERROR_IS_CLEAR(error);
>  
> +      /* TODO: set socket options similar to unux */

"similar to Unix"

This is referring to SO_REUSEADDR and TCP_NODELAY, right? Yes, those two would be good to implement in separate commits, and would probably be quite simple.

::: dbus/dbus-sysdeps.c
@@ +49,4 @@
>  
>  #ifdef DBUS_WIN
>    #include <stdlib.h>
> +  #include <winsock2.h>

I think Attachment #137929 [details] should have made this change unnecessary.
Comment 66 Simon McVittie 2018-03-09 12:52:57 UTC
Comment on attachment 137918 [details] [review]
Windows _dbus_connect_tcp_socket_with_nonce: Combine all connect() errors

Review of attachment 137918 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-sysdeps-win.c
@@ +1544,5 @@
>        dbus_set_error (error,
>                        _dbus_error_from_errno (res),
>                        "Failed to lookup host/port: \"%s:%s\": %s (%d)",
> +                      host, port, _dbus_strerror (res), res);
> +      _dbus_socket_get_invalid ();

This call doesn't do anything useful. Did you mean _dbus_socket_invalidate (&fd)?

Strictly speaking it's unnecessary here because fd is still set to DBUS_SOCKET_INIT anyway, but when doing the Unix side I thought it was more obviously correct if I included this unnecessary call to _dbus_socket_invalidate(), and the same reasoning works equally well on the Windows side.

@@ +1611,5 @@
>    if (noncefile != NULL)
>      {
>        DBusString noncefileStr;
>        dbus_bool_t ret;
> +      _dbus_string_init_const (&noncefileStr, noncefile);

This optimization should ideally be a separate commit, but it does seem correct.

@@ +1737,4 @@
>          {
>            saved_errno = _dbus_get_low_level_socket_errno ();
>            dbus_set_error (error,
> +                          _dbus_error_from_errno (saved_errno),

Sorry, I should have spotted this while reviewing the previous commit. This should be part of the previous commit instead, otherwise it'll fail to compile.

@@ +1835,4 @@
>                                      NI_NUMERICSERV)) != 0)
>                  {
>                    saved_errno = _dbus_get_low_level_socket_errno ();
> +                  dbus_set_error (error, _dbus_error_from_errno (saved_errno),

Same
Comment 67 Simon McVittie 2018-03-09 12:53:25 UTC
Comment on attachment 137919 [details] [review]
On Windows use the same error code for 'Unknown address family' errors in _dbus_connect_tcp_socket_with_nonce() as on unix

Review of attachment 137919 [details] [review]:
-----------------------------------------------------------------

Go ahead
Comment 68 Simon McVittie 2018-03-09 12:55:33 UTC
(In reply to Ralf Habacker from comment #53)
> Comment on attachment 137932 [details] [review]
> Unix _dbus_connect_tcp_socket_with_nonce: Always goto out

I won't apply this one immediately to give you a chance to check my reasoning, but if you're happy with what I said in Comment #57, it's ready to apply.

(In reply to Ralf Habacker from comment #54)
> Comment on attachment 137933 [details] [review]
> 
> looks good

I haven't applied this one yet because it requires Attachment #137932 [details].
Comment 69 Ralf Habacker 2018-03-10 10:15:35 UTC
Comment on attachment 137932 [details] [review]
Unix _dbus_connect_tcp_socket_with_nonce: Always goto out

Review of attachment 137932 [details] [review]:
-----------------------------------------------------------------

looks good

::: dbus/dbus-sysdeps-unix.c
@@ +1452,1 @@
>    return fd;

after looking into the source I saw that fd is of type DBusSocket, _dbus_socket_invalidate() gets a DBusSocket *type parameter and the return type is also of type DBusSocket.
Comment 70 Ralf Habacker 2018-03-10 11:16:58 UTC
Comment on attachment 137932 [details] [review]
Unix _dbus_connect_tcp_socket_with_nonce: Always goto out

applied
Comment 71 Ralf Habacker 2018-03-10 11:17:14 UTC
Comment on attachment 137933 [details] [review]
Unix _dbus_connect_tcp_socket_with_nonce: Combine all connect() errors

applied
Comment 72 Ralf Habacker 2018-03-10 13:21:27 UTC
Created attachment 137962 [details] [review]
Windows _dbus_listen_tcp_socket: Treat bind() failures as non-fatal

Bug:
Comment 73 Ralf Habacker 2018-03-10 13:21:30 UTC
Created attachment 137963 [details] [review]
Windows _dbus_connect_tcp_socket_with_nonce: Combine all connect() errors

Previously, we took the errno from the most recent connect() error,
and used a more generic diagnostic message.

Bug:
Comment 74 Ralf Habacker 2018-03-10 13:21:34 UTC
Created attachment 137964 [details] [review]
Windows _dbus_connect_tcp_socket_with_nonce: Simplify initialization of variable noncefileStr

This optimization has been detected while comparing the related unix
implemention.

Bug:
Comment 75 Ralf Habacker 2018-03-10 13:21:38 UTC
Created attachment 137965 [details] [review]
Windows _dbus_connect_tcp_socket_with_nonce: Keep error code for 'Unknown address family' in sync with unix

Bug:
Comment 76 Ralf Habacker 2018-03-10 13:21:42 UTC
Created attachment 137966 [details] [review]
Windows _dbus_listen_tcp_socket: Use AF_UNSPEC if family is unspecified

Now that we cope with getting WSAEADDRNOTAVAIL when trying to listen on
::1 with IPv6 disabled, and gracefully fall back to only listening on
127.0.0.1, we can use AF_UNSPEC like the corresponding Unix code does.

Bug:
Comment 77 Ralf Habacker 2018-03-10 13:21:46 UTC
Created attachment 137967 [details] [review]
Windows _dbus_listen_tcp_socket: Set socket options SO_REUSEADDR and TCP_NODELAY

A difference to the related unix implementation is that the prototype of
setsockopt() on Windows defines socket option value type as 'const char'.

Bug:
Comment 78 Ralf Habacker 2018-03-10 13:22:42 UTC
Comment on attachment 137919 [details] [review]
On Windows use the same error code for 'Unknown address family' errors in _dbus_connect_tcp_socket_with_nonce() as on unix

superseeded by attachment 137965 [details] [review]
Comment 79 Ralf Habacker 2018-03-10 13:28:19 UTC
(In reply to Simon McVittie from comment #65)
> Comment on attachment 137917 [details] [review] [review]
> Windows _dbus_listen_tcp_socket: Treat bind() failures as non-fatal
> 
> Review of attachment 137917 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Looks good, just some minor comments
> 
> ::: dbus/dbus-sysdeps-win.c
> @@ +1670,4 @@
> >    _DBUS_ZERO (hints);
> >  
> >    if (!family)
> > +    hints.ai_family = AF_UNSPEC;
> 
> I'd slightly prefer this to be a separate commit after this one, with a
> commit message something like this:
fixed

> (Or since this is Windows-specific code
> anyway, you could inline the call to WSAGetLastError(), whichever you think
> is clearer - generic sockets code in dbus-sysdeps.c is the only place where
> we *have to* use _dbus_get_low_level_socket_errno().)
I use _dbus_get_low_level_socket_errno() for now. This could be refactored along with the mentioned sysdeps-win cleanup. But I need to have some other things before
> 
> If you want to clean up sysdeps-win a bit more by doing the equivalent of
> this in other places, I'd be happy to review separate commits. Maybe one day
> DBUS_SOCKET_SET_ERRNO() can go away completely.

> 
> @@ +1716,5 @@
> >            goto failed;
> >          }
> >        _DBUS_ASSERT_ERROR_IS_CLEAR(error);
> >  
> > +      /* TODO: set socket options similar to unux */
> 
> "similar to Unix"
> This is referring to SO_REUSEADDR and TCP_NODELAY, right? Yes, those two
> would be good to implement in separate commits, and would probably be quite
> simple.
fxed 

> ::: dbus/dbus-sysdeps.c
> @@ +49,4 @@
> >  
> >  #ifdef DBUS_WIN
> >    #include <stdlib.h>
> > +  #include <winsock2.h>
> 
> I think Attachment #137929 [details] should have made this change
> unnecessary.
yes
Comment 80 Ralf Habacker 2018-03-10 13:29:32 UTC
(In reply to Simon McVittie from comment #67)
> Comment on attachment 137919 [details] [review] [review]
> On Windows use the same error code for 'Unknown address family' errors in
> _dbus_connect_tcp_socket_with_nonce() as on unix
> 
> Review of attachment 137919 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Go ahead

need to be applied after the other
Comment 81 Simon McVittie 2018-03-12 12:05:07 UTC
Comment on attachment 137962 [details] [review]
Windows _dbus_listen_tcp_socket: Treat bind() failures as non-fatal

Review of attachment 137962 [details] [review]:
-----------------------------------------------------------------

Looks good
Comment 82 Simon McVittie 2018-03-12 12:09:35 UTC
Comment on attachment 137963 [details] [review]
Windows _dbus_connect_tcp_socket_with_nonce: Combine all connect() errors

Review of attachment 137963 [details] [review]:
-----------------------------------------------------------------

Looks good apart from the useless call to _dbus_socket_get_invalid().

::: dbus/dbus-sysdeps-win.c
@@ +1544,5 @@
>        dbus_set_error (error,
>                        _dbus_error_from_errno (res),
>                        "Failed to lookup host/port: \"%s:%s\": %s (%d)",
> +                      host, port, _dbus_strerror (res), res);
> +      _dbus_socket_get_invalid ();

You are still calling _dbus_socket_get_invalid() for its side-effects, but it doesn't have any side-effects. The only thing that function does is to return a DBusSocket that is initialized to { INVALID_SOCKET } on Windows or { -1 } on Unix - but you're ignoring its return value, so there's no point in calling it at all.

You have two choices here: either explicitly clear fd to { INVALID_SOCKET } (preferably by calling _dbus_socket_invalidate (&fd)), or rely on the fact that at this point in the function, fd already contains { INVALID_SOCKET } because we initialized it to DBUS_SOCKET_INIT and didn't change it. Either way, don't call _dbus_socket_get_invalid().
Comment 83 Simon McVittie 2018-03-12 12:12:10 UTC
Comment on attachment 137964 [details] [review]
Windows _dbus_connect_tcp_socket_with_nonce: Simplify initialization of variable noncefileStr

Review of attachment 137964 [details] [review]:
-----------------------------------------------------------------

Looks good

::: dbus/dbus-sysdeps-win.c
@@ +1616,3 @@
>        ret = _dbus_send_nonce (fd, &noncefileStr, error);
>  
>        _dbus_string_free (&noncefileStr);

You can delete this _dbus_string_free() call, too, if you want (and the corresponding one in the Unix side). A DBusString that was initialized with a constant doesn't hold any allocated memory, so it doesn't need to be freed.
Comment 84 Simon McVittie 2018-03-12 12:12:26 UTC
Comment on attachment 137965 [details] [review]
Windows _dbus_connect_tcp_socket_with_nonce: Keep error code for 'Unknown address family' in sync with unix

Review of attachment 137965 [details] [review]:
-----------------------------------------------------------------

Looks good
Comment 85 Simon McVittie 2018-03-12 12:12:57 UTC
Comment on attachment 137966 [details] [review]
Windows _dbus_listen_tcp_socket: Use AF_UNSPEC if family is unspecified

Review of attachment 137966 [details] [review]:
-----------------------------------------------------------------

Looks good, thanks for splitting this - I think that'll make future maintainers' lives easier.
Comment 86 Simon McVittie 2018-03-12 12:22:02 UTC
Comment on attachment 137967 [details] [review]
Windows _dbus_listen_tcp_socket: Set socket options SO_REUSEADDR and TCP_NODELAY

Review of attachment 137967 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-sysdeps-win.c
@@ +1733,4 @@
>    tmp = ai;
>    while (tmp)
>      {
> +      const char reuseaddr = 1, tcp_nodelay_on = 1;

I don't think this is right, despite the "const char *" prototype. https://msdn.microsoft.com/en-us/library/windows/desktop/ms740476(v=vs.85).aspx says:

"""
There are two types of socket options: Boolean options that enable or disable a feature or behavior, and options that require an integer value or structure. To enable a Boolean option, the optval parameter points to a nonzero integer. To disable the option optval points to an integer equal to zero. **The optlen parameter should be equal to sizeof(int) for Boolean options.**
"""

(my emphasis)

So I think you need to use an int and a cast, like this:

int yes = 1;

setsockopt (&fd.sock, SOL_SOCKET, SO_REUSEADDR, (const char *) &yes, sizeof (yes))

(It might as well be the same int for both options, since you set them both to 1 anyway.)

@@ +1749,5 @@
>  
> +      if (setsockopt (fd.sock, SOL_SOCKET, SO_REUSEADDR, &reuseaddr, sizeof(reuseaddr))==-1)
> +        {
> +          _dbus_warn ("Failed to set socket option \"%s:%s\": %s",
> +                      host ? host : "*", port, _dbus_strerror (errno));

Winsock functions like setsockopt() are documented to set WSAGetLastError(), not errno. errno won't be set unless you set it yourself by using DBUS_SOCKET_SET_ERRNO(), which I think we should phase out.

They're also documented to return SOCKET_ERROR, not -1, although hopefully SOCKET_ERROR is numerically equal to -1 in practice because anything else would be silly.

So I think you're meant to use:

if (setsockopt (...) == SOCKET_ERROR)
  {
    saved_errno = WSAGetLastError ();
    _dbus_warn (..., _dbus_strerror (saved_errno));
  }

(or replace WSAGetLastError() with _dbus_get_low_level_socket_errno() if you think that's clearer).
Comment 87 Simon McVittie 2018-03-12 12:24:27 UTC
(In reply to Simon McVittie from comment #82)
> You are still calling _dbus_socket_get_invalid() for its side-effects, but
> it doesn't have any side-effects.

Perhaps we should declare it _DBUS_GNUC_WARN_UNUSED_RESULT.
Comment 88 Simon McVittie 2018-03-12 12:45:10 UTC
Created attachment 138019 [details] [review]
DBusSocket helpers: Declare as _DBUS_GNUC_WARN_UNUSED_RESULT

There is no point in calling these functions for their side-effects,
because they don't have any.

---

I think this would have produced a compiler warning for the useless call that I noticed while reviewing Attachment #137963 [details].
Comment 89 Ralf Habacker 2018-03-12 14:02:58 UTC
Comment on attachment 137962 [details] [review]
Windows _dbus_listen_tcp_socket: Treat bind() failures as non-fatal

Thanks for reviewing, applied to master
Comment 90 Ralf Habacker 2018-03-12 14:05:33 UTC
Created attachment 138023 [details] [review]
Windows _dbus_connect_tcp_socket_with_nonce: Combine all connect() errors

- remove call to dbus_socket_get_invalid()
Comment 91 Ralf Habacker 2018-03-12 14:29:37 UTC
Created attachment 138024 [details] [review]
Remove obsolate call to _dbus_string_free() in unix variant of _dbus_connect_tcp_socket_with_nonce

A DBusString that was initialized with a constant doesn't hold any
allocated memory, so it doesn't need to be freed.
Comment 92 Ralf Habacker 2018-03-12 14:30:40 UTC
Created attachment 138025 [details] [review]
Windows _dbus_connect_tcp_socket_with_nonce: Simplify initialization of variable noncefileStr

- remove obsolate call to dbus_string_free
Comment 93 Simon McVittie 2018-03-12 14:33:46 UTC
Comment on attachment 138023 [details] [review]
Windows _dbus_connect_tcp_socket_with_nonce: Combine all connect() errors

Review of attachment 138023 [details] [review]:
-----------------------------------------------------------------

Go ahead
Comment 94 Simon McVittie 2018-03-12 14:34:25 UTC
Comment on attachment 138024 [details] [review]
Remove obsolate call to _dbus_string_free() in unix variant of _dbus_connect_tcp_socket_with_nonce

Review of attachment 138024 [details] [review]:
-----------------------------------------------------------------

It's spelled "obsolete", not "obsolate". Fine to apply with that fix
Comment 95 Simon McVittie 2018-03-12 14:34:40 UTC
Comment on attachment 138025 [details] [review]
Windows _dbus_connect_tcp_socket_with_nonce: Simplify initialization of variable noncefileStr

Review of attachment 138025 [details] [review]:
-----------------------------------------------------------------

Looks good
Comment 96 Ralf Habacker 2018-03-12 14:47:53 UTC
Created attachment 138026 [details] [review]
Windows _dbus_listen_tcp_socket: Set socket options SO_REUSEADDR and TCP_NODELAY

- fix variable type
- fix errno usage
Comment 97 Simon McVittie 2018-03-12 15:02:31 UTC
Comment on attachment 138026 [details] [review]
Windows _dbus_listen_tcp_socket: Set socket options SO_REUSEADDR and TCP_NODELAY

Review of attachment 138026 [details] [review]:
-----------------------------------------------------------------

Looks good
Comment 98 Ralf Habacker 2018-03-12 19:35:23 UTC
remaining patch applied
Comment 99 Ralf Habacker 2018-03-20 07:06:24 UTC
Created attachment 138212 [details] [review]
Fix using uninitialized value "name" in _dbus_combine_tcp_errors

Coverity CID 265359.

Bug:
Comment 100 Simon McVittie 2018-03-20 12:19:01 UTC
Comment on attachment 138212 [details] [review]
Fix using uninitialized value "name" in _dbus_combine_tcp_errors

Review of attachment 138212 [details] [review]:
-----------------------------------------------------------------

Thanks, please apply.
Comment 101 Ralf Habacker 2018-03-20 12:31:29 UTC
- last patch applied to master


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.