ZOOKEEPER-3726: invalid ipv6 address comparison

[vtyulb]

Fix for https://issues.apache.org/jira/browse/ZOOKEEPER-3726

sockaddr_storage can contain ipv4 or ipv6 address. If address is ipv6, then we need to compare more bytes.

In this PR correct comparison of sockaddr_storage was added.

[vtyulb]

@eolivelli @nkalmar

Hi guys, can you please review changes, or may be point me to some required actions?

[nkalmar]

LGTM, I have one question which I'm not sure about at all, just seems logical to me. (See my code comment)

Anyway, I would need someone else to also take a look, perhaps @symat who has more C experience.

[symat]

LGTM in general, nice catch!

We already have similar logics here:

zookeeper/zookeeper-client/zookeeper-client-c/src/zookeeper.c

Lines 4957 to 4992 in db87335

	static const char* format_endpoint_info(const struct sockaddr_storage* ep)
	{
	static char buf[128] = { 0 };
	char addrstr[INET6_ADDRSTRLEN] = { 0 };
	const char *fmtstring;
	void *inaddr;
	char is_inet6 = 0; // poor man's boolean
	#ifdef _WIN32
	char * addrstring;
	#endif
	int port;
	if(ep==0)
	return "null";
	#if defined(AF_INET6)
	if(ep->ss_family==AF_INET6){
	inaddr=&((struct sockaddr_in6*)ep)->sin6_addr;
	port=((struct sockaddr_in6*)ep)->sin6_port;
	is_inet6 = 1;
	} else {
	#endif
	inaddr=&((struct sockaddr_in*)ep)->sin_addr;
	port=((struct sockaddr_in*)ep)->sin_port;
	#if defined(AF_INET6)
	}
	#endif
	fmtstring = (is_inet6 ? "[%s]:%d" : "%s:%d");
	#ifdef _WIN32
	addrstring = inet_ntoa (*(struct in_addr*)inaddr);
	sprintf(buf,fmtstring,addrstring,ntohs(port));
	#else
	inet_ntop(ep->ss_family,inaddr,addrstr,sizeof(addrstr)-1);
	sprintf(buf,fmtstring,addrstr,ntohs(port));
	#endif
	return buf;
	}

It would be great to add a unit test for it (both for IPv4 and IPv6), but it seems to be a complicated issue to reproduce with unit tests. Please think about it. But I am OK to commit this without tests as well.

[vtyulb]

Well, for sure it's almost impossible to reproduce full problem behavior. But it's possible to show how new addrvec_contains differs from the old one. In second commit I generate all addresses that differ by one bit from source. These tests can't pass on previous implementation.

Also, I found 2 duplicate tickets for my issue:
https://issues.apache.org/jira/browse/ZOOKEEPER-1677
https://issues.apache.org/jira/browse/ZOOKEEPER-2490

Both tickets have patches attached. ZOOKEEPER-2490 is not very elegant, but ZOOKEEPER-1677 even had tests (on which mine are based now). I am not really sure why they weren't merged, it looks like I am not the first one to encounter the bug.

[vtyulb]

@symat 7 years have passed since bug discovery, I think it's time to merge the fix.

[symat]

agree, and I also like your unit tests, thanks for taking the time to implement them! :)
@anmolnar, @nkalmar, @eolivelli PTAL

[nkalmar]

Okay, LGTM!

[vtyulb]

please don't lose my PR @symat @nkalmar

[symat]

Sure, thanks for the notification! I'll merge it on Tuesday, if noone else does before.

[symat]

The kids woke up late, I think I can merge it now :)
I will push for master and branch-3.6, and also check it if I can also push on branch-3.5 without conflict.

[vtyulb]

Thank you!

[symat]

I compiled and re-executed the C client tests on all branches (as this commit was tested by the CI a long time ago), everything seemed to be fine.

@vtyulb Thanks again for your help and your patience! :)