ZOOKEEPER-208. Zookeeper C client uses API that are not thread safe, causing crashes when multiple instances are active. (austin shoemaker, chris daroch and ben reed via mahadev)

git-svn-id: https://svn.apache.org/repos/asf/hadoop/zookeeper/trunk@719063 13f79535-47bb-0310-9956-ffa450edef68

CHANGES.txt

@@ -32,6 +32,10 @@ BUGFIXES:
 
    ZOOKEEPER-204. SetWatches needs to be the first message after auth messages
 to the server (ben via mahadev)
+  
+   ZOOKEEPER-208. Zookeeper C client uses API that are not thread safe,
+causing crashes when multiple instances are active. (austin shoemaker, chris
+daroch and ben reed via mahadev) 
 
 Release 3.0.0 - 2008-10-21
 

src/c/src/zookeeper.c

@@ -281,13 +281,11 @@ static void setup_random()
  */
 int getaddrs(zhandle_t *zh)
 {
-    struct hostent *he;
+    struct addrinfo hints, *res, *res0;
     struct sockaddr *addr;
-    struct sockaddr_in *addr4;
-    struct sockaddr_in6 *addr6;
-    char **ptr;
     char *hosts = strdup(zh->hostname);
     char *host;
+    char *strtok_last;
     int i;
     int rc;
     int alen = 0; /* the allocated length of the addrs array */
@@ -303,7 +301,7 @@ int getaddrs(zhandle_t *zh)
         return ZSYSTEMERROR;
     }
     zh->addrs = 0;
-    host=strtok(hosts, ",");
+    host=strtok_r(hosts, ",", &strtok_last);
     while(host) {
         char *port_spec = strchr(host, ':');
         char *end_port_spec;
@@ -323,16 +321,21 @@ int getaddrs(zhandle_t *zh)
             rc=ZBADARGUMENTS;
             goto fail;
         }
