miniupnp.tuxfamily.org Forum Index miniupnp.tuxfamily.org
The forum about miniupnp and libnatpmp
 
 FAQFAQ   SearchSearch   MemberlistMemberlist   UsergroupsUsergroups   RegisterRegister 
 ProfileProfile   Log in to check your private messagesLog in to check your private messages   Log inLog in 

SO_RCVTIMEO and SO_SNDTIMEO don't affect connect() timeout

 
Post new topic   Reply to topic    miniupnp.tuxfamily.org Forum Index -> miniupnpc Bugs
View previous topic :: View next topic  
Author Message
David



Joined: 17 Feb 2014
Posts: 2

PostPosted: Mon Feb 17, 2014 9:32 am    Post subject: SO_RCVTIMEO and SO_SNDTIMEO don't affect connect() timeout Reply with quote

Hi,
Thanks for this great library. I encountered some problems and wanted to share the fixes, in case anyone runs into similar problems.

The socket options SO_RCVTIMEO and SO_SNDTIMEO don't seem to influence the timeout for the connect() call on the windows platform. This causes some UPnP calls to wait for the default timeout of 20s (in rare cases with unresponsive UPnP devices). I attached a patch below.

Code:

Changes:
--------
- Changed socket to use non-blocking mode. The socket options SO_RCVTIMEO and SO_SNDTIMEO don't
  work for the connect call. In rare cases with unresponsive UPnP devices this causes the connect
  call to wait for the default timeout of 20 seconds on the windows platform. Select is now used on
  connect/send to wait for the socket to become available. SOCKET_TIMEOUT can be specified at
  compile time, will default to 3000.
- IF_NAMESIZE and if_nametoindex are known on windows platforms starting from Vista. These are
  defined in "iphlpapi.h", which was not included in miniwget.c. This caused both scope methods to
  be used alternately, added the include to miniwget.c
- Added WINVER check When compiling for windows xp compatibility. If a version under Vista is
  specified, don't use the if_nametoindex method.

Changes are tested on windows 7 x64 and ubuntu 13.04 (in vmware). Other platforms might need some
more adaptations and testing.


Code:

diff -Naur miniupnpc-1.9/connecthostport.c miniupnpc-1.9_patched/connecthostport.c
--- miniupnpc-1.9/connecthostport.c   2013-08-01 22:21:28.000000000 -0400
+++ miniupnpc-1.9_patched/connecthostport.c   2014-02-14 13:08:50.000000000 -0500
@@ -36,6 +36,7 @@
 #include <sys/types.h>
 #include <sys/socket.h>
 #endif /* #ifndef USE_GETHOSTBYNAME */
+#include <fcntl.h>
 #endif /* #else _WIN32 */
 
 /* definition of PRINT_SOCKET_ERROR */
@@ -55,6 +56,83 @@
 #define MAXHOSTNAMELEN 64
 #endif
 
+#ifndef SOCKET_TIMEOUT
+#define SOCKET_TIMEOUT 3000
+#endif
+
+/* connectAsync()
+ * connect the specified socket asynchronously.
+ * returns 0 in case of success or -1 in case of error */
+static int connectAsync(int s, struct addrinfo *p)
+{
+   int n;
+   /* Set the socket to non-blocking mode */
+#ifdef _WIN32
+   u_long mode = 1;
+   ioctlsocket(s, FIONBIO, &mode);
+#else /* #ifdef _WIN32 */
+   int flags = fcntl(s, F_GETFL);
+   fcntl(s, F_SETFL, flags & ~O_NONBLOCK);
+#endif /* #else _WIN32 */
+   
+   n = connect(s, p->ai_addr, p->ai_addrlen);
+#ifdef _WIN32
+   if ((n < 0) && (WSAGetLastError() != WSAEWOULDBLOCK))
+   {
+      return n;
+   }
+#else /* #ifdef _WIN32 */
+   if ((n < 0) && (errno != EINPROGRESS))
+   {
+      return n;
+   }
+#endif /* #else _WIN32 */
+
+   if (n == 0)
+   {
+      /* connection attempt has succeeded immediately */
+      return n;
+   }
+   else
+   {
+      /* connection attempt is still in progress */
+       fd_set wset;
+      struct timeval timeout;
+      timeout.tv_sec = SOCKET_TIMEOUT / 1000;
+       timeout.tv_usec = (SOCKET_TIMEOUT % 1000) * 1000;
+
+       /* Wait for the socket to become writable */
+#ifdef MINIUPNPC_IGNORE_EINTR
+       do
+      {
+#endif
+         FD_ZERO(&wset);
+         FD_SET(s, &wset);
+         n = select(s + 1, NULL, &wset, NULL, &timeout);
+#ifdef MINIUPNPC_IGNORE_EINTR
+      } while(n < 0 && errno == EINTR);
+#endif
+
+      if (n < 0)
+      {
+         /* There was a socket error */
+         PRINT_SOCKET_ERROR("select");
+         return n;
+      }
+      else if (n == 0)
+      {
+         /* Timeout exceeded */
+         return -1;
+      }
+
+      if (FD_ISSET(s, &wset))
+      {
+         return 0;
+      }
+      return -1;
+   }
+}
+
 /* connecthostport()
  * return a socket connected (TCP) to the host and port
  * or -1 in case of error */
@@ -91,47 +169,25 @@
       return -1;
    }
 #ifdef MINIUPNPC_SET_SOCKET_TIMEOUT
-   /* setting a 3 seconds timeout for the connect() call */
-   timeout.tv_sec = 3;
-   timeout.tv_usec = 0;
-   if(setsockopt(s, SOL_SOCKET, SO_RCVTIMEO, &timeout, sizeof(struct timeval)) < 0)
-   {
-      PRINT_SOCKET_ERROR("setsockopt");
-   }
-   timeout.tv_sec = 3;
-   timeout.tv_usec = 0;
-   if(setsockopt(s, SOL_SOCKET, SO_SNDTIMEO, &timeout, sizeof(struct timeval)) < 0)
-   {
-      PRINT_SOCKET_ERROR("setsockopt");
+   /* setting a timeout for the connect() call */
+   /* TODO setting a timeout on the socket doesn't work for the connect() call. Now the client
+      is made to work asynchronously this code can probably be removed.*/
+   timeout.tv_sec = SOCKET_TIMEOUT / 1000;
+   timeout.tv_usec = (SOCKET_TIMEOUT % 1000) * 1000;
+   if(setsockopt(s, SOL_SOCKET, SO_RCVTIMEO, (const char *) &timeout, sizeof(struct timeval)) < 0)
+   {
+      PRINT_SOCKET_ERROR("setsockopt");
+   }
+   timeout.tv_sec = SOCKET_TIMEOUT / 1000;
+   timeout.tv_usec = (SOCKET_TIMEOUT % 1000) * 1000;
+   if(setsockopt(s, SOL_SOCKET, SO_SNDTIMEO, (const char *) &timeout, sizeof(struct timeval)) < 0)
+   {
+      PRINT_SOCKET_ERROR("setsockopt");
    }
 #endif /* #ifdef MINIUPNPC_SET_SOCKET_TIMEOUT */
    dest.sin_family = AF_INET;
    dest.sin_port = htons(port);
-   n = connect(s, (struct sockaddr *)&dest, sizeof(struct sockaddr_in));
-#ifdef MINIUPNPC_IGNORE_EINTR
-   while(n < 0 && errno == EINTR)
-   {
-      socklen_t len;
-      fd_set wset;
-      int err;
-      FD_ZERO(&wset);
-      FD_SET(s, &wset);
-      if((n = select(s + 1, NULL, &wset, NULL, NULL)) == -1 && errno == EINTR)
-         continue;
-      /*len = 0;*/
-      /*n = getpeername(s, NULL, &len);*/
-      len = sizeof(err);
-      if(getsockopt(s, SOL_SOCKET, SO_ERROR, &err, &len) < 0) {
-         PRINT_SOCKET_ERROR("getsockopt");
-         closesocket(s);
-         return -1;
-      }
-      if(err != 0) {
-         errno = err;
-         n = -1;
-      }
-   }
-#endif /* #ifdef MINIUPNPC_IGNORE_EINTR */
+   n = connectAsync(s, &dest);
    if(n<0)
    {
       PRINT_SOCKET_ERROR("connect");
@@ -187,46 +243,23 @@
          addr6->sin6_scope_id = scope_id;
       }
 #ifdef MINIUPNPC_SET_SOCKET_TIMEOUT
-      /* setting a 3 seconds timeout for the connect() call */
-      timeout.tv_sec = 3;
-      timeout.tv_usec = 0;
-      if(setsockopt(s, SOL_SOCKET, SO_RCVTIMEO, &timeout, sizeof(struct timeval)) < 0)
-      {
-         PRINT_SOCKET_ERROR("setsockopt");
-      }
-      timeout.tv_sec = 3;
-      timeout.tv_usec = 0;
-      if(setsockopt(s, SOL_SOCKET, SO_SNDTIMEO, &timeout, sizeof(struct timeval)) < 0)
-      {
-         PRINT_SOCKET_ERROR("setsockopt");
+      /* setting a timeout for the connect() call */
+      /* TODO setting a timeout on the socket doesn't work for the connect() call. Now the client
+          is made to work asynchronously this code can probably be removed.*/
+      timeout.tv_sec = SOCKET_TIMEOUT / 1000;
+      timeout.tv_usec = (SOCKET_TIMEOUT % 1000) * 1000;
+      if(setsockopt(s, SOL_SOCKET, SO_RCVTIMEO, (const char *) &timeout, sizeof(struct timeval)) < 0)
+      {
+         PRINT_SOCKET_ERROR("setsockopt");
+      }
+      timeout.tv_sec = SOCKET_TIMEOUT / 1000;
+      timeout.tv_usec = (SOCKET_TIMEOUT % 1000) * 1000;
+      if(setsockopt(s, SOL_SOCKET, SO_SNDTIMEO, (const char *) &timeout, sizeof(struct timeval)) < 0)
+      {
+         PRINT_SOCKET_ERROR("setsockopt");
       }
 #endif /* #ifdef MINIUPNPC_SET_SOCKET_TIMEOUT */
-      n = connect(s, p->ai_addr, p->ai_addrlen);
-#ifdef MINIUPNPC_IGNORE_EINTR
-      while(n < 0 && errno == EINTR)
-      {
-         socklen_t len;
-         fd_set wset;
-         int err;
-         FD_ZERO(&wset);
-         FD_SET(s, &wset);
-         if((n = select(s + 1, NULL, &wset, NULL, NULL)) == -1 && errno == EINTR)
-            continue;
-         /*len = 0;*/
-         /*n = getpeername(s, NULL, &len);*/
-         len = sizeof(err);
-         if(getsockopt(s, SOL_SOCKET, SO_ERROR, &err, &len) < 0) {
-            PRINT_SOCKET_ERROR("getsockopt");
-            closesocket(s);
-            freeaddrinfo(ai);
-            return -1;
-         }
-         if(err != 0) {
-            errno = err;
-            n = -1;
-         }
-      }
-#endif /* #ifdef MINIUPNPC_IGNORE_EINTR */
+      n = connectAsync(s, p);
       if(n < 0)
       {
          closesocket(s);
diff -Naur miniupnpc-1.9/minisoap.c miniupnpc-1.9_patched/minisoap.c
--- miniupnpc-1.9/minisoap.c   2012-01-21 15:02:20.000000000 -0500
+++ miniupnpc-1.9_patched/minisoap.c   2014-02-14 13:02:28.000000000 -0500
@@ -17,6 +17,7 @@
 #include <unistd.h>
 #include <sys/types.h>
 #include <sys/socket.h>
+#include <errno.h>
 #endif
 #include "minisoap.h"
 #include "miniupnpcstrings.h"
@@ -30,6 +31,10 @@
 #define PRINT_SOCKET_ERROR(x) perror(x)
 #endif
 
+#ifndef SOCKET_TIMEOUT
+#define SOCKET_TIMEOUT 3000
+#endif
+
 /* httpWrite sends the headers and the body to the socket
  * and returns the number of bytes sent */
 static int
@@ -49,6 +54,34 @@
      return 0;
    memcpy(p, headers, headerssize);
    memcpy(p+headerssize, body, bodysize);
+   {
+      /* Wait for the socket to become writable */
+      fd_set wset;
+      struct timeval timeout;
+      timeout.tv_sec = SOCKET_TIMEOUT / 1000;
+      timeout.tv_usec = (SOCKET_TIMEOUT % 1000) * 1000;
+
+#ifdef MINIUPNPC_IGNORE_EINTR
+      do {
+#endif
+         FD_ZERO(&wset);
+         FD_SET(fd, &wset);
+         n = select(fd + 1, NULL, &wset, NULL, &timeout);
+#ifdef MINIUPNPC_IGNORE_EINTR
+      } while(n < 0 && errno == EINTR);
+#endif
+
+      if (n < 0) {
+         /* There was a socket error */
+         PRINT_SOCKET_ERROR("select");
+         free(p);
+         return -1;
+      } else if (n == 0) {
+         /* Timeout exceeded */
+         free(p);
+         return -1;
+      }
+   }
    /*n = write(fd, p, headerssize+bodysize);*/
    n = send(fd, p, headerssize+bodysize, 0);
    if(n<0) {
diff -Naur miniupnpc-1.9/miniupnpc.c miniupnpc-1.9_patched/miniupnpc.c
--- miniupnpc-1.9/miniupnpc.c   2013-12-13 13:22:28.000000000 -0500
+++ miniupnpc-1.9_patched/miniupnpc.c   2014-02-14 15:03:54.000000000 -0500
@@ -40,6 +40,9 @@
 #endif /* defined(_MSC_VER) && (_MSC_VER >= 1400) */
 #endif /* #ifndef strncasecmp */
 #define MAXHOSTNAMELEN 64
+#if defined(WINVER) && (WINVER < 0x600)
+#undef IF_NAMESIZE
+#endif
 #else /* #ifdef _WIN32 */
 /* Standard POSIX includes */
 #include <unistd.h>
diff -Naur miniupnpc-1.9/miniwget.c miniupnpc-1.9_patched/miniwget.c
--- miniupnpc-1.9/miniwget.c   2013-10-07 11:07:44.000000000 -0400
+++ miniupnpc-1.9_patched/miniwget.c   2014-02-14 15:06:26.000000000 -0500
@@ -14,6 +14,7 @@
 #include <winsock2.h>
 #include <ws2tcpip.h>
 #include <io.h>
+#include <iphlpapi.h>
 #define MAXHOSTNAMELEN 64
 #define MIN(x,y) (((x)<(y))?(x):(y))
 #define snprintf _snprintf
@@ -25,6 +26,9 @@
 #define strncasecmp memicmp
 #endif /* defined(_MSC_VER) && (_MSC_VER >= 1400) */
 #endif /* #ifndef strncasecmp */
+#if defined(WINVER) && (WINVER < 0x600)
+#undef IF_NAMESIZE
+#endif
 #else /* #ifdef _WIN32 */
 #include <unistd.h>
 #include <sys/param.h>
@@ -38,6 +42,7 @@
 #include <arpa/inet.h>
 #include <net/if.h>
 #include <netdb.h>
+#include <errno.h>
 #define closesocket close
 /* defining MINIUPNPC_IGNORE_EINTR enable the ignore of interruptions
  * during the connect() call */
@@ -56,6 +61,10 @@
 #define MAXHOSTNAMELEN 64
 #endif
 
+#ifndef SOCKET_TIMEOUT
+#define SOCKET_TIMEOUT 3000
+#endif
+
 /*
  * Read a HTTP response from a socket.
  * Process Content-Length and Transfer-encoding headers.
@@ -374,7 +383,34 @@
    sent = 0;
    /* sending the HTTP request */
    while(sent < len)
-   {
+   {
+      /* Wait for the socket to become writable */
+      fd_set wset;
+      struct timeval timeout;
+      timeout.tv_sec = SOCKET_TIMEOUT / 1000;
+      timeout.tv_usec = (SOCKET_TIMEOUT % 1000) * 1000;
+
+#ifdef MINIUPNPC_IGNORE_EINTR
+      do {
+#endif
+         FD_ZERO(&wset);
+         FD_SET(s, &wset);
+         n = select(s + 1, NULL, &wset, NULL, &timeout);
+#ifdef MINIUPNPC_IGNORE_EINTR
+      } while(n < 0 && errno == EINTR);
+#endif
+
+      if (n < 0) {
+         /* There was a socket error */
+         perror("select");
+         closesocket(s);
+         return NULL;
+      } else if (n == 0) {
+         /* Timeout exceeded */
+         closesocket(s);
+         return NULL;
+      }
+
       n = send(s, buf+sent, len-sent, 0);
       if(n < 0)
       {
Back to top
View user's profile Send private message
miniupnp
Site Admin


Joined: 14 Apr 2007
Posts: 1469

PostPosted: Mon Feb 17, 2014 3:29 pm    Post subject: Reply with quote

hum copy/paste is a bit painful. Is your patch available somewhere ?
https://github.com/miniupnp/miniupnp ?
_________________
Main miniUPnP author.
http://miniupnp.tuxfamily.org/
Back to top
View user's profile Send private message Visit poster's website
David



Joined: 17 Feb 2014
Posts: 2

PostPosted: Tue Feb 18, 2014 3:39 pm    Post subject: Reply with quote

Sorry, completely missed the existence of the github repo. This makes things a lot easier. If everything went as planned you should have a pull request.
Back to top
View user's profile Send private message
Display posts from previous:   
Post new topic   Reply to topic    miniupnp.tuxfamily.org Forum Index -> miniupnpc Bugs All times are GMT
Page 1 of 1

 
Jump to:  
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum


Powered by phpBB © 2001, 2005 phpBB Group
Protected by Anti-Spam ACP
© 2007 Thomas Bernard, author of MiniUPNP.