ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-311

handle small path lengths in zoo_create()

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 3.0.0, 3.0.1, 3.1.0, 3.1.1, 3.2.0
    • Fix Version/s: 3.2.1, 3.3.0
    • Component/s: c client
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change
    • Release Note:
      Hide
      The zoo_create() function is revised to take path_buffer and path_buffer_len arguments. The path_buffer will be filled with as much of the actual path of the new node fits into the buffer, including a null terminator. Unlike previous behavior, if path_buffer_len (formerly max_realpath_len) is 1, the buffer will be filled with a single null terminator.
      Show
      The zoo_create() function is revised to take path_buffer and path_buffer_len arguments. The path_buffer will be filled with as much of the actual path of the new node fits into the buffer, including a null terminator. Unlike previous behavior, if path_buffer_len (formerly max_realpath_len) is 1, the buffer will be filled with a single null terminator.

      Description

      The synchronous completion for zoo_create() contains the following code:

      if (sc->u.str.str_len > strlen(res.path)) {
          len = strlen(res.path);
      } else {
          len = sc->u.str.str_len-1;
      }
      if (len > 0) {
          memcpy(sc->u.str.str, res.path, len);
          sc->u.str.str[len] = '\0';
      }
      

      In the case where the max_realpath_len argument to zoo_create() is 0, none of this code executes, which is OK. In the case where max_realpath_len is 1, a user might expect their buffer to be filled with a null terminator, but again, nothing will happen (even if strlen(res.path) is 0, which is unlikely since new node's will have paths longer than "/").

      The name of the argument to zoo_create() is also a little misleading, as is its description ("the maximum length of real path you would want") in zookeeper.h, and the example usage in the Programmer's Guide:

      int rc = zoo_create(zh,"/xyz","value", 5, &CREATE_ONLY, ZOO_EPHEMERAL, buffer, sizeof(buffer)-1);
      

      In fact this value should be the actual length of the buffer, including space for the null terminator. If the user supplies a max_realpath_len of 10 and a buffer of 11 bytes, and strlen(res.path) is 10, the code will truncate the returned value to 9 bytes and put the null terminator in the second-last byte, leaving the final byte of the buffer unused.

      It would be better, I think, to rename the realpath and max_realpath_len arguments to something like path_buffer and path_buffer_len, akin to zoo_set(). The path_buffer_len would be treated as the full length of the buffer (as the code does now, in fact, but the docs suggest otherwise).

      The code in the synchronous completion could then be changed as per the attached patch.

      Since this would change, slightly, the behaviour or "contract" of the API, I would be inclined to suggest waiting until 4.0.0 to implement this change.

      1. ZOOKEEPER-311.patch
        6 kB
        Chris Darroch
      2. ZOOKEEPER-311.patch
        3 kB
        Chris Darroch

        Activity

        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #407 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/407/)
        . handle small path lengths in zoo_create()

        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #407 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/407/ ) . handle small path lengths in zoo_create()
        Hide
        Benjamin Reed added a comment -

        commit to 3.2 branch: Committed revision 801756.
        commit to trunk: Committed revision 801747.

        Show
        Benjamin Reed added a comment - commit to 3.2 branch: Committed revision 801756. commit to trunk: Committed revision 801747.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12413380/ZOOKEEPER-311.patch
        against trunk revision 798038.

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

        +1 tests included. The patch appears to include 3 new or modified tests.

        +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-vesta.apache.org/160/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/160/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/160/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/12413380/ZOOKEEPER-311.patch against trunk revision 798038. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +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-vesta.apache.org/160/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/160/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/160/console This message is automatically generated.
        Hide
        Mahadev konar added a comment -

        the qa isnt running because the nightly builds are failing. This should be fixed soon and we can get the patch process back on track.

        Show
        Mahadev konar added a comment - the qa isnt running because the nightly builds are failing. This should be fixed soon and we can get the patch process back on track.
        Hide
        Benjamin Reed added a comment -

        +1 great job chris! i likke your test cases. thanx. now let me see if i can find out why qa isn't running...

        Show
        Benjamin Reed added a comment - +1 great job chris! i likke your test cases. thanx. now let me see if i can find out why qa isn't running...
        Hide
        Chris Darroch added a comment -

        Updated to current trunk, with some tests added for extra crunchy goodness.

        Show
        Chris Darroch added a comment - Updated to current trunk, with some tests added for extra crunchy goodness.
        Hide
        Chris Darroch added a comment -

        This revision is a good demonstration of why one should never trust a programmer, especially if there's a degree of hubris involved ("It's just a small patch, what could go wrong?") In testing, I found I had an off-by-one error when calculating where to put the trailing null. Sigh.

        I agree this shouldn't be committed without some tests. I'm at virtually 100% capacity at the moment with just my paid work so it may take me a while to do this; it is my next to-do, though. If someone else wants to write a few tests in the meantime, that would also be great.

        Show
        Chris Darroch added a comment - This revision is a good demonstration of why one should never trust a programmer, especially if there's a degree of hubris involved ("It's just a small patch, what could go wrong?") In testing, I found I had an off-by-one error when calculating where to put the trailing null. Sigh. I agree this shouldn't be committed without some tests. I'm at virtually 100% capacity at the moment with just my paid work so it may take me a while to do this; it is my next to-do, though. If someone else wants to write a few tests in the meantime, that would also be great.
        Hide
        Benjamin Reed added a comment -

        i'm ready to commit this, but we need a test case. it would be nice if it could test name buffers of size 0 to 3. can you do this chris, or do you need me to write it?

        Show
        Benjamin Reed added a comment - i'm ready to commit this, but we need a test case. it would be nice if it could test name buffers of size 0 to 3. can you do this chris, or do you need me to write it?
        Hide
        Benjamin Reed added a comment -

        i think we should commit this patch now. the only change in behavior is when the length of the buffer to receive the name is of length 1. it is a silly corner case, but really the fact that we don't put a null there should be considered a bug.

        Show
        Benjamin Reed added a comment - i think we should commit this patch now. the only change in behavior is when the length of the buffer to receive the name is of length 1. it is a silly corner case, but really the fact that we don't put a null there should be considered a bug.
        Hide
        Chris Darroch added a comment -

        Mostly because the existing logic is a little counter-intuitive. I spent quite a bit of time looking at it while trying to determine it's behaviour in various cases — e.g., what happens if my buffer is 1 byte long? Because I was writing a wrapper library (Net::ZooKeeper) and not a client application, I didn't know what "max path len" the user might supply and I wanted to ensure consistent behaviour.

        Typical C functions that take a buffer and fill it (e.g., snprintf()) rarely do nothing if the buffer is 1 byte long; instead, they fill it with a null terminator. So that's surprising and counterintuitive.

        What I ended up doing in Net::ZooKeeper was preventing users from supplying "max path len" values less than 2.

        My other concern is really just about future-proofing. Suppose in a future release ZooKeeper returns null paths from create() calls for some reason — because of some new feature ("virtual nodes"?) or whatever. Anyway, in this case, the user might reasonably expect a 1-byte buffer to be (marginally) useful. Or perhaps the code gets copied and re-used in another context where 1-byte buffers make more sense.

        The code rewrite, although simple, I think clarifies what's going on in a way that will help prevent buffer problems in the future — always a sore point when coding in C. First we calculate the len of the buffer that would be required for the full path, including space for the null terminator. If that's longer than what we've been given, we truncate to what we have. If there's anything to do at this point (i.e., the supplied buffer had at least one byte), then we do it.

        Show
        Chris Darroch added a comment - Mostly because the existing logic is a little counter-intuitive. I spent quite a bit of time looking at it while trying to determine it's behaviour in various cases — e.g., what happens if my buffer is 1 byte long? Because I was writing a wrapper library (Net::ZooKeeper) and not a client application, I didn't know what "max path len" the user might supply and I wanted to ensure consistent behaviour. Typical C functions that take a buffer and fill it (e.g., snprintf()) rarely do nothing if the buffer is 1 byte long; instead, they fill it with a null terminator. So that's surprising and counterintuitive. What I ended up doing in Net::ZooKeeper was preventing users from supplying "max path len" values less than 2. My other concern is really just about future-proofing. Suppose in a future release ZooKeeper returns null paths from create() calls for some reason — because of some new feature ("virtual nodes"?) or whatever. Anyway, in this case, the user might reasonably expect a 1-byte buffer to be (marginally) useful. Or perhaps the code gets copied and re-used in another context where 1-byte buffers make more sense. The code rewrite, although simple, I think clarifies what's going on in a way that will help prevent buffer problems in the future — always a sore point when coding in C. First we calculate the len of the buffer that would be required for the full path, including space for the null terminator. If that's longer than what we've been given, we truncate to what we have. If there's anything to do at this point (i.e., the supplied buffer had at least one byte), then we do it.
        Hide
        Benjamin Reed added a comment -

        this is really just a documentation bug. is there a reason you are changing logic?

        Show
        Benjamin Reed added a comment - this is really just a documentation bug. is there a reason you are changing logic?
        Hide
        Chris Darroch added a comment -

        Changes parameter names as suggested and revises C API documentation.

        Show
        Chris Darroch added a comment - Changes parameter names as suggested and revises C API documentation.
        Hide
        Chris Darroch added a comment -

        Renamed patch file with issue key.

        Show
        Chris Darroch added a comment - Renamed patch file with issue key.

          People

          • Assignee:
            Chris Darroch
            Reporter:
            Chris Darroch
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development