ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-530

Memory corruption: Zookeeper c client IPv6 implementation does not honor struct sockaddr_in6 size

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.2.0, 3.2.1
    • Fix Version/s: 3.3.0
    • Component/s: c client
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      get the c client working with IPV6.
    • Tags:
      ipv6

      Description

      I tried to run zookeeper c-client on a machine with IPv6 enabled. When connecting to the IPv6 address a connect(...) gave a "Address family not supported by protocol" error. The reason was, that a few lines earlier, the socket was opened with PF_INET instead of PF_INET6. Changing that the following way:

                 if (zh->addrs[zh->connect_index].sa_family == AF_INET) {
                  	zh->fd = socket(PF_INET, SOCK_STREAM, 0);
                  } else {
                  	zh->fd = socket(PF_INET6, SOCK_STREAM, 0);
                  }
      

      turned the error message into "Invalid argument".

      When printing out sizeof(struct sockaddr), sizeof(struct sockaddr_in) and sizeof(struct sockaddr_in6) I got sockaddr: 16, sockaddr_in: 16 and sockaddr_in6: 28.

      So in the code calling

                 connect(zh->fd, &zh->addrs[zh->connect_index], sizeof(struct sockaddr_in));
      

      the parameter address_len is too small.

      Same applies to how IPv6 addresses are handled in the function getaddrs(zhandle_t *zh).

      (Big Thanks+kiss to Thilo Fromm for helping me debug this.)

      1. ZOOKEEPER-530_spaces.patch
        4 kB
        Mahadev konar
      2. ZOOKEEPER-530.patch
        4 kB
        Isabel Drost-Fromm
      3. ZOOKEEPER-530.patch
        4 kB
        Isabel Drost-Fromm

        Activity

        Hide
        Isabel Drost-Fromm added a comment -

        Just found with man 7 ipv6:

        NOTES
        The sockaddr_in6 structure is bigger than the generic sockaddr. Programs that assume that all address types can be stored safely in a struct sockaddr need to be changed to use struct sockaddr_storage for that instead.

        Show
        Isabel Drost-Fromm added a comment - Just found with man 7 ipv6: NOTES The sockaddr_in6 structure is bigger than the generic sockaddr. Programs that assume that all address types can be stored safely in a struct sockaddr need to be changed to use struct sockaddr_storage for that instead.
        Hide
        Isabel Drost-Fromm added a comment -

        These changes fix the problem for me.

        Show
        Isabel Drost-Fromm added a comment - These changes fix the problem for me.
        Hide
        Patrick Hunt added a comment -

        Thanks for the patch!

        Show
        Patrick Hunt added a comment - Thanks for the patch!
        Hide
        Patrick Hunt added a comment -

        Isabel, is this ready for review? Please click the "submit patch" link if so.

        Also, is there some test we can add for this? Or we just need to run the existing tests on IPv6?

        Show
        Patrick Hunt added a comment - Isabel, is this ready for review? Please click the "submit patch" link if so. Also, is there some test we can add for this? Or we just need to run the existing tests on IPv6?
        Hide
        Isabel Drost-Fromm added a comment -

        As explained above, I just tried to use the c-client on a 64bit system with IPv6 support (SuSE 10.3), when connecting to the server I got the errors from above - not consistently though: Whenever the client decided to connect to the IPv4 address all would work fine. So running the existing tests on a IPv6 only, 64bit system should do the trick, I do not know what a test that checks exactly that scenario might look like.

        Show
        Isabel Drost-Fromm added a comment - As explained above, I just tried to use the c-client on a 64bit system with IPv6 support (SuSE 10.3), when connecting to the server I got the errors from above - not consistently though: Whenever the client decided to connect to the IPv4 address all would work fine. So running the existing tests on a IPv6 only, 64bit system should do the trick, I do not know what a test that checks exactly that scenario might look like.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12420179/ZOOKEEPER-530.patch
        against trunk revision 824981.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no tests are needed for this patch.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/26/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/26/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/26/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12420179/ZOOKEEPER-530.patch against trunk revision 824981. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/26/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/26/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/26/console This message is automatically generated.
        Hide
        Mahadev konar added a comment -

        Isabel, the patch looks good. thanks for the fix.

        One thing I noticed in the patch is this -

        	zh->fd = socket(zh->addrs[zh->connect_index].ss_family, SOCK_STREAM, 0);
        			if (zh->fd < 0) {
        				return api_epilog(zh,handle_socket_error_msg(zh,__LINE__,
        				            ZCONNECTIONLOSS,"connect() call failed"));
        			}
        

        there is a new error check, which is a good thing, but can the error be changed to ZSYSTEMERROR, the log message to socket() call failed?

        Show
        Mahadev konar added a comment - Isabel, the patch looks good. thanks for the fix. One thing I noticed in the patch is this - zh->fd = socket(zh->addrs[zh->connect_index].ss_family, SOCK_STREAM, 0); if (zh->fd < 0) { return api_epilog(zh,handle_socket_error_msg(zh,__LINE__, ZCONNECTIONLOSS, "connect() call failed" )); } there is a new error check, which is a good thing, but can the error be changed to ZSYSTEMERROR, the log message to socket() call failed?
        Hide
        Isabel Drost-Fromm added a comment -

        Changed error code and error message.

        Show
        Isabel Drost-Fromm added a comment - Changed error code and error message.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12422653/ZOOKEEPER-530.patch
        against trunk revision 826787.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no tests are needed for this patch.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/34/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/34/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/34/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12422653/ZOOKEEPER-530.patch against trunk revision 826787. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/34/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/34/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/34/console This message is automatically generated.
        Hide
        Mahadev konar added a comment -

        +1 the patch looks good....

        Show
        Mahadev konar added a comment - +1 the patch looks good....
        Hide
        Mahadev konar added a comment -

        one minor change ... there were tabs in the patch . I just changed it to spaces... will commit it...

        Show
        Mahadev konar added a comment - one minor change ... there were tabs in the patch . I just changed it to spaces... will commit it...
        Hide
        Mahadev konar added a comment -

        I just committed this.

        Show
        Mahadev konar added a comment - I just committed this.
        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #505 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/505/)
        . Memory corruption: Zookeeper c client IPv6 implementation does not honor struct sockaddr_in6 size (isabel drost via mahadev)

        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #505 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/505/ ) . Memory corruption: Zookeeper c client IPv6 implementation does not honor struct sockaddr_in6 size (isabel drost via mahadev)

          People

          • Assignee:
            Isabel Drost-Fromm
            Reporter:
            Isabel Drost-Fromm
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development