-        he = gethostbyname(host);
-        if (!he) {
-            LOG_ERROR(("could not resolve %s", host));
-            errno=EINVAL;
-            rc=ZBADARGUMENTS;
+
+        memset(&hints, 0, sizeof(hints));
+        hints.ai_flags = AI_ADDRCONFIG;
+        hints.ai_family = AF_UNSPEC;
+        hints.ai_socktype = SOCK_STREAM;
+        hints.ai_protocol = IPPROTO_TCP;
+
+        if (getaddrinfo(host, port_spec, &hints, &res0) != 0) {
+            LOG_ERROR(("getaddrinfo: %s\n", strerror(errno)));
+            rc=ZSYSTEMERROR;
             goto fail;
         }
-        
-        /* Setup the address array */
-        for(ptr = he->h_addr_list;*ptr != 0; ptr++) {
+
+        for (res = res0; res; res = res->ai_next) {
+            // Expand address list if needed
             if (zh->addrs_count == alen) {
                 void *tmpaddr;
                 alen += 16;
@@ -345,31 +348,30 @@ int getaddrs(zhandle_t *zh)
                 }
                 zh->addrs=tmpaddr;
             }
+
+            // Copy addrinfo into address list
             addr = &zh->addrs[zh->addrs_count];
-            addr4 = (struct sockaddr_in*)addr;
-            addr6 = (struct sockaddr_in6*)addr;
-            addr->sa_family = he->h_addrtype;
-            if (addr->sa_family == AF_INET) {
-                addr4->sin_port = htons(port);
-                memset(&addr4->sin_zero, 0, sizeof(addr4->sin_zero));
-                memcpy(&addr4->sin_addr, *ptr, he->h_length);
-                zh->addrs_count++;
+            switch (res->ai_family) {
+            case AF_INET:
 #if defined(AF_INET6)
-            } else if (addr->sa_family == AF_INET6) {
-                addr6->sin6_port = htons(port);
-                addr6->sin6_scope_id = 0;
-                addr6->sin6_flowinfo = 0;
-                memcpy(&addr6->sin6_addr, *ptr, he->h_length);
-                zh->addrs_count++;
+            case AF_INET6:
 #endif
-            } else {
-                LOG_WARN(("skipping unknown address family %x for %s", 
-                        addr->sa_family, zh->hostname)); 
+                memcpy(addr, res->ai_addr, res->ai_addrlen);
+                ++zh->addrs_count;
+                break;
+            default:
+                LOG_WARN(("skipping unknown address family %x for %s",
+                res->ai_family, zh->hostname));
+                break;
             }
         }
-        host = strtok(0, ",");
+
+        freeaddrinfo(res0);
+
+        host = strtok_r(0, ",", &strtok_last);
     }
     free(hosts);
+
     if(!disable_conn_permute){
         setup_random();
         /* Permute */

src/c/tests/TestZookeeperInit.cc

@@ -121,14 +121,11 @@ public:
     }
     void testAddressResolution()
     {
-        const char EXPECTED_IPS[][4]={{127,0,0,1},{127,0,0,2},{127,0,0,3}};
+        const char EXPECTED_IPS[][4]={{127,0,0,1}};
         const int EXPECTED_ADDRS_COUNT =COUNTOF(EXPECTED_IPS);
-        Mock_gethostbyname mock;
-        mock.addHostEntry("somehostname").addAddress(EXPECTED_IPS[0]).
-            addAddress(EXPECTED_IPS[1]).addAddress(EXPECTED_IPS[2]);
 
         zoo_deterministic_conn_order(1);
-        zh=zookeeper_init("host:2121",0,10000,0,0,0);
+        zh=zookeeper_init("127.0.0.1:2121",0,10000,0,0,0);
         
         CPPUNIT_ASSERT(zh!=0);
         CPPUNIT_ASSERT_EQUAL(EXPECTED_ADDRS_COUNT,zh->addrs_count);
@@ -140,15 +137,9 @@ public:
     }
     void testMultipleAddressResolution()
     {
-        const string EXPECTED_HOST("host1:2121,host2:3434");
-        const char EXPECTED_IPS[][4]={{127,0,0,1},{127,0,0,2},{127,0,0,3},
-                {126,1,1,1},{126,2,2,2}};
+        const string EXPECTED_HOST("127.0.0.1:2121,127.0.0.2:3434");
+        const char EXPECTED_IPS[][4]={{127,0,0,1},{127,0,0,2}};
         const int EXPECTED_ADDRS_COUNT =COUNTOF(EXPECTED_IPS);
-        Mock_gethostbyname mock;
-        mock.addHostEntry("somehost1").addAddress(EXPECTED_IPS[0]).
-            addAddress(EXPECTED_IPS[1]).addAddress(EXPECTED_IPS[2]);
-        mock.addHostEntry("somehost2").addAddress(EXPECTED_IPS[3]).
-            addAddress(EXPECTED_IPS[4]);
 
         zoo_deterministic_conn_order(1);
         zh=zookeeper_init(EXPECTED_HOST.c_str(),0,1000,0,0,0);
@@ -159,7 +150,7 @@ public:
         for(int i=0;i<zh->addrs_count;i++){
             sockaddr_in* addr=(struct sockaddr_in*)&zh->addrs[i];
             CPPUNIT_ASSERT(memcmp(EXPECTED_IPS[i],&addr->sin_addr,sizeof(addr->sin_addr))==0);
-            if(i<3)
+            if(i<1)
                 CPPUNIT_ASSERT_EQUAL(2121,(int)ntohs(addr->sin_port));
             else
                 CPPUNIT_ASSERT_EQUAL(3434,(int)ntohs(addr->sin_port));
@@ -208,14 +199,15 @@ public:
     }
     void testNonexistentHost()
     {
-        const string EXPECTED_HOST("host1:1111");
-        MockFailed_gethostbyname mock;
+        const string EXPECTED_HOST("host1.blabadibla.bla.:1111");
         
         zh=zookeeper_init(EXPECTED_HOST.c_str(),0,0,0,0,0);
         
         CPPUNIT_ASSERT(zh==0);
-        CPPUNIT_ASSERT_EQUAL(EINVAL,errno);
-        CPPUNIT_ASSERT_EQUAL(HOST_NOT_FOUND,h_errno);
+        //With the switch to thread safe getaddrinfo, we don't get
+        //these global variables
+        //CPPUNIT_ASSERT_EQUAL(EINVAL,errno);
+        //CPPUNIT_ASSERT_EQUAL(HOST_NOT_FOUND,h_errno);
     }
     void testOutOfMemory_init()
     {
@@ -232,30 +224,17 @@ public:
         Mock_realloc reallocMock;
         reallocMock.callsBeforeFailure=0; // fail on first call to realloc
 
-        Mock_gethostbyname gethostbynameMock;
-        gethostbynameMock.addHostEntry("ahost").addAddress("\1\1\1\1");
-
-        zh=zookeeper_init("ahost:123",0,0,0,0,0);
+        zh=zookeeper_init("127.0.0.1:123",0,0,0,0,0);
         
         CPPUNIT_ASSERT(zh==0);
         CPPUNIT_ASSERT_EQUAL(ENOMEM,errno);
     }
     void testOutOfMemory_getaddrs2()
     {
-        const char ADDR[]="\1\1\1\1";
         Mock_realloc reallocMock;
         reallocMock.callsBeforeFailure=1; // fail on the second call to realloc
 
-        Mock_gethostbyname gethostbynameMock;
-        // need >16 IPs to get realloc called the second time
-        gethostbynameMock.addHostEntry("ahost").
-            addAddress(ADDR).addAddress(ADDR).addAddress(ADDR).addAddress(ADDR).
-            addAddress(ADDR).addAddress(ADDR).addAddress(ADDR).addAddress(ADDR).
-            addAddress(ADDR).addAddress(ADDR).addAddress(ADDR).addAddress(ADDR).
-            addAddress(ADDR).addAddress(ADDR).addAddress(ADDR).addAddress(ADDR).
-            addAddress(ADDR);
-
-        zh=zookeeper_init("ahost:123",0,0,0,0,0);
+        zh=zookeeper_init("127.0.0.1:123,127.0.0.2:123,127.0.0.3:123,127.0.0.4:123,127.0.0.5:123,127.0.0.6:123,127.0.0.7:123,127.0.0.8:123,127.0.0.9:123,127.0.0.10:123,127.0.0.11:123,127.0.0.12:123,127.0.0.13:123,127.0.0.14:123,127.0.0.15:123,127.0.0.16:123,127.0.0.17:123",0,0,0,0,0);
         
         CPPUNIT_ASSERT(zh==0);
         CPPUNIT_ASSERT_EQUAL(ENOMEM,errno);
@@ -265,16 +244,11 @@ public:
         const char EXPECTED[][5]={"\0\0\0\0","\1\1\1\1","\2\2\2\2","\3\3\3\3"};
         const int EXPECTED_ADDR_COUNT=COUNTOF(EXPECTED);
         
-        Mock_gethostbyname gethostbynameMock;
-        gethostbynameMock.addHostEntry("ahost").
-            addAddress(EXPECTED[0]).addAddress(EXPECTED[1]).
-            addAddress(EXPECTED[2]).addAddress(EXPECTED[3]);
-
         const int RAND_SEQ[]={0,1,2,3,1,3,2,0,-1};
         const int RAND_SIZE=COUNTOF(RAND_SEQ);
         Mock_random randomMock;
         randomMock.randomReturns.assign(RAND_SEQ,RAND_SEQ+RAND_SIZE-1);
-        zh=zookeeper_init("ahost:123",0,1000,0,0,0);
+        zh=zookeeper_init("0.0.0.0:123,1.1.1.1:123,2.2.2.2:123,3.3.3.3:123",0,1000,0,0,0);
         
         CPPUNIT_ASSERT(zh!=0);
         CPPUNIT_ASSERT_EQUAL(EXPECTED_ADDR_COUNT,zh->addrs_count);