Bugfix #279 Bad return value for ldns_udp_connect and ldns_tcp_connect

ldns_udp_connect, ldns_tcp_connect, ldns_udp_bgsend and ldns_tcp_bgsend return
a socket and otherwise 0 on failure.  This is wrong because 0 can be a valid
socket.

To not mess with the existing API, ldns_udp_connect2, ldns_tcp_connect2,
ldns_udp_bgsend2 and ldns_tcp_bgsend2 are introduced that do the same but
return -1 on failure.  Internal usage of the old functions replaced with the
new ones.

diff --git a/configure.ac b/configure.ac
index 845cd9991220616f1486c8e268e25b7ed47b21d9..48e34357646d7aaf9a8c52cac6f9eb1e16d3db9f 100644 (file)
--- a/configure.ac
+++ b/configure.ac
@@ -1046,10 +1046,10 @@ size_t strlcpy(char *dst, const char *src, size_t siz);
 
 #ifdef USE_WINSOCK
 #define SOCK_INVALID INVALID_SOCKET
-#define close_socket(_s) do { if (_s > SOCK_INVALID) {closesocket(_s); _s = SOCK_INVALID;} } while(0)
+#define close_socket(_s) do { if (_s != SOCK_INVALID) {closesocket(_s); _s = -1;} } while(0)
 #else
 #define SOCK_INVALID -1
-#define close_socket(_s) do { if (_s > SOCK_INVALID) {close(_s); _s = SOCK_INVALID;} } while(0)
+#define close_socket(_s) do { if (_s != SOCK_INVALID) {close(_s); _s = -1;} } while(0)
 #endif
 
 #ifdef __cplusplus

