You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2020/02/13 17:29:14 UTC
[GitHub] [zookeeper] vtyulb opened a new pull request #1252: ZOOKEEPER-3726:
invalid ipv6 address comparison
vtyulb opened a new pull request #1252: ZOOKEEPER-3726: invalid ipv6 address comparison
URL: https://github.com/apache/zookeeper/pull/1252
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] vtyulb commented on issue #1252: ZOOKEEPER-3726:
invalid ipv6 address comparison
Posted by GitBox <gi...@apache.org>.
vtyulb commented on issue #1252: ZOOKEEPER-3726: invalid ipv6 address comparison
URL: https://github.com/apache/zookeeper/pull/1252#issuecomment-592744082
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] symat commented on issue #1252: ZOOKEEPER-3726: invalid
ipv6 address comparison
Posted by GitBox <gi...@apache.org>.
symat commented on issue #1252: ZOOKEEPER-3726: invalid ipv6 address comparison
URL: https://github.com/apache/zookeeper/pull/1252#issuecomment-595456556
agree, and I also like your unit tests, thanks for taking the time to implement them! :)
@anmolnar, @nkalmar, @eolivelli PTAL
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] nkalmar commented on issue #1252: ZOOKEEPER-3726:
invalid ipv6 address comparison
Posted by GitBox <gi...@apache.org>.
nkalmar commented on issue #1252: ZOOKEEPER-3726: invalid ipv6 address comparison
URL: https://github.com/apache/zookeeper/pull/1252#issuecomment-591447169
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] vtyulb commented on issue #1252: ZOOKEEPER-3726:
invalid ipv6 address comparison
Posted by GitBox <gi...@apache.org>.
vtyulb commented on issue #1252: ZOOKEEPER-3726: invalid ipv6 address comparison
URL: https://github.com/apache/zookeeper/pull/1252#issuecomment-612332075
please don't lose my PR @symat @nkalmar
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] vtyulb commented on issue #1252: ZOOKEEPER-3726:
invalid ipv6 address comparison
Posted by GitBox <gi...@apache.org>.
vtyulb commented on issue #1252: ZOOKEEPER-3726: invalid ipv6 address comparison
URL: https://github.com/apache/zookeeper/pull/1252#issuecomment-586880305
@eolivelli @nkalmar
Hi guys, can you please review changes, or may be point me to some required actions?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] vtyulb commented on issue #1252: ZOOKEEPER-3726:
invalid ipv6 address comparison
Posted by GitBox <gi...@apache.org>.
vtyulb commented on issue #1252: ZOOKEEPER-3726: invalid ipv6 address comparison
URL: https://github.com/apache/zookeeper/pull/1252#issuecomment-595398781
@symat 7 years have passed since bug discovery, I think it's time to merge the fix.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] asfgit closed pull request #1252: ZOOKEEPER-3726:
invalid ipv6 address comparison
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1252: ZOOKEEPER-3726: invalid ipv6 address comparison
URL: https://github.com/apache/zookeeper/pull/1252
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] symat commented on issue #1252: ZOOKEEPER-3726: invalid
ipv6 address comparison
Posted by GitBox <gi...@apache.org>.
symat commented on issue #1252: ZOOKEEPER-3726: invalid ipv6 address comparison
URL: https://github.com/apache/zookeeper/pull/1252#issuecomment-612334390
Sure, thanks for the notification! I'll merge it on Tuesday, if noone else does before.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] vtyulb commented on issue #1252: ZOOKEEPER-3726:
invalid ipv6 address comparison
Posted by GitBox <gi...@apache.org>.
vtyulb commented on issue #1252: ZOOKEEPER-3726: invalid ipv6 address comparison
URL: https://github.com/apache/zookeeper/pull/1252#issuecomment-612346574
Thank you!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] symat commented on issue #1252: ZOOKEEPER-3726: invalid
ipv6 address comparison
Posted by GitBox <gi...@apache.org>.
symat commented on issue #1252: ZOOKEEPER-3726: invalid ipv6 address comparison
URL: https://github.com/apache/zookeeper/pull/1252#issuecomment-591917686
LGTM in general, nice catch!
We already have similar logics here: https://github.com/apache/zookeeper/blob/db87335fd2593cacc49bc23c1b9065256d0d3d36/zookeeper-client/zookeeper-client-c/src/zookeeper.c#L4957-L4992
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] symat commented on issue #1252: ZOOKEEPER-3726: invalid
ipv6 address comparison
Posted by GitBox <gi...@apache.org>.
symat commented on issue #1252: ZOOKEEPER-3726: invalid ipv6 address comparison
URL: https://github.com/apache/zookeeper/pull/1252#issuecomment-612343144
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] nkalmar commented on a change in pull request #1252:
ZOOKEEPER-3726: invalid ipv6 address comparison
Posted by GitBox <gi...@apache.org>.
nkalmar commented on a change in pull request #1252: ZOOKEEPER-3726: invalid ipv6 address comparison
URL: https://github.com/apache/zookeeper/pull/1252#discussion_r384516704
##########
File path: zookeeper-client/zookeeper-client-c/src/addrvec.c
##########
@@ -126,8 +126,26 @@ int addrvec_contains(const addrvec_t *avec, const struct sockaddr_storage *addr)
for (i = 0; i < avec->count; i++)
{
- if(memcmp(&avec->data[i], addr, INET_ADDRSTRLEN) == 0)
- return 1;
+ if (avec->data[i].ss_family != addr->ss_family)
+ continue;
+ switch (addr->ss_family) {
+ case AF_INET:
+ if (memcmp(&((struct sockaddr_in*)&avec->data[i])->sin_addr,
+ &((struct sockaddr_in*)addr)->sin_addr,
+ sizeof(struct in_addr)) == 0)
+ return 1;
+ break;
+#ifdef AF_INET6
Review comment:
Shouldn't we add #ifndef AF_INET6 before the previous if?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] symat commented on issue #1252: ZOOKEEPER-3726: invalid
ipv6 address comparison
Posted by GitBox <gi...@apache.org>.
symat commented on issue #1252: ZOOKEEPER-3726: invalid ipv6 address comparison
URL: https://github.com/apache/zookeeper/pull/1252#issuecomment-612351224
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! :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] vtyulb commented on a change in pull request #1252:
ZOOKEEPER-3726: invalid ipv6 address comparison
Posted by GitBox <gi...@apache.org>.
vtyulb commented on a change in pull request #1252: ZOOKEEPER-3726: invalid ipv6 address comparison
URL: https://github.com/apache/zookeeper/pull/1252#discussion_r384690997
##########
File path: zookeeper-client/zookeeper-client-c/src/addrvec.c
##########
@@ -126,8 +126,26 @@ int addrvec_contains(const addrvec_t *avec, const struct sockaddr_storage *addr)
for (i = 0; i < avec->count; i++)
{
- if(memcmp(&avec->data[i], addr, INET_ADDRSTRLEN) == 0)
- return 1;
+ if (avec->data[i].ss_family != addr->ss_family)
+ continue;
+ switch (addr->ss_family) {
+ case AF_INET:
+ if (memcmp(&((struct sockaddr_in*)&avec->data[i])->sin_addr,
+ &((struct sockaddr_in*)addr)->sin_addr,
+ sizeof(struct in_addr)) == 0)
+ return 1;
+ break;
+#ifdef AF_INET6
Review comment:
I think it's possible. However, it adds several lines and it duplicates comparison logic. In AF_INET case I fully cover ipv4 address type.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services