Re: [Zookeeper-user] Data races in ZooKeeper C client

[Andrew K. <ak...@ya...>]

Austin,

Thanks a lot for the patch! It was a known issue and we even had a bug open
for this in our internal bug tracking system.

As Ben has suggested, could you please open a bug in the zookeeper project's
Jira here https://issues.apache.org/jira/browse/ZOOKEEPER ? Also, this patch
will break a few unit tests (specifically, the ones that test zookeeper
client initialization). Would you mind updating them so that they work with
your patch?

Thanks,
Andrew 

> -----Original Message-----
> From: zoo...@li... [mailto:zookeeper-user-
> bo...@li...] On Behalf Of Benjamin Reed
> Sent: Wednesday, July 30, 2008 8:40 AM
> To: zoo...@li...
> Subject: Re: [Zookeeper-user] Data races in ZooKeeper C client
> 
> Sorry about that and thanx for the patch!
> 
> We have moved the development process to Apache. Would you mind opening a
> Jira
> and submitting the patch through the Jira?
> 
> thank you
> ben
> 
> On Wednesday 30 July 2008 02:23:11 Austin Shoemaker wrote:
> > We were debugging a crash when using the C client on multiple threads
> > and isolated the crash to gethostbyname, which is not reentrant.
> > Replacing it with the newer getaddrinfo resolved the problem. The same
> > function was also using strtok instead of the thread-safe strtok_r.
> > The patch below incorporates both fixes.
> >
> > There may be other non-threadsafe calls that we haven't discovered-
> > let us know if you find any.
> >
> > Austin
> >
> >
> > Patch for zookeeper-c-client-2.2.1/src/zookeeper.c (2008-06-09 on
> > SF.net)
> >
> > 241c241
> > <     struct hostent *he;
> > ---
> >
> >  > 	struct addrinfo hints, *res, *res0;
> >
> > 243,245d242
> > <     struct sockaddr_in *addr4;
> > <     struct sockaddr_in6 *addr6;
> > <     char **ptr;
> > 247a245
> >
> >  > 	char *strtok_last;
> >
> > 263c261
> > <     host=strtok(hosts, ",");
> > ---
> >
> >  >     host=strtok_r(hosts, ",", &strtok_last);
> >
> > 283,294c281,297
> > <         he = gethostbyname(host);
> > <         if (!he) {
> > <             LOG_ERROR(("could not resolve %s", host));
> > <             errno=EINVAL;
> > <             rc=ZBADARGUMENTS;
> > <             goto fail;
> > <         }
> > <
> > <         /* Setup the address array */
> > <         for(ptr = he->h_addr_list;*ptr != 0; ptr++) {
> > <             if (zh->addrs_count == alen) {
> > <                 void *tmpaddr;
> > ---
> >
> >  > 		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;
> >  > 		}
> >  >
> >  > 		for (res = res0; res; res = res->ai_next) {
> >  > 			// Expand address list if needed
> >  > 			if (zh->addrs_count == alen) {
> >  > 				void *tmpaddr;
> >
> > 304,313c307,312
> > <             }
> > <             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++;
> > ---
> >
> >  > 			}
> >  >
> >  > 			// Copy addrinfo into address list
> >  > 			addr = &zh->addrs[zh->addrs_count];
> >  > 			switch (res->ai_family) {
> >  > 			case AF_INET:
> >
> > 315,320c314
> > <             } 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:
> >
> > 322,327c316,328
> > <             } else {
> > <                 LOG_WARN(("skipping unknown address family %x for %s",
> > <                         addr->sa_family, zh->hostname));
> > <             }
> > <         }
> > <         host = strtok(0, ",");
> > ---
> >
> >  > 				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;
> >  > 			}
> >  > 		}
> >  >
> >  > 		freeaddrinfo(res0);
> >  >
> >  > 		host = strtok_r(0, ",", &strtok_last);
> >
> > 329a331
> >
> >
> >
> > ------------------------------------------------------------------------
> -
> > This SF.Net email is sponsored by the Moblin Your Move Developer's
> > challenge Build the coolest Linux based applications with Moblin SDK &
> win
> > great prizes Grand prize is a trip for two to an Open Source event
> anywhere
> > in the world http://moblin-contest.org/redirect.php?banner_id=100&url=/
> > _______________________________________________
> > Zookeeper-user mailing list
> > Zoo...@li...
> > https://lists.sourceforge.net/lists/listinfo/zookeeper-user
> 
> 
> 
> -------------------------------------------------------------------------
> This SF.Net email is sponsored by the Moblin Your Move Developer's
> challenge
> Build the coolest Linux based applications with Moblin SDK & win great
> prizes
> Grand prize is a trip for two to an Open Source event anywhere in the
> world
> http://moblin-contest.org/redirect.php?banner_id=100&url=/
> _______________________________________________
> Zookeeper-user mailing list
> Zoo...@li...
> https://lists.sourceforge.net/lists/listinfo/zookeeper-user