Unit tests and bug fixes for XmlRpcSocket (#1218)

* Unit tests and bug fixes for XmlRpcSocket

Add system call mocks for socket functions, and use these to test the XmlRpcSocket class.
Improve the address resolution error messages to include a description the error generated by getaddrinfo.
Fix an existing TODO where EWOULDBLOCK was not handled as a fatal error on Linux.
Fix get_port so that it checks the return code from getsockname; this prevents it from using the socket data uninitialized if getsockname fails.
Make a few of the string arguments to XmlRpcSocket const so that they're compatible with string literals.

* Add LGPL copyright block and add myself to authors.

* Wrap features that obviously won't work on Windows

* Indent

utilities/xmlrpcpp/include/xmlrpcpp/XmlRpcSocket.h

@@ -39,7 +39,7 @@ namespace XmlRpc {
     static bool nbRead(int socket, std::string& s, bool *eof);
 
     //! Write text to the specified socket. Returns false on error.
-    static bool nbWrite(int socket, std::string& s, int *bytesSoFar);
+    static bool nbWrite(int socket, const std::string& s, int *bytesSoFar);
 
 
     // The next four methods are appropriate for servers.
@@ -62,7 +62,7 @@ namespace XmlRpc {
 
 
     //! Connect a socket to a server (from a client)
-    static bool connect(int socket, std::string& host, int port);
+    static bool connect(int socket, const std::string& host, int port);
 
 
     //! Returns last errno

utilities/xmlrpcpp/package.xml

@@ -14,6 +14,7 @@
   <author>Chris Morley</author>
   <author>Konstantin Pilipchuk</author>
   <author>Morgan Quigley</author>
+  <author>Austin Hendrix</author>
 
   <buildtool_depend>catkin</buildtool_depend>
 

utilities/xmlrpcpp/src/XmlRpcSocket.cpp

@@ -197,7 +197,7 @@ XmlRpcSocket::accept(int fd)
 
 // Connect a socket to a server (from a client)
 bool
-XmlRpcSocket::connect(int fd, std::string& host, int port)
+XmlRpcSocket::connect(int fd, const std::string& host, int port)
 {
   sockaddr_storage ss;
   socklen_t ss_len;
@@ -207,9 +207,13 @@ XmlRpcSocket::connect(int fd, std::string& host, int port)
   struct addrinfo hints;
   memset(&hints, 0, sizeof(hints));
   hints.ai_family = AF_UNSPEC;
-  if (getaddrinfo(host.c_str(), NULL, &hints, &addr) != 0)
-  {
-    fprintf(stderr, "Couldn't find an %s address for [%s]\n", s_use_ipv6_ ? "AF_INET6" : "AF_INET", host.c_str());
+  int getaddr_err = getaddrinfo(host.c_str(), NULL, &hints, &addr);
+  if (0 != getaddr_err) {
+    if(getaddr_err == EAI_SYSTEM) {
+      XmlRpcUtil::error("Couldn't find an %s address for [%s]: %s\n", s_use_ipv6_ ? "AF_INET6" : "AF_INET", host.c_str(), XmlRpcSocket::getErrorMsg().c_str());
+    } else {
+      XmlRpcUtil::error("Couldn't find an %s address for [%s]: %s\n", s_use_ipv6_ ? "AF_INET6" : "AF_INET", host.c_str(), gai_strerror(getaddr_err));
+    }
     return false;
   }
 
@@ -251,24 +255,31 @@ XmlRpcSocket::connect(int fd, std::string& host, int port)
 
   if (!found)
   {
-    printf("Couldn't find an %s address for [%s]\n", s_use_ipv6_ ? "AF_INET6" : "AF_INET", host.c_str());
+    XmlRpcUtil::error("Couldn't find an %s address for [%s]\n", s_use_ipv6_ ? "AF_INET6" : "AF_INET", host.c_str());
     freeaddrinfo(addr);
     return false;
   }
 
   // For asynch operation, this will return EWOULDBLOCK (windows) or
   // EINPROGRESS (linux) and we just need to wait for the socket to be writable...
   int result = ::connect(fd, (sockaddr*)&ss, ss_len);
+  bool success = true;
   if (result != 0 ) {
-	  int error = getError();
-	  if ( (error != EINPROGRESS) && error != EWOULDBLOCK) { // actually, should probably do a platform check here, EWOULDBLOCK on WIN32 and EINPROGRESS otherwise
-		    printf("::connect error = %d\n", getError());
-	  }
+    int error = getError();
+    // platform check here, EWOULDBLOCK on WIN32 and EINPROGRESS otherwise
+#if defined(_WINDOWS)
+    if (error != EWOULDBLOCK) {
+#else
+    if (error != EINPROGRESS) {
+#endif
+      XmlRpcUtil::error("::connect error = %s\n", getErrorMsg(error).c_str());
+      success = false;
+    }
   }
 
   freeaddrinfo(addr);
 
-  return result == 0 || nonFatalError();
+  return success;
 }
 
 
@@ -308,7 +319,7 @@ XmlRpcSocket::nbRead(int fd, std::string& s, bool *eof)
 
 // Write text to the specified socket. Returns false on error.
 bool
-XmlRpcSocket::nbWrite(int fd, std::string& s, int *bytesSoFar)
+XmlRpcSocket::nbWrite(int fd, const std::string& s, int *bytesSoFar)
 {
   int nToWrite = int(s.length()) - *bytesSoFar;
   char *sp = const_cast<char*>(s.c_str()) + *bytesSoFar;
@@ -372,18 +383,18 @@ int XmlRpcSocket::get_port(int socket)
 {
   sockaddr_storage ss;
   socklen_t ss_len = sizeof(ss);
-  getsockname(socket, (sockaddr *)&ss, &ss_len);
+  if(getsockname(socket, (sockaddr *)&ss, &ss_len) == 0) {
+    sockaddr_in *sin = (sockaddr_in *)&ss;
+    sockaddr_in6 *sin6 = (sockaddr_in6 *)&ss;
 
-  sockaddr_in *sin = (sockaddr_in *)&ss;
-  sockaddr_in6 *sin6 = (sockaddr_in6 *)&ss;
-
-  switch (ss.ss_family)
-  {
-    case AF_INET:
-      return ntohs(sin->sin_port);
-    case AF_INET6:
-      return ntohs(sin6->sin6_port);
-  }  
+    switch (ss.ss_family)
+    {
+      case AF_INET:
+        return ntohs(sin->sin_port);
+      case AF_INET6:
+        return ntohs(sin6->sin6_port);
+    }
+  }
   return 0;
 }
 

utilities/xmlrpcpp/test/CMakeLists.txt

@@ -3,6 +3,19 @@ if(TARGET xmlrpcvalue_base64)
   target_link_libraries(xmlrpcvalue_base64 xmlrpcpp)
 endif()
 
+if(NOT WIN32)
+  catkin_add_gtest(test_socket
+    test_socket.cpp
+    test_system_mocks.c
+    ../src/XmlRpcSocket.cpp
+    ../src/XmlRpcUtil.cpp
+  )
+  set_target_properties(test_socket PROPERTIES
+    LINK_FLAGS
+    "-Wl,--wrap=accept -Wl,--wrap=bind -Wl,--wrap=close -Wl,--wrap=connect -Wl,--wrap=getaddrinfo -Wl,--wrap=getsockname -Wl,--wrap=listen -Wl,--wrap=read -Wl,--wrap=setsockopt -Wl,--wrap=select -Wl,--wrap=socket -Wl,--wrap=write -Wl,--wrap=fcntl -Wl,--wrap=freeaddrinfo"
+  )
+endif()
+
 catkin_add_gtest(TestValues TestValues.cpp)
 target_link_libraries(TestValues xmlrpcpp)
 

utilities/xmlrpcpp/test/TestValues.cpp

@@ -1,3 +1,27 @@
+/*
+ * Unit tests for XmlRpc++
+ *
+ * Copyright (C) 2017, Zoox Inc
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ * Author: Austin Hendrix <austin@zoox.com>
+ * Loosely based on the original TestValues.cpp by Chris Morley
+ *
+ */
+
 // TestValues.cpp : Test XML encoding and decoding of XmlRpcValues.
 
 #include <stdlib.h>

utilities/xmlrpcpp/test/TestXml.cpp

@@ -1,3 +1,27 @@
+/*
+ * Unit tests for XmlRpc++
+ *
+ * Copyright (C) 2017, Zoox Inc
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ * Author: Austin Hendrix <austin@zoox.com>
+ * Loosely based on the original TestXml.cpp by Chris Morley
+ *
+ */
+
 // TestXml.cpp : Test XML encoding and decoding.
 // The characters <>&'" are illegal in xml and must be encoded.