diff --git a/ldns/net.h.in b/ldns/net.h.in
index 9faf3a7dead531b276483d33434204cad0ac8ecf..25912582c6a8a2ac94df0da926434dc4b203ef4a 100644 (file)
--- a/ldns/net.h.in
+++ b/ldns/net.h.in
@@ -48,7 +48,20 @@ ldns_status ldns_udp_send(uint8_t **result, ldns_buffer *qbin, const struct sock
  * \param[in] to the ip addr to send to
  * \param[in] tolen length of the ip addr
  * \param[in] timeout *unused*, was the timeout value for the network
- * \return the socket used
+ * \return the socket used or -1 on failure
+ */
+int ldns_udp_bgsend2(ldns_buffer *qbin, const struct sockaddr_storage *to, socklen_t tolen, struct timeval timeout);
+
+/**
+ * Send an udp query and don't wait for an answer but return
+ * the socket
+ * This function has the flaw that it returns 0 on failure, but 0 could be a
+ * valid socket.  Please use ldns_udp_bgsend2 instead of this function.
+ * \param[in] qbin the ldns_buffer to be send
+ * \param[in] to the ip addr to send to
+ * \param[in] tolen length of the ip addr
+ * \param[in] timeout *unused*, was the timeout value for the network
+ * \return the socket used or 0 on failure
  */
 int ldns_udp_bgsend(ldns_buffer *qbin, const struct sockaddr_storage *to, socklen_t tolen, struct timeval timeout);
 
@@ -59,7 +72,20 @@ int ldns_udp_bgsend(ldns_buffer *qbin, const struct sockaddr_storage *to, sockle
  * \param[in] to the ip addr to send to
  * \param[in] tolen length of the ip addr
  * \param[in] timeout the timeout value for the connect attempt
- * \return the socket used
+ * \return the socket used or -1 on failure
+ */
+int ldns_tcp_bgsend2(ldns_buffer *qbin, const struct sockaddr_storage *to, socklen_t tolen, struct timeval timeout);
+
+/**
+ * Send an tcp query and don't wait for an answer but return
+ * the socket
+ * This function has the flaw that it returns 0 on failure, but 0 could be a
+ * valid socket.  Please use ldns_tcp_bgsend2 instead of this function.
+ * \param[in] qbin the ldns_buffer to be send
+ * \param[in] to the ip addr to send to
+ * \param[in] tolen length of the ip addr
+ * \param[in] timeout the timeout value for the connect attempt
+ * \return the socket used or 0 on failure
  */
 int ldns_tcp_bgsend(ldns_buffer *qbin, const struct sockaddr_storage *to, socklen_t tolen, struct timeval timeout);
 
@@ -104,7 +130,18 @@ ldns_status ldns_send_buffer(ldns_pkt **pkt, ldns_resolver *r, ldns_buffer *qb,
  * \param[in] to ip and family
  * \param[in] tolen length of to
  * \param[in] timeout timeout for the connect attempt
- * \return a socket descriptor
+ * \return a socket descriptor or -1 on failure
+ */
+int ldns_tcp_connect2(const struct sockaddr_storage *to, socklen_t tolen, struct timeval timeout);
+
+/**
+ * Create a tcp socket to the specified address
+ * This function has the flaw that it returns 0 on failure, but 0 could be a
+ * valid socket.  Please use ldns_tcp_connect2 instead of this function.
+ * \param[in] to ip and family
+ * \param[in] tolen length of to
+ * \param[in] timeout timeout for the connect attempt
+ * \return a socket descriptor or 0 on failure
  */
 int ldns_tcp_connect(const struct sockaddr_storage *to, socklen_t tolen, struct timeval timeout);
 
@@ -112,7 +149,17 @@ int ldns_tcp_connect(const struct sockaddr_storage *to, socklen_t tolen, struct
  * Create a udp socket to the specified address
  * \param[in] to ip and family
  * \param[in] timeout *unused*, was timeout for the socket
- * \return a socket descriptor
+ * \return a socket descriptor or -1 on failure
+ */
+int ldns_udp_connect2(const struct sockaddr_storage *to, struct timeval timeout);
+
+/**
+ * Create a udp socket to the specified address
+ * This function has the flaw that it returns 0 on failure, but 0 could be a
+ * valid socket.  Please use ldns_udp_connect2 instead of this function.
+ * \param[in] to ip and family
+ * \param[in] timeout *unused*, was timeout for the socket
+ * \return a socket descriptor or 0 on failure
  */
 int ldns_udp_connect(const struct sockaddr_storage *to, struct timeval timeout);
 

diff --git a/net.c b/net.c
index a296cc8bbaf0230b7e591301fbfe415ec7fb9284..13bfc97240524c3d3a400e8a90fa715786ef6ba2 100644 (file)
--- a/net.c
+++ b/net.c
@@ -198,12 +198,12 @@ ldns_tcp_connect_from(const struct sockaddr_storage *to, socklen_t tolen,
 #ifndef S_SPLINT_S
        if ((sockfd = socket((int)((struct sockaddr*)to)->sa_family, SOCK_STREAM, 
                                        IPPROTO_TCP)) == SOCK_INVALID) {
-               return 0;
+               return -1;
        }
 #endif
        if (from && bind(sockfd, (const struct sockaddr*)from, fromlen) == SOCK_INVALID){
                close_socket(sockfd);
-               return 0;
+               return -1;
        }
 
        /* perform nonblocking connect, to be able to wait with select() */
@@ -216,13 +216,13 @@ ldns_tcp_connect_from(const struct sockaddr_storage *to, socklen_t tolen,
                if(1) {
 #endif
                        close_socket(sockfd);
-                       return 0;
+                       return -1;
                }
 #else /* USE_WINSOCK */
                if(WSAGetLastError() != WSAEINPROGRESS &&
                        WSAGetLastError() != WSAEWOULDBLOCK) {
                        close_socket(sockfd);
-                       return 0;
+                       return -1;
                }
 #endif
                /* error was only telling us that it would block */
@@ -235,7 +235,7 @@ ldns_tcp_connect_from(const struct sockaddr_storage *to, socklen_t tolen,
 
                if(!ldns_sock_wait(sockfd, timeout, 1)) {
                        close_socket(sockfd);
-                       return 0;
+                       return -1;
                }
 
                /* check if there is a pending error for nonblocking connect */
@@ -256,7 +256,7 @@ ldns_tcp_connect_from(const struct sockaddr_storage *to, socklen_t tolen,
                        close_socket(sockfd);
                        /* error in errno for our user */
                        errno = error;
-                       return 0;
+                       return -1;
                }
 #else /* USE_WINSOCK */
                if(error == WSAEINPROGRESS)
@@ -266,7 +266,7 @@ ldns_tcp_connect_from(const struct sockaddr_storage *to, socklen_t tolen,
                else if(error != 0) {
                        close_socket(sockfd);
                        errno = error;
-                       return 0;
+                       return -1;
                }
 #endif /* USE_WINSOCK */
                /* connected */
@@ -282,6 +282,14 @@ ldns_tcp_connect_from(const struct sockaddr_storage *to, socklen_t tolen,
 int
 ldns_tcp_connect(const struct sockaddr_storage *to, socklen_t tolen, 
                struct timeval timeout)
+{
+       int s = ldns_tcp_connect_from(to, tolen, NULL, 0, timeout);
+       return s > 0 ? s : 0;
+}
+
+int
+ldns_tcp_connect2(const struct sockaddr_storage *to, socklen_t tolen, 
+               struct timeval timeout)
 {
        return ldns_tcp_connect_from(to, tolen, NULL, 0, timeout);
 }
@@ -296,13 +304,9 @@ ldns_tcp_bgsend_from(ldns_buffer *qbin,
        
        sockfd = ldns_tcp_connect_from(to, tolen, from, fromlen, timeout);
        
-       if (sockfd == 0) {
-               return 0;
-       }
-       
-       if (ldns_tcp_send_query(qbin, sockfd, to, tolen) == 0) {
+       if (sockfd >= 0 && ldns_tcp_send_query(qbin, sockfd, to, tolen) == 0) {
                close_socket(sockfd);
-               return 0;
+               return -1;
        }
        
        return sockfd;
@@ -313,9 +317,17 @@ ldns_tcp_bgsend(ldns_buffer *qbin,
                const struct sockaddr_storage *to, socklen_t tolen, 
                struct timeval timeout)
 {
-       return ldns_tcp_bgsend_from(qbin, to, tolen, NULL, 0, timeout);
+       int s = ldns_tcp_bgsend_from(qbin, to, tolen, NULL, 0, timeout);
+       return s > 0 ? s : 0;
 }
 
+int
+ldns_tcp_bgsend2(ldns_buffer *qbin,
+               const struct sockaddr_storage *to, socklen_t tolen, 
+               struct timeval timeout)
+{
+       return ldns_tcp_bgsend_from(qbin, to, tolen, NULL, 0, timeout);
+}
 
 /* keep in mind that in DNS tcp messages the first 2 bytes signal the
  * amount data to expect
@@ -331,7 +343,7 @@ ldns_tcp_send_from(uint8_t **result,  ldns_buffer *qbin,
        
        sockfd = ldns_tcp_bgsend_from(qbin, to, tolen, from, fromlen, timeout);
        
-       if (sockfd == 0) {
+       if (sockfd == -1) {
                return LDNS_STATUS_ERR;
        }
 
@@ -369,13 +381,28 @@ ldns_udp_connect(const struct sockaddr_storage *to, struct timeval ATTR_UNUSED(t
 #ifndef S_SPLINT_S
        if ((sockfd = socket((int)((struct sockaddr*)to)->sa_family, SOCK_DGRAM, 
                                        IPPROTO_UDP)) 
-                       == -1) {
+                       == SOCK_INVALID) {
                 return 0;
         }
 #endif
        return sockfd;
 }
 
+int
+ldns_udp_connect2(const struct sockaddr_storage *to, struct timeval ATTR_UNUSED(timeout))
+{
+       int sockfd;
+
+#ifndef S_SPLINT_S
+       if ((sockfd = socket((int)((struct sockaddr*)to)->sa_family, SOCK_DGRAM, 
+                                       IPPROTO_UDP)) 
+                       == SOCK_INVALID) {
+                return -1;
+        }
+#endif
+       return sockfd;
+}
+
 static int
 ldns_udp_bgsend_from(ldns_buffer *qbin,
                const struct sockaddr_storage *to  , socklen_t tolen, 
@@ -384,20 +411,20 @@ ldns_udp_bgsend_from(ldns_buffer *qbin,
 {
        int sockfd;
 
-       sockfd = ldns_udp_connect(to, timeout);
+       sockfd = ldns_udp_connect2(to, timeout);
 
-       if (sockfd == 0) {
-               return 0;
+       if (sockfd == -1) {
+               return -1;
        }
 
        if (from && bind(sockfd, (const struct sockaddr*)from, fromlen) == -1){
                close_socket(sockfd);
-               return 0;
+               return -1;
        }
 
        if (ldns_udp_send_query(qbin, sockfd, to, tolen) == 0) {
                close_socket(sockfd);
-               return 0;
+               return -1;
        }
        return sockfd;
 }
@@ -406,6 +433,15 @@ int
 ldns_udp_bgsend(ldns_buffer *qbin,
                const struct sockaddr_storage *to  , socklen_t tolen, 
                struct timeval timeout)
+{
+       s = ldns_udp_bgsend_from(qbin, to, tolen, NULL, 0, timeout);
+       return s > 0 ? s : 0;
+}
+
+int
+ldns_udp_bgsend2(ldns_buffer *qbin,
+               const struct sockaddr_storage *to  , socklen_t tolen, 
+               struct timeval timeout)
 {
        return ldns_udp_bgsend_from(qbin, to, tolen, NULL, 0, timeout);
 }
@@ -421,7 +457,7 @@ ldns_udp_send_from(uint8_t **result, ldns_buffer *qbin,
 
        sockfd = ldns_udp_bgsend_from(qbin, to, tolen, from, fromlen, timeout);
 
-       if (sockfd == 0) {
+       if (sockfd == -1) {
                return LDNS_STATUS_SOCKET_ERROR;
        }
 

diff --git a/resolver.c b/resolver.c
index 5a1db90113af5d65e8b86b8b069304dc0f406c41..a1a746e5b2b4136363b7c4ad7e1a8e781a367220 100644 (file)
--- a/resolver.c
+++ b/resolver.c
@@ -1533,14 +1533,14 @@ void
 ldns_axfr_abort(ldns_resolver *resolver)
 {
        /* Only abort if an actual AXFR is in progress */
-       if (resolver->_socket != 0)
+       if (resolver->_socket != -1)
        {
 #ifndef USE_WINSOCK
                close(resolver->_socket);
 #else
                closesocket(resolver->_socket);
 #endif
-               resolver->_socket = 0;
+               resolver->_socket = -1;
        }
 }