[PATCH] Fix iscsi_connect_async() that sometimes blocks indefinitely

[saxen...@gmail.com]

iscsi_connect_async() calls getaddrinfo(), which sometimes blocks indefinitely.
Ref : http://lists.busybox.net/pipermail/busybox/2012-July/078123.html

This patch removes call to getaddrinfo() so that iscsi_connect_async() is truly
nonblocking.

(I've attached the patch, copy-pasting it inline below for quick browsing )

iscsi_connect_async() calls getaddrinfo(), which sometimes blocks indefinitely.
Ref : http://lists.busybox.net/pipermail/busybox/2012-July/078123.html

This patch removes call to getaddrinfo() so that iscsi_connect_async() is truly
nonblocking.

Signed-off-by: Prerna Saxena <saxen...@gmail.com>
---
 lib/socket.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/lib/socket.c b/lib/socket.c
index a7b0a2f..16adbfe 100644
--- a/lib/socket.c
+++ b/lib/socket.c
@@ -245,12 +245,6 @@ iscsi_connect_async(struct iscsi_context *iscsi, const char *portal,
        }

        /* is it a hostname ? */
-       if (getaddrinfo(host, NULL, NULL, &ai) != 0) {
-               iscsi_free(iscsi, addr);
-               iscsi_set_error(iscsi, "Invalid target:%s  "
-                       "Can not resolv into IPv4/v6.", portal);
-               return -1;
-       }
        iscsi_free(iscsi, addr);

        memset(&sa, 0, sizeof(sa));
--
1.7.1

[ronnie sahlberg]

Hi,

I don't think that patch can work. Because if you remove the call to
getaddrinfo() there is no
longer anything that converts the string into an ip address/structure
and you would end up
further down trying to dereference ai (which is NULL).


But If what we pass to getaddrinfo() is an ipv4/ipv6 address it
should never block.
It is only if we pass a hostname there and need to rely on the local
resolver to convert into an address it can block.


Since there is no standard async resolver here, we need to rely on
getaddrinfo() at least to parse the address into an ai structure.

The right thing here is probably to document in the header something like
...
iscsi_connect_async() calls getaddrinfo() to resolve the string into
an ip address. This call is only
non-blocking if you pass a string containing an ip address to it.
If you pass a hostname that needs to be looked up, then this function
will block while the local resolver
will try to do the host lookup. This may block for an extended period
in some configurations/environments.

If you require that iscsi_connect_async() to be non-blocking, you can
not call it with a hostname as argument but will have to convert it
into an ipv4/v6 string before, using what async name resolution
libraries are available on your platform.
...

or something.


Can you create a documentation patch for the header to document this ?


regards
ronnie sahlberg

[Prerna]

Hi Ronnie,

Thank you for the quick response.

Apologies, this was not a complete fix. However, I was thinking : can we attempt an 'inet_pton' on the host IPv4 address string first, in lib/socket.c ? In this case, we call getaddrinfo() only if the string is a hostname and needs explicit name resolution. This leaves lesser scope for potential blocking calls.

I have attached another patch on these lines. If this is good, I'll add a documentation snippet too.

 [PATCH][v2] Fix iscsi_connect_async() that sometimes blocks
 indefinitely.

iscsi_connect_async() calls getaddrinfo(), which sometimes blocks
indefinitely

Ref : http://lists.busybox.net/pipermail/busybox/2012-July/078123.html

This patch introduces a call to inet_pton before getaddrinfo(). Now,
iscsi_connect_async() only calls (and potentially blocks on) getaddrinfo()
for explicit domain/host names. 'Hosts' specified as IP addresses bypass
getaddrinfo and are truly nonblocking.

Signed-off-by: Prerna Saxena <prerna...@nutanix.com>
---
 lib/socket.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/lib/socket.c b/lib/socket.c
index a7b0a2f..1524d8c 100644
--- a/lib/socket.c
+++ b/lib/socket.c
@@ -244,12 +244,15 @@ iscsi_connect_async(struct iscsi_context *iscsi, const char *portal,
         *str = 0;
     }
 
-    /* is it a hostname ? */

-    if (getaddrinfo(host, NULL, NULL, &ai) != 0) {
-        iscsi_free(iscsi, addr);
-        iscsi_set_error(iscsi, "Invalid target:%s  "
-            "Can not resolv into IPv4/v6.", portal);
-        return -1;

+    /* Avoid getaddrinfo if the 'host' is an IPv4 address */
+    if (inet_pton(AF_INET, host, (void*)&ai) <= 0) {
+        /* is it a hostname ? */
+        if (getaddrinfo(host, NULL, NULL, &ai) != 0) {
+            iscsi_free(iscsi, addr);
+            iscsi_set_error(iscsi, "Invalid target:%s  "
+                "Can not resolv into IPv4/v6.", portal);
+            return -1;
+        }
      }
     iscsi_free(iscsi, addr);

[saxen...@gmail.com]

I havent clarified why this fix is needed in libiscsi, here is an explanantion :
You mentioned that:  If what we pass to getaddrinfo() is an ipv4/ipv6 address it should never block. That indeed is correct under normal circumstances.
However, there has been a linux kernel regression from 3.17 all the way till 4.0.9, wherein changes in netlink layer cause getaddrinfo() calls to be blocked forever
[ Ref : https://bugzilla.kernel.org/show_bug.cgi?id=99671 ]
This patch is an attempt for libiscsi to circumvent the getaddrinfo() call. Hope this explains the rationale behind this fix.