ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1398

zkpython corrupts session passwords that contain nulls

    Details

    • Type: Bug Bug
    • Status: Patch Available
    • Priority: Critical Critical
    • Resolution: Unresolved
    • Affects Version/s: 3.3.4
    • Fix Version/s: None
    • Component/s: c client, contrib-bindings
    • Labels:
      None

      Description

      If the session password contains a nul character (\0), it will be mutated as it is passed to python. zkpython currently uses the ParseArgs flag that stops on nul.

        Activity

        Hide
        Mike Lundy added a comment -

        In writing the test, I discovered the real problem: zookeeper-c treats the session password as a \0-terminated string, but the java generates the session password as a 16-byte binary blob, where \0 is data. Some ideas for fixing that are 1) break the c api and add a passwd_len to the clientid_t or 2) change ZooKeeperServer.generatePasswd to not make \0. Which is better is up to you, I think.

        Currently everything that uses the C api and does not pass in a session password will randomly break if they expect session passwords to work (since the password will be truncated to the null every time it crosses the C<=>Python interface). I would expect the perl api has the same problem. With this patch, the password will not be truncated (though the tradeoff is you must pass a 16-byte string from python).

        Show
        Mike Lundy added a comment - In writing the test, I discovered the real problem: zookeeper-c treats the session password as a \0-terminated string, but the java generates the session password as a 16-byte binary blob, where \0 is data. Some ideas for fixing that are 1) break the c api and add a passwd_len to the clientid_t or 2) change ZooKeeperServer.generatePasswd to not make \0. Which is better is up to you, I think. Currently everything that uses the C api and does not pass in a session password will randomly break if they expect session passwords to work (since the password will be truncated to the null every time it crosses the C<=>Python interface). I would expect the perl api has the same problem. With this patch, the password will not be truncated (though the tradeoff is you must pass a 16-byte string from python).
        Hide
        Patrick Hunt added a comment -

        Patch is applying cleanly now, however would you mind adding a test that verifies this case? That will ensure we don't regress in future versions. Thanks!

        Show
        Patrick Hunt added a comment - Patch is applying cleanly now, however would you mind adding a test that verifies this case? That will ensure we don't regress in future versions. Thanks!
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12516255/0001-make-sure-the-client-password-isn-t-corrupted.patch
        against trunk revision 1294000.

        +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 new tests are needed for this patch.
        Also please list what manual steps were performed to verify 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 (version 1.3.9) 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: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/966//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/966//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/966//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/12516255/0001-make-sure-the-client-password-isn-t-corrupted.patch against trunk revision 1294000. +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 new tests are needed for this patch. Also please list what manual steps were performed to verify 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 (version 1.3.9) 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: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/966//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/966//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/966//console This message is automatically generated.
        Hide
        Mike Lundy added a comment -

        Cut a -p0 patch instead of a -p1 patch (patch is the same for all three branches).

        Show
        Mike Lundy added a comment - Cut a -p0 patch instead of a -p1 patch (patch is the same for all three branches).
        Hide
        Patrick Hunt added a comment -

        Sorry for the trouble Mike. I believe the issue is that you are using git without the "--no-prefix" option. I typically do

        git diff --no-prefix HEAD^..HEAD

        from the same directly you're currently using (toplevel, ie the one containing "src", as in src/contrib/..."). Notice the original patch has a/src/con.. or b/src/con... which svn/patch don't like. Could you give that a try?

        Thanks!

        Show
        Patrick Hunt added a comment - Sorry for the trouble Mike. I believe the issue is that you are using git without the "--no-prefix" option. I typically do git diff --no-prefix HEAD^..HEAD from the same directly you're currently using (toplevel, ie the one containing "src", as in src/contrib/..."). Notice the original patch has a/src/con.. or b/src/con... which svn/patch don't like. Could you give that a try? Thanks!
        Hide
        Mike Lundy added a comment -

        shrug I can't reproduce the patch apply failure, I just cut those patches from the heads of the three branches and I can apply them without a hunk failure.

        Show
        Mike Lundy added a comment - shrug I can't reproduce the patch apply failure, I just cut those patches from the heads of the three branches and I can apply them without a hunk failure.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12516244/3.5-0001-make-sure-the-client-password-isn-t-corrupted.patch
        against trunk revision 1294000.

        +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 new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/964//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/12516244/3.5-0001-make-sure-the-client-password-isn-t-corrupted.patch against trunk revision 1294000. +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 new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/964//console This message is automatically generated.
        Hide
        Mike Lundy added a comment -

        Examining the code, I have no reason to believe the problem is fixed in 3.4 or trunk. I haven't tested there, however; we're seeing this in production with 3.3.

        Show
        Mike Lundy added a comment - Examining the code, I have no reason to believe the problem is fixed in 3.4 or trunk. I haven't tested there, however; we're seeing this in production with 3.3.
        Hide
        Mike Lundy added a comment -

        Patches cut against current branch-3.3, branch-3.4, and trunk

        Show
        Mike Lundy added a comment - Patches cut against current branch-3.3, branch-3.4, and trunk
        Hide
        Patrick Hunt added a comment -

        Hi Mike, is this issue in 3.3/3.4/3.5? Then typically we'd need patch(es) that apply to each cleanly. Would you mind creating such a patch? (3.4/3.5 may only require a single patch, not sure).

        Show
        Patrick Hunt added a comment - Hi Mike, is this issue in 3.3/3.4/3.5? Then typically we'd need patch(es) that apply to each cleanly. Would you mind creating such a patch? (3.4/3.5 may only require a single patch, not sure).
        Hide
        Mike Lundy added a comment -

        As before, this patch is cut against 3.3.4 and not 3.4.x.

        Show
        Mike Lundy added a comment - As before, this patch is cut against 3.3.4 and not 3.4.x.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12515633/0001-make-sure-the-client-password-isn-t-corrupted.patch
        against trunk revision 1244776.

        +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 new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/959//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/12515633/0001-make-sure-the-client-password-isn-t-corrupted.patch against trunk revision 1244776. +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 new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/959//console This message is automatically generated.

          People

          • Assignee:
            Mike Lundy
            Reporter:
            Mike Lundy
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:

              Development