imap-send: add support for IPv6

PATCH In review Benjamin Kramer 2 participants 3 messages Jun 15, 2016

Share & tools

Benjamin Kramer

author

2016-06-15 22:46 UTC #0 lore raw

PATCH 1/1 imap-send: add support for IPv6

Add IPv6 support by implementing name resolution with the protocol agnostic getaddrinfo(3) API. The old gethostbyname(3) code is still available when git is compiled with NO_IPV6.

Signed-off-by Benjamin Kramer <[email protected]>

---
 imap-send.c |   54 +++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 51 insertions(+), 3 deletions(-)

imap-send.c +51 -3

index 8154cb2..861268a 100644
@@ -982,9 +982,7 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
 	struct imap_store *ctx;
 	struct imap *imap;
 	char *arg, *rsp;
-	struct hostent *he;
-	struct sockaddr_in addr;
-	int s, a[2], preauth;
+	int s = -1, a[2], preauth;
 	pid_t pid;
 
 	ctx = xcalloc(sizeof(*ctx), 1);

@@ -1021,6 +1019,51 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
 
 		imap_info("ok\n");
 	} else {
+#ifndef NO_IPV6
+		struct addrinfo hints, *ai0, *ai;
+		int gai;
+		char portstr[6];
+
+		snprintf(portstr, sizeof(portstr), "%hu", srvc->port);
+
+		memset(&hints, 0, sizeof(hints));
+		hints.ai_socktype = SOCK_STREAM;
+		hints.ai_protocol = IPPROTO_TCP;
+
+		imap_info("Resolving %s... ", srvc->host);
+		gai = getaddrinfo(srvc->host, portstr, &hints, &ai);
+		if (gai) {
+			fprintf(stderr, "getaddrinfo: %s\n", gai_strerror(gai));
+			goto bail;
+		}
+		imap_info("ok\n");
+
+		for (ai0 = ai; ai; ai = ai->ai_next) {
+			char addr[NI_MAXHOST];
+
+			s = socket(ai->ai_family, ai->ai_socktype,
+				   ai->ai_protocol);
+			if (s < 0)
+				continue;
+
+			getnameinfo(ai->ai_addr, ai->ai_addrlen, addr,
+				    sizeof(addr), NULL, 0, NI_NUMERICHOST);
+			imap_info("Connecting to [%s]:%s... ", addr, portstr);
+
+			if (connect(s, ai->ai_addr, ai->ai_addrlen) < 0) {
+				close(s);
+				s = -1;
+				perror("connect");
+				continue;
+			}
+
+			break;
+		}
+		freeaddrinfo(ai0);
+#else /* NO_IPV6 */
+		struct hostent *he;
+		struct sockaddr_in addr;
+
 		memset(&addr, 0, sizeof(addr));
 		addr.sin_port = htons(srvc->port);
 		addr.sin_family = AF_INET;

@@ -1040,7 +1083,12 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
 		imap_info("Connecting to %s:%hu... ", inet_ntoa(addr.sin_addr), ntohs(addr.sin_port));
 		if (connect(s, (struct sockaddr *)&addr, sizeof(addr))) {
 			close(s);
+			s = -1;
 			perror("connect");
+		}
+#endif
+		if (s < 0) {
+			fputs("Error: unable to connect to server.\n", stderr);
 			goto bail;
 		}

Signature · 1 line

1.6.3.1.153.g63801e

Junio C Hamano

2016-06-15 22:46 UTC #1 lore raw

Quoted 9 lines · Benjamin Kramer

Add IPv6 support by implementing name resolution with the protocol agnostic getaddrinfo(3) API. The old gethostbyname(3) code is still available when git is compiled with NO_IPV6.
imap-send.c +0 -0
index 8154cb2..861268a 100644

imap-send.c static struct store *imap_open_store(struct imap_server_conf *srvc) · line 1027

Quoted 9 lines

imap-send.c +6 -0

@@ -1021,6 +1019,51 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
 
 		imap_info("ok\n");
 	} else {
+#ifndef NO_IPV6
+		struct addrinfo hints, *ai0, *ai;
+		int gai;
+		char portstr[6];
+
+		snprintf(portstr, sizeof(portstr), "%hu", srvc->port);

We already use %h length specifier to explicitly say the parameter is a short in the IPV4 part of this program, so I am sure this won't regress anything for people, but I wonder what the point of it is... (I am not asking nor even suggesting to change this, by the way).

imap-send.c static struct store *imap_open_store(struct imap_server_conf *srvc) · line 1037

Quoted 9 lines

imap-send.c +9 -0

+		memset(&hints, 0, sizeof(hints));
+		hints.ai_socktype = SOCK_STREAM;
+		hints.ai_protocol = IPPROTO_TCP;
+
+		imap_info("Resolving %s... ", srvc->host);
+		gai = getaddrinfo(srvc->host, portstr, &hints, &ai);
+		if (gai) {
+			fprintf(stderr, "getaddrinfo: %s\n", gai_strerror(gai));
+			goto bail;

It is Ok for now (as existing codepath liberally uses fprintf() and fputs() to report errors), but ideally we should start converting these to error() calls, I think, in a follow-up patch.

Quoted 4 lines

imap-send.c +4 -0

+		}
+		imap_info("ok\n");
+
+		for (ai0 = ai; ai; ai = ai->ai_next) {

imap-send.c static struct store *imap_open_store(struct imap_server_conf *srvc) · line 1051

Quoted 10 lines

imap-send.c +10 -0

+			char addr[NI_MAXHOST];
+
+			s = socket(ai->ai_family, ai->ai_socktype,
+				   ai->ai_protocol);
+			if (s < 0)
+				continue;
+
+			getnameinfo(ai->ai_addr, ai->ai_addrlen, addr,
+				    sizeof(addr), NULL, 0, NI_NUMERICHOST);
+			imap_info("Connecting to [%s]:%s... ", addr, portstr);

Is forcing to NUMERICHOST done to match IPV4 codepath that does inet_ntoa()? I guess that makes sense.

Quoted 19 lines

imap-send.c +14 -0

+			if (connect(s, ai->ai_addr, ai->ai_addrlen) < 0) {
+				close(s);
+				s = -1;
+				perror("connect");
+				continue;
+			}
+
+			break;
+		}
+		freeaddrinfo(ai0);
+#else /* NO_IPV6 */
+		struct hostent *he;
+		struct sockaddr_in addr;
+
 		memset(&addr, 0, sizeof(addr));
 		addr.sin_port = htons(srvc->port);
 		addr.sin_family = AF_INET;

@@ -1040,7 +1083,12 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
 		imap_info("Connecting to %s:%hu... ", inet_ntoa(addr.sin_addr), ntohs(addr.sin_port));

imap-send.c static struct store *imap_open_store(struct imap_server_conf *srvc) · line 1093

Quoted 10 lines

imap-send.c +5 -0

 		if (connect(s, (struct sockaddr *)&addr, sizeof(addr))) {
 			close(s);
+			s = -1;
 			perror("connect");
+		}
+#endif
+		if (s < 0) {
+			fputs("Error: unable to connect to server.\n", stderr);
 			goto bail;
 		}

I do not use imap-send myself, and I suspect not many people do so either, so I originally guessed the best way to judge if this has any portability (or other) issues is to directly apply to 'master' and see if anybody screams.

Thanks.

Benjamin Kramer

author

2016-06-15 22:46 UTC #2 lore raw

Quoted 4 lines · Junio C Hamano

We already use %h length specifier to explicitly say the parameter is a short in the IPV4 part of this program, so I am sure this won't regress anything for people, but I wonder what the point of it is... (I am not asking nor even suggesting to change this, by the way).

getaddrinfo(3) takes the port as a string so we have to convert it. I tried to match the style of the existing code with the format string. The portstr buffer is 6 chars long so the highest possible unsigned short 65535 fits in exactly.

Quoted 3 lines

It is Ok for now (as existing codepath liberally uses fprintf() and fputs() to report errors), but ideally we should start converting these to error() calls, I think, in a follow-up patch.

Looking at the number of fprintfs in imap-send.c a simple search/replace probably won't do the job here. I tried to contact the original author but his mail address seems to be dead ...

Quoted 2 lines

Is forcing to NUMERICHOST done to match IPV4 codepath that does inet_ntoa()? I guess that makes sense.

We need to get the IP string, otherwise the output of imap-send would make no sense. Here's what imap-send outputs when I try to connect to localhost.

Without the patch:
Resolving localhost... ok
Connecting to 127.0.0.1:993... connect: Connection refused

With the patch:
Resolving localhost... ok
Connecting to [::1]:993... connect: Connection refused Connecting to [fe80::1%lo0]:993... connect: Connection refused Connecting to [127.0.0.1]:993... connect: Connection refused
Error: unable to connect to server.

Using the hostname instead of the IP address here wouldn't be very useful.