[PATCH] Improve NNTPconnect error reporting

[Ian Jackson]

In inn2 2.4.5, NNTPconnect does not always report errors very well.
In particular:

 * If the domain name lookup fails or gives a permanent error, it
   returns -1, leaves the buffer without a message, but doesn't set
   errno; inn's internal callers seem to assume that errno will be set
   appropriately if the buffer has no message.

 * Connection failures of various kinds result in a pretty much
   arbitrary choice of errno.  In particular, I think (although I
   haven't tested this) that on some systems with IPv6 library support
   but no kernel support a useful errno value from connect failing
   with ECONNREFUSED (say) may be replaced by a comparatively useless
   one from socket saying Address family not supported.

 * The documentation doesn't make it clear how to handle the return
   values.

In the patch below I address this by always arranging for the buffer
to contain a useful message.  It either contains the message from the
peer, or a message in [square brackets] indicating which call failed
and how, including the results from gai_strerror et al.

I don't think this is perfect but it does at least mean that you
always get a useful error message.

Ian.

diff --git a/lib/remopen.c b/lib/remopen.c
index ab8a770..595f620 100644
--- a/lib/remopen.c
+++ b/lib/remopen.c
@@ -19,6 +19,16 @@
 #include "paths.h"
 
 /*
+** store an error message in buff like libinn(3) says NNTPconnect does
+*/
+static void store_error(char *buff, const char *string1, const char *string2) {
+    /* we put these in [...] to make it easier to tell them apart
+     * from messages sent by the server */
+    snprintf(buff, NNTP_STRLEN, "[%s%s]", string1, string2);
+    buff[NNTP_STRLEN-1] = 0;
+}
+
+/*
 **  Open a connection to an NNTP server and create stdio FILE's for talking
 **  to it.  Return -1 on error.
 */
@@ -29,6 +39,7 @@ int NNTPconnect(char *host, int port, FILE **FromServerp, FILE **ToServerp, char
     int	                i = -1;
     int 	        j;
     int			oerrno;
+    int			gai_r;
     FILE		*F;
 #ifdef HAVE_INET6
     struct addrinfo	hints, *ressave, *addr;
@@ -53,8 +64,11 @@ int NNTPconnect(char *host, int port, FILE **FromServerp, FILE **ToServerp, char
     hints.ai_family = PF_UNSPEC;
     hints.ai_socktype = SOCK_STREAM;
     sprintf(portbuf, "%d", port);
-    if (getaddrinfo(host, portbuf, &hints, &addr) != 0)
+    if ((gai_r = getaddrinfo(host, portbuf, &hints, &addr)) != 0) {
+	store_error(buff, "getaddrinfo failed: ", gai_strerror(gai_r));
+	errno = 0;
 	return -1;
+    }
 
     for (ressave = addr; addr; addr = addr->ai_next) {
 	if ((i = socket(addr->ai_family, addr->ai_socktype,
@@ -103,6 +117,7 @@ int NNTPconnect(char *host, int port, FILE **FromServerp, FILE **ToServerp, char
 	/* all connect(2) calls failed or some other error has occurred */
 	oerrno = errno;
 	close(i);
+	store_error(buff, "connect(s) failed: ", strerror(oerrno));
 	errno = oerrno;
 	return -1;
     }
@@ -120,9 +135,12 @@ int NNTPconnect(char *host, int port, FILE **FromServerp, FILE **ToServerp, char
     }
     else if ((hp = gethostbyname(host)) != NULL)
 	ap = hp->h_addr_list;
-    else
+    else {
 	/* Not a host name. */
+	store_error(buff, "gethostbyname failed: ", hstrerror(h_errno));
+	errno = 0;
 	return -1;
+    }
 
     /* Set up the socket address. */
     memset(&server, 0, sizeof server);
@@ -139,20 +157,31 @@ int NNTPconnect(char *host, int port, FILE **FromServerp, FILE **ToServerp, char
     client.sin_len = sizeof( struct sockaddr_in );
 #endif
     if (innconf->sourceaddress) {
-        if (!inet_aton(innconf->sourceaddress, &client.sin_addr))
+        if (!inet_aton(innconf->sourceaddress, &client.sin_addr)) {
+	    store_error(buff, "inet_aton failed on innconf's"
+			" source address: ", innconf->sourceaddress);
+	    errno = EINVAL;
 	    return -1;
+	}
     } else
 	client.sin_addr.s_addr = htonl(INADDR_ANY);
-  
+
     /* Loop through the address list, trying to connect. */
     for (; ap && *ap; ap++) {
 	/* Make a socket and try to connect. */
-	if ((i = socket(hp->h_addrtype, SOCK_STREAM, 0)) < 0)
+	if ((i = socket(hp->h_addrtype, SOCK_STREAM, 0)) < 0) {
+	    if (!buff[0]) {
+		oerrno = errno;
+		store_error(buff, "create socket failed: ", strerror(oerrno));
+		errno = oerrno;
+	    }
 	    break;
+	}
 	/* Bind to the source address we want. */
 	if (bind(i, (struct sockaddr *)&client, sizeof client) < 0) {
 	    oerrno = errno;
 	    close(i);
+	    store_error(buff, "bind failed: ", strerror(oerrno));
 	    errno = oerrno;
 	    continue;
 	}
@@ -164,6 +193,7 @@ int NNTPconnect(char *host, int port, FILE **FromServerp, FILE **ToServerp, char
 	if (connect(i, (struct sockaddr *)&server, sizeof server) < 0) {
 	    oerrno = errno;
 	    close(i);
+	    store_error(buff, "connect failed: ", strerror(oerrno));
 	    errno = oerrno;
 	    continue;
 	}
@@ -174,9 +204,13 @@ int NNTPconnect(char *host, int port, FILE **FromServerp, FILE **ToServerp, char
 	    oerrno = errno;
 	    close(i);
 	    errno = oerrno;
+	    store_error(buff, "fdopen failed: ", strerror(oerrno));
+	    errno = EINVAL;
 	    return -1;
 	}
 	if (fgets(buff, sizeof mybuff, F) == NULL) {
+	    if (!ferror(F)) store_error(buff, "peer closed connection","");
+	    else store_error(buff, "fgets failed: ", strerror(errno));
 	    oerrno = errno;
 	    fclose(F);
 	    errno = oerrno;
@@ -194,12 +228,16 @@ int NNTPconnect(char *host, int port, FILE **FromServerp, FILE **ToServerp, char
 	if ((*ToServerp = fdopen(dup(i), "w")) == NULL) {
 	    oerrno = errno;
 	    fclose(F);
+	    store_error(buff, "fdopen/dup failed: ", strerror(oerrno));
 	    errno = oerrno;
 	    return -1;
 	}
 	return 0;
     }
 
+    if (!buff[0])
+	store_error(buff, "no address(es) for host","");
+
     return -1;
 }

-- 
Ian Jackson                  personal email: <ijackson at chiark.greenend.org.uk>
These opinions are my own.        http://www.chiark.greenend.org.uk/~ijackson/
PGP2 key 1024R/0x23f5addb,     fingerprint 5906F687 BD03ACAD 0D8E602E FCF37657