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
As documentated at https://bugs.freedesktop.org/show_bug.cgi?id=87999#c42 this error message indicates that the system does not support ipv6.
Created attachment 112995 [details] [review] Detect ipv6 support and handler related error cases
_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 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
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.
Created attachment 135874 [details] [review] Fix IPV6 support in _dbus_get_peer_pid_from_tcp_handle() on Windows Bug:
(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.
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
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.
Created attachment 137863 [details] [review] Fix msvc runtime check failure "The variable 'has_ipv6' is being used without being initialized
(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.
Created attachment 137882 [details] [review] Fix "failed to bind socket: Cannot assign requested address" on linux with ipv6 disabled
Created attachment 137883 [details] [review] Fix dbus-daemon startup on Windows in case no family is specified and ipv6 is disable
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 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 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
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
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 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.
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.
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.
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.
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.
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).
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.
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.
(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
(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.
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 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 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).
(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.
(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.
(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
Created attachment 137917 [details] [review] Windows _dbus_listen_tcp_socket: Treat bind() failures as non-fatal Based on experimental branch
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
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
(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
Created attachment 137927 [details] [review] _dbus_get_low_level_socket_errno: Add
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>
Created attachment 137929 [details] [review] _dbus_append_address_from_socket: Factor out inet_sockaddr_to_string Signed-off-by: Simon McVittie <smcv@collabora.com>
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>
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>
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>
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 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 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 on attachment 137927 [details] [review] _dbus_get_low_level_socket_errno: Add Review of attachment 137927 [details] [review]: ----------------------------------------------------------------- looks good
Comment on attachment 137928 [details] [review] Windows: Target Windows Vista or higher Review of attachment 137928 [details] [review]: ----------------------------------------------------------------- looks good
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 on attachment 137930 [details] [review] sysdeps-unix: Set more specific IP-related errors when listening Review of attachment 137930 [details] [review]: ----------------------------------------------------------------- looks good
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 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 on attachment 137933 [details] [review] Unix _dbus_connect_tcp_socket_with_nonce: Combine all connect() errors Review of attachment 137933 [details] [review]: ----------------------------------------------------------------- looks good
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.
(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.
(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 on attachment 137891 [details] [review] [1/7] _dbus_append_address_from_socket: Make control flow clearer Applied, thanks
Comment on attachment 137892 [details] [review] [2/7] _dbus_append_address_from_socket: Correct misleading error message Applied, thanks
Comment on attachment 137927 [details] [review] _dbus_get_low_level_socket_errno: Add Applied, thanks
Comment on attachment 137928 [details] [review] Windows: Target Windows Vista or higher Applied, thanks
Comment on attachment 137929 [details] [review] _dbus_append_address_from_socket: Factor out inet_sockaddr_to_string Applied, thanks
Comment on attachment 137930 [details] [review] sysdeps-unix: Set more specific IP-related errors when listening Applied, thanks
Comment on attachment 137931 [details] [review] sysdeps-unix: Treat bind() failures as non-fatal Applied, thanks
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 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 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
(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 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 on attachment 137932 [details] [review] Unix _dbus_connect_tcp_socket_with_nonce: Always goto out applied
Comment on attachment 137933 [details] [review] Unix _dbus_connect_tcp_socket_with_nonce: Combine all connect() errors applied
Created attachment 137962 [details] [review] Windows _dbus_listen_tcp_socket: Treat bind() failures as non-fatal Bug:
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:
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:
Created attachment 137965 [details] [review] Windows _dbus_connect_tcp_socket_with_nonce: Keep error code for 'Unknown address family' in sync with unix Bug:
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:
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 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]
(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
(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 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 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 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 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 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 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).
(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.
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 on attachment 137962 [details] [review] Windows _dbus_listen_tcp_socket: Treat bind() failures as non-fatal Thanks for reviewing, applied to master
Created attachment 138023 [details] [review] Windows _dbus_connect_tcp_socket_with_nonce: Combine all connect() errors - remove call to dbus_socket_get_invalid()
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.
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 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 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 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
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 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
remaining patch applied
Created attachment 138212 [details] [review] Fix using uninitialized value "name" in _dbus_combine_tcp_errors Coverity CID 265359. Bug:
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.
- 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.