default tcp host
Daniel P. Berrange
dan at berrange.com
Wed Jun 13 13:42:47 PDT 2007
On Wed, Jun 13, 2007 at 07:09:03PM +0100, Daniel P. Berrange wrote:
> On Wed, Jun 13, 2007 at 04:56:44PM -0400, Havoc Pennington wrote:
> > Here is the patch I put in for this. Better ideas welcome.
>
> The TCP stuff is pretty badly broken in the way it does lookups
> using the legacy gethostbyname() functions. Its API contract does
> not enable it to provide sorted results wrt to IPv4 & IPv6 - you
> can only query one family at a time. DBus never asks for an address
> family so its only getting IPv4 results and then only using the first
> address returned anyway. So this is all going to behave rather badly
> in a multi-protocol environemnt. gethostbyname is also not in the
> slightest bit thread safe :-(
>
> IMHO the whole lot needs to be rewritten & made to use getaddrinfo().
> There's a very good coverage of how to do adddress lookup that
> works correctly in a dual IPv4/IPv6 network environment at:
>
> http://people.redhat.com/drepper/userapi-ipv6.html
>
> I've followed these guidelines for a few other apps & although it may
> seem complex at first it works very well indeed in a dual IPv4/6 network
> environment
>
> I'd be willing to take a stab at it, but its not something I'll likely
> have spare cycles for in short term. I'd certainly help any testing since
> I'm running a fully connected IPv6 stack everywhere.
Oh well, so much for getting away from a computer this evening. Decided
to try hacking DBus to use getaddrinfo after all. Attaching the patch
I've got so far. It works with the following setting for <listen> in
the session.conf or system.conf:
- tcp:
don't specify host or port. getaddrinfo will resolve the address
to be either 0.0.0.0 or :: depending on whether IPv6 is
enabled or not. The kernel will choose a port
- tcp:host=127.0.0.1
explicitly bind to IPv4 localhost only. Kernel chooses port
- tcp:host=%3a%3a1
explicitly bind to IPv6 localhost only (::1). Kernel chooses port
- tcp:host=localhost
bind to either 127.0.0.1 or ::1 depending on what you've got
localhost configured to point to & whether IPv6 is active.
Again kernel chooses port.
(All of the above + an explicit port=XXX also work of course)
Now, when printing out the address, we add in the kernel's chosen
port, eg
- tcp:port=123456
- tcp:host=127.0.0.1,port=123456
- tcp:host=%3a%3a1,port=123456
- tcp:host=localhost,port=123456
You might wonder how a client connects to the server in the first
scenario ? Well getaddrinfo() treats a NULL hostname as meaning
127.0.0.1 or ::1 - this works because the server side has bound
to all addresses via 0.0.0.0 or :: which obviously includes the
localhost addresses.
You can see this in action if you run netstat. Notice here I have
a server listening on ::, port 2000, and when I connected with
the client, it made the connection over IPv6 to ::1
# netstat -t -a -n -p | grep 2000
tcp 0 0 :::2000 :::* LISTEN 14621/dbus-daemon
tcp 0 0 ::1:47240 ::1:2000 ESTABLISHED 14844/lt-dbus-monit
tcp 23 0 ::1:2000 ::1:47240 ESTABLISHED 14621/dbus-daemon
So there is no need for an explicit 'all_interfaces=true' address
flag with this scheme. The general recommendation can be to set
either use 'tcp:localhost' for local only access, or 'tcp:' for
global access - both doing 'the right thing(tm)' if the user has
IPv6 enabled on their system.
There is only one minor problem with the patch I'm attaching. A given
hostname record may resolve to multiple IP addresses. On the client
side this works fine - the addresses are sorted in order most likely
to succeed, so we just try connect() in order until we establish a
connection. On the server side though there's a small irritating
issue:
- it is implementation defined whether you can accept an IPv4
connection over an IPv6 bound socket. eg if you listen on
the IPv6 any address :: you may or may not be able to accept
connections to IPv4 addresses. So you may need to bind to the
same port several times - once per address family. In Linux
this behaviour is toggled by net.ipv6.bindv6only, other OS
have it hardcoded.
- a name may well resolve to multiple addresses - eg an IPv4 and
IPv6 address.
- a machine can have multiple interfaces with different IPs and
all mapping to a single name
The crux of the matter is that for correctness & portability when doing
the TCP server setup we may need to open & bind to multiple sockets.
The DBusTransportSocket struct currently only has a single fd. So I
still need to adapt this such that it opens & deals with multiple fds
on the server side of things. A little unpleasant, but certainly
doable
One other point - getaddrinfo will do service lookups to convert from
a named port number to a numeric port number. I still need to change
several functions to pass about char * for port instead of an int.
Finally, Windows does have support for getnameinfo/getaddrinfo since
Windows XP or later, but I've not tried doing Windows sysdep changes
yet
http://msdn2.microsoft.com/en-us/library/ms738520.aspx
One final thought with all this - we may want to add an explicit
'protocol=ipv4' and/or 'protocol=ipv6' field to the <listen>
strings, so an admin can explicitly stop DBus from binding to
certain protocols, even if they're enabled on the machine. This
would translate into setting the ai_family field in the 'hints'
param passed into getaddrinfo().
Regards,
Dan.
--
|=- GPG key: http://www.berrange.com/~dan/gpgkey.txt -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- berrange at redhat.com - Daniel Berrange - dan at berrange.com -=|
-------------- next part --------------
Index: dbus/dbus-server-socket.c
===================================================================
RCS file: /cvs/dbus/dbus/dbus/dbus-server-socket.c,v
retrieving revision 1.5
diff -u -p -r1.5 dbus-server-socket.c
--- dbus/dbus-server-socket.c 13 Jun 2007 20:52:58 -0000 1.5
+++ dbus/dbus-server-socket.c 14 Jun 2007 01:49:19 -0000
@@ -296,23 +296,18 @@ _dbus_server_new_for_socket (int
/**
* Creates a new server listening on TCP.
- * If inaddr_any is TRUE, listens on all local interfaces.
+ * If host is NULL, listens on all local interfaces.
* Otherwise, it resolves the hostname and listens only on
- * the resolved address of the hostname. The hostname is used
- * even if inaddr_any is TRUE, as the hostname to report when
- * dbus_server_get_address() is called. If the hostname is #NULL,
- * localhost is used.
+ * the resolved address of the hostname.
*
* @param host the hostname to listen on.
* @param port the port to listen on or 0 to let the OS choose
- * @param inaddr_any #TRUE to listen on all local interfaces
* @param error location to store reason for failure.
* @returns the new server, or #NULL on failure.
*/
DBusServer*
_dbus_server_new_for_tcp_socket (const char *host,
dbus_uint32_t port,
- dbus_bool_t inaddr_any,
DBusError *error)
{
DBusServer *server;
@@ -328,22 +323,30 @@ _dbus_server_new_for_tcp_socket (const c
return NULL;
}
- if (host == NULL)
- host = "localhost";
-
- listen_fd = _dbus_listen_tcp_socket (host, &port, inaddr_any, error);
+ listen_fd = _dbus_listen_tcp_socket (host, &port, error);
_dbus_fd_set_close_on_exec (listen_fd);
- _dbus_string_init_const (&host_str, host);
- if (!_dbus_string_append (&address, "tcp:host=") ||
- !_dbus_address_append_escaped (&address, &host_str) ||
- !_dbus_string_append (&address, ",port=") ||
- !_dbus_string_append_int (&address, port))
- {
- _dbus_string_free (&address);
- dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
- return NULL;
- }
+ if (host) {
+ _dbus_string_init_const (&host_str, host);
+ if (!_dbus_string_append (&address, "tcp:host=") ||
+ !_dbus_address_append_escaped (&address, &host_str) ||
+ !_dbus_string_append (&address, ",port=") ||
+ !_dbus_string_append_int (&address, port))
+ {
+ _dbus_string_free (&address);
+ dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
+ return NULL;
+ }
+ } else {
+ if (!_dbus_string_append (&address, "tcp:") ||
+ !_dbus_string_append (&address, "port=") ||
+ !_dbus_string_append_int (&address, port))
+ {
+ _dbus_string_free (&address);
+ dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
+ return NULL;
+ }
+ }
if (listen_fd < 0)
@@ -395,33 +398,11 @@ _dbus_server_listen_socket (DBusAddressE
{
const char *host;
const char *port;
- const char *all_interfaces;
- dbus_bool_t inaddr_any;
long lport;
host = dbus_address_entry_get_value (entry, "host");
port = dbus_address_entry_get_value (entry, "port");
- all_interfaces = dbus_address_entry_get_value (entry, "all_interfaces");
- inaddr_any = FALSE;
- if (all_interfaces != NULL)
- {
- if (strcmp (all_interfaces, "true") == 0)
- {
- inaddr_any = TRUE;
- }
- else if (strcmp (all_interfaces, "false") == 0)
- {
- inaddr_any = FALSE;
- }
- else
- {
- _dbus_set_bad_address(error, NULL, NULL,
- "all_interfaces flag in tcp: address should be 'true' or 'false'");
- return DBUS_SERVER_LISTEN_BAD_ADDRESS;
- }
- }
-
if (port == NULL)
{
lport = 0;
@@ -443,7 +424,7 @@ _dbus_server_listen_socket (DBusAddressE
}
}
- *server_p = _dbus_server_new_for_tcp_socket (host, lport, inaddr_any, error);
+ *server_p = _dbus_server_new_for_tcp_socket (host, lport, error);
if (*server_p)
{
Index: dbus/dbus-server-socket.h
===================================================================
RCS file: /cvs/dbus/dbus/dbus/dbus-server-socket.h,v
retrieving revision 1.2
diff -u -p -r1.2 dbus-server-socket.h
--- dbus/dbus-server-socket.h 13 Jun 2007 20:52:58 -0000 1.2
+++ dbus/dbus-server-socket.h 14 Jun 2007 01:49:19 -0000
@@ -32,7 +32,6 @@ DBusServer* _dbus_server_new_for_socket
const DBusString *address);
DBusServer* _dbus_server_new_for_tcp_socket (const char *host,
dbus_uint32_t port,
- dbus_bool_t inaddr_any,
DBusError *error);
DBusServerListenResult _dbus_server_listen_socket (DBusAddressEntry *entry,
DBusServer **server_p,
Index: dbus/dbus-sysdeps-unix.c
===================================================================
RCS file: /cvs/dbus/dbus/dbus/dbus-sysdeps-unix.c,v
retrieving revision 1.29
diff -u -p -r1.29 dbus-sysdeps-unix.c
--- dbus/dbus-sysdeps-unix.c 13 Jun 2007 20:52:58 -0000 1.29
+++ dbus/dbus-sysdeps-unix.c 14 Jun 2007 01:49:20 -0000
@@ -745,10 +745,10 @@ _dbus_connect_tcp_socket (const char
dbus_uint32_t port,
DBusError *error)
{
- int fd;
- struct sockaddr_in addr;
- struct hostent *he;
- struct in_addr *haddr;
+ int fd = -1;
+ struct addrinfo hints;
+ struct addrinfo *ai, *tmp;
+ char portstr[100];
_DBUS_ASSERT_ERROR_IS_CLEAR (error);
@@ -761,45 +761,62 @@ _dbus_connect_tcp_socket (const char
}
_DBUS_ASSERT_ERROR_IS_CLEAR(error);
- if (host == NULL)
- host = "localhost";
+ _DBUS_ZERO (hints);
- he = gethostbyname (host);
- if (he == NULL)
+ hints.ai_socktype = SOCK_STREAM;
+ hints.ai_flags = AI_ADDRCONFIG;
+
+ /* XXX lame - should pass port in as a str */
+ sprintf(portstr, "%d", port);
+
+ if (getaddrinfo(host, portstr, &hints, &ai) < 0)
{
dbus_set_error (error,
- _dbus_error_from_errno (errno),
- "Failed to lookup hostname: %s",
- host);
+ _dbus_error_from_errno (errno),
+ "Failed to lookup hostname: %s",
+ host);
_dbus_close (fd, NULL);
return -1;
}
-
- haddr = ((struct in_addr *) (he->h_addr_list)[0]);
- _DBUS_ZERO (addr);
- memcpy (&addr.sin_addr, haddr, sizeof(struct in_addr));
- addr.sin_family = AF_INET;
- addr.sin_port = htons (port);
-
- if (connect (fd, (struct sockaddr*) &addr, sizeof (addr)) < 0)
- {
- dbus_set_error (error,
- _dbus_error_from_errno (errno),
- "Failed to connect to socket %s:%d %s",
- host, port, _dbus_strerror (errno));
-
- _dbus_close (fd, NULL);
- fd = -1;
+ tmp = ai;
+ while (tmp)
+ {
+ if (!_dbus_open_socket (&fd, tmp->ai_family, SOCK_STREAM, 0, error))
+ {
+ freeaddrinfo(ai);
+ _DBUS_ASSERT_ERROR_IS_SET(error);
+ return -1;
+ }
+ _DBUS_ASSERT_ERROR_IS_CLEAR(error);
+
+ if (connect (fd, (struct sockaddr*) tmp->ai_addr, tmp->ai_addrlen) < 0)
+ {
+ _dbus_close(fd, NULL);
+ fd = -1;
+ tmp = tmp->ai_next;
+ continue;
+ }
+ break;
+ }
+ freeaddrinfo(ai);
+
+ if (fd == -1)
+ {
+ dbus_set_error (error,
+ _dbus_error_from_errno (errno),
+ "Failed to connect to socket %s:%d %s",
+ host, port, _dbus_strerror(errno));
return -1;
}
+
if (!_dbus_set_fd_nonblocking (fd, error))
{
_dbus_close (fd, NULL);
fd = -1;
-
+
return -1;
}
@@ -814,79 +831,98 @@ _dbus_connect_tcp_socket (const char
*
* @param host the host name to listen on
* @param port the prot to listen on, if zero a free port will be used
- * @param inaddr_any TRUE to listen on all local interfaces instead of on the host name
* @param error return location for errors
* @returns the listening file descriptor or -1 on error
*/
int
_dbus_listen_tcp_socket (const char *host,
dbus_uint32_t *port,
- dbus_bool_t inaddr_any,
DBusError *error)
{
int listen_fd;
- struct sockaddr_in addr;
- socklen_t len = (socklen_t) sizeof (struct sockaddr);
+ struct addrinfo hints;
+ struct addrinfo *ai, *tmp;
+ struct sockaddr_storage addr;
+ socklen_t addrlen;
+ char portstr[100];
+ char hoststr[1000];
_DBUS_ASSERT_ERROR_IS_CLEAR (error);
- if (!_dbus_open_tcp_socket (&listen_fd, error))
- {
- _DBUS_ASSERT_ERROR_IS_SET(error);
- return -1;
- }
- _DBUS_ASSERT_ERROR_IS_CLEAR(error);
+ _DBUS_ZERO (hints);
- _DBUS_ZERO (addr);
+ hints.ai_socktype = SOCK_STREAM;
+ hints.ai_flags = AI_ADDRCONFIG | AI_PASSIVE;
+
+ /* XXX lame should pass port in & out as a char* */
+ sprintf(portstr, "%d", *port);
- if (inaddr_any)
+ if (getaddrinfo(host, portstr, &hints, &ai) < 0 || !ai)
{
- addr.sin_addr.s_addr = INADDR_ANY;
+ dbus_set_error (error,
+ _dbus_error_from_errno (errno),
+ "Failed to lookup hostname: %s",
+ host);
+ _dbus_close (listen_fd, NULL);
+ return -1;
}
- else
- {
- struct hostent *he;
- struct in_addr *haddr;
- he = gethostbyname (host);
- if (he == NULL)
- {
- dbus_set_error (error,
- _dbus_error_from_errno (errno),
- "Failed to lookup hostname: %s",
- host);
- _dbus_close (listen_fd, NULL);
- return -1;
- }
-
- haddr = ((struct in_addr *) (he->h_addr_list)[0]);
+ tmp = ai;
+ while (tmp)
+ {
+ if (!_dbus_open_socket (&listen_fd, tmp->ai_family, SOCK_STREAM, 0, error))
+ {
+ _DBUS_ASSERT_ERROR_IS_SET(error);
+ return -1;
+ }
+ _DBUS_ASSERT_ERROR_IS_CLEAR(error);
- memcpy (&addr.sin_addr, haddr, sizeof (struct in_addr));
- }
-
- addr.sin_family = AF_INET;
- addr.sin_port = htons (*port);
+
+ if (bind (listen_fd, (struct sockaddr*) tmp->ai_addr, tmp->ai_addrlen) < 0)
+ {
+ if (errno == EADDRINUSE)
+ {
+ tmp = tmp->ai_next;
+ continue;
+ }
+ dbus_set_error (error, _dbus_error_from_errno (errno),
+ "Failed to bind socket \"%s:%d\": %s",
+ host, *port, _dbus_strerror (errno));
+ _dbus_close (listen_fd, NULL);
+ freeaddrinfo(ai);
+ return -1;
+ }
- if (bind (listen_fd, (struct sockaddr*) &addr, sizeof (struct sockaddr)))
- {
- dbus_set_error (error, _dbus_error_from_errno (errno),
- "Failed to bind socket \"%s:%d\": %s",
- host, *port, _dbus_strerror (errno));
- _dbus_close (listen_fd, NULL);
- return -1;
+ if (listen (listen_fd, 30 /* backlog */) < 0)
+ {
+ dbus_set_error (error, _dbus_error_from_errno (errno),
+ "Failed to listen on socket \"%s:%d\": %s",
+ host, *port, _dbus_strerror (errno));
+ _dbus_close (listen_fd, NULL);
+ freeaddrinfo(ai);
+ return -1;
+ }
+
+ /* XXX wrong - we should bind to all returned addresses really */
+ break;
}
+ freeaddrinfo(ai);
- if (listen (listen_fd, 30 /* backlog */) < 0)
+ addrlen = sizeof(addr);
+ getsockname(listen_fd, (struct sockaddr*) &addr, &addrlen);
+
+ if (getnameinfo((struct sockaddr*)&addr, addrlen, hoststr, sizeof(hoststr), portstr, sizeof(portstr),
+ NI_NUMERICHOST) < 0)
{
- dbus_set_error (error, _dbus_error_from_errno (errno),
- "Failed to listen on socket \"%s:%d\": %s",
- host, *port, _dbus_strerror (errno));
- _dbus_close (listen_fd, NULL);
+ dbus_set_error (error, _dbus_error_from_errno (errno),
+ "Failed to resolve port \"%s:%d\": %s",
+ host, *port, _dbus_strerror (errno));
+ _dbus_close(listen_fd, NULL);
return -1;
}
- getsockname(listen_fd, (struct sockaddr*) &addr, &len);
- *port = (dbus_uint32_t) ntohs(addr.sin_port);
+ /* XXX lame should pass port in & out as a char* */
+ sscanf(portstr, "%d", port);
if (!_dbus_set_fd_nonblocking (listen_fd, error))
{
@@ -1072,7 +1108,7 @@ _dbus_read_credentials_socket (int
#ifdef SO_PEERCRED
struct ucred cr;
int cr_len = sizeof (cr);
-
+
if (getsockopt (client_fd, SOL_SOCKET, SO_PEERCRED, &cr, &cr_len) == 0 &&
cr_len == sizeof (cr))
{
Index: dbus/dbus-sysdeps.h
===================================================================
RCS file: /cvs/dbus/dbus/dbus/dbus-sysdeps.h,v
retrieving revision 1.76
diff -u -p -r1.76 dbus-sysdeps.h
--- dbus/dbus-sysdeps.h 13 Jun 2007 20:52:58 -0000 1.76
+++ dbus/dbus-sysdeps.h 14 Jun 2007 01:49:21 -0000
@@ -151,7 +151,6 @@ int _dbus_connect_tcp_socket (const cha
DBusError *error);
int _dbus_listen_tcp_socket (const char *host,
dbus_uint32_t *port,
- dbus_bool_t inaddr_any,
DBusError *error);
int _dbus_accept (int listen_fd);
More information about the dbus
mailing list