diff --git a/zookeeper-client/zookeeper-client-c/src/zookeeper.c b/zookeeper-client/zookeeper-client-c/src/zookeeper.c index 2e08b3e0bfe..9c508db7513 100644 --- a/zookeeper-client/zookeeper-client-c/src/zookeeper.c +++ b/zookeeper-client/zookeeper-client-c/src/zookeeper.c @@ -1705,42 +1705,73 @@ void free_duplicate_path(const char *free_path, const char* path) { } /** - prepend the chroot path if available else return the path -*/ -static char* prepend_string(zhandle_t *zh, const char* client_path) { + * Map a client path to a server path under the chroot. + * + * A valid client path is absolute. The chroot prefix does not make an + * invalid path valid. A sequential create of the client root stays in + * the chroot namespace. A null result is an invalid path or a failed + * allocation. + */ +static char* prepend_chroot(zhandle_t *zh, const char* client_path, int mode) { + size_t chroot_len; + size_t path_len; char *ret_str; if (zh == NULL || zh->chroot == NULL) return (char *) client_path; - // handle the chroot itself, client_path = "/" - if (strlen(client_path) == 1) { + if (client_path == NULL || client_path[0] != '/') + return NULL; + /* + * The client root is the chroot node. A sequential create of the root + * keeps a slash at the end of the server path. The server adds the + * sequence number as a child of the chroot, not as a sibling. + */ + if (client_path[1] == '\0' && !ZOOKEEPER_IS_SEQUENCE(mode)) { return strdup(zh->chroot); } - ret_str = (char *) malloc(strlen(zh->chroot) + strlen(client_path) + 1); - strcpy(ret_str, zh->chroot); - return strcat(ret_str, client_path); + chroot_len = strlen(zh->chroot); + path_len = strlen(client_path); + ret_str = (char *) malloc(chroot_len + path_len + 1); + if (ret_str == NULL) + return NULL; + memcpy(ret_str, zh->chroot, chroot_len); + memcpy(ret_str + chroot_len, client_path, path_len + 1); + return ret_str; +} + +/** + * Map a client path to a server path when the request does not create a + * sequential node. + */ +static char* prepend_string(zhandle_t *zh, const char* client_path) { + return prepend_chroot(zh, client_path, 0); } /** - strip off the chroot string from the server path - if there is one else return the exact path + * Map a server path to a client path. + * + * The chroot prefix matches only at a path component boundary, f.e. a chroot + * of /app does not match the path /app2. Cluster configuration is not in + * the chroot namespace, so it is not affected by the chroot prefix. */ char* sub_string(zhandle_t *zh, const char* server_path) { - char *ret_str; - if (zh->chroot == NULL) + size_t chroot_len; + if (zh->chroot == NULL || server_path == NULL) return (char *) server_path; //ZOOKEEPER-1027 - if (strncmp(server_path, zh->chroot, strlen(zh->chroot)) != 0) { + chroot_len = strlen(zh->chroot); + if (strncmp(server_path, zh->chroot, chroot_len) == 0) { + if (server_path[chroot_len] == '\0') { + return strdup("/"); + } + if (server_path[chroot_len] == '/') { + return strdup(server_path + chroot_len); + } + } + if (strcmp(server_path, ZOO_CONFIG_NODE) != 0) { LOG_ERROR(LOGCALLBACK(zh), "server path %s does not include chroot path %s", server_path, zh->chroot); - return (char *) server_path; - } - if (strlen(server_path) == strlen(zh->chroot)) { - //return "/" - ret_str = strdup("/"); - return ret_str; } - ret_str = strdup(server_path + strlen(zh->chroot)); - return ret_str; + return (char *) server_path; } static buffer_list_t *allocate_buffer(char *buff, int len) @@ -4114,7 +4145,7 @@ static int Request_path_init(zhandle_t *zh, int mode, { assert(path_out); - *path_out = prepend_string(zh, path); + *path_out = prepend_chroot(zh, path, mode); /* * Restore *path_out to caller's original path pointer on validation * failure so caller-side free_duplicate_path() remains safe. @@ -5677,13 +5708,39 @@ int zoo_wget(zhandle_t *zh, const char *path, int zoo_getconfig(zhandle_t *zh, int watch, char *buffer, int* buffer_len, struct Stat *stat) { - return zoo_wget(zh,ZOO_CONFIG_NODE,watch?zh->watcher:0,zh->context, buffer,buffer_len,stat); + return zoo_wgetconfig(zh,watch?zh->watcher:0,zh->context, buffer,buffer_len,stat); } +/* + * Cluster configuration is not in the chroot namespace. This function + * uses the asynchronous configuration call so that the synchronous and + * asynchronous APIs will both read the same configuration node. + */ int zoo_wgetconfig(zhandle_t *zh, watcher_fn watcher, void* watcherCtx, char *buffer, int* buffer_len, struct Stat *stat) { - return zoo_wget(zh, ZOO_CONFIG_NODE, watcher, watcherCtx, buffer, buffer_len, stat); + struct sync_completion *sc; + int rc=0; + + if(buffer_len==NULL) + return ZBADARGUMENTS; + if((sc=alloc_sync_completion())==NULL) + return ZSYSTEMERROR; + + sc->u.data.buffer = buffer; + sc->u.data.buff_len = *buffer_len; + rc=zoo_awgetconfig(zh, watcher, watcherCtx, SYNCHRONOUS_MARKER, sc); + if(rc==ZOK){ + wait_sync_completion(sc); + rc = sc->rc; + if (rc == 0) { + if(stat) + *stat = sc->u.data.stat; + *buffer_len = sc->u.data.buff_len; + } + } + free_sync_completion(sc); + return rc; } diff --git a/zookeeper-client/zookeeper-client-c/tests/TestClient.cc b/zookeeper-client/zookeeper-client-c/tests/TestClient.cc index 36739e11f04..5e6c9f70280 100644 --- a/zookeeper-client/zookeeper-client-c/tests/TestClient.cc +++ b/zookeeper-client/zookeeper-client-c/tests/TestClient.cc @@ -51,6 +51,9 @@ struct buff_struct_2 { // it needs a overhaul to work properly with both threaded and single // threaded (ZOOKEEPER-2640) #ifdef THREADED +// The client uses this internal function to map a server path to a client path. +extern "C" char* sub_string(zhandle_t *zh, const char* server_path); + // For testing LogMessage Callback functionality list logMessages; void logMessageHandler(const char* message) { @@ -225,6 +228,9 @@ class Zookeeper_simpleSystem : public CPPUNIT_NS::TestFixture CPPUNIT_TEST(testPing); CPPUNIT_TEST(testAcl); CPPUNIT_TEST(testChroot); + CPPUNIT_TEST(testChrootPathValidation); + CPPUNIT_TEST(testChrootGetConfig); + CPPUNIT_TEST(testChrootServerPathMapping); CPPUNIT_TEST(testAuth); CPPUNIT_TEST(testCloseInAuthCompletion); CPPUNIT_TEST(testHangingClient); @@ -1205,6 +1211,166 @@ class Zookeeper_simpleSystem : public CPPUNIT_NS::TestFixture CPPUNIT_ASSERT(count == 0); } + // A handle with a chroot and a handle with no chroot reject the same + // invalid client paths. Invalid paths do not reach nodes outside the + // chroot. + void testChrootPathValidation() { + watchctx_t ctx, ctx_ch; + struct Stat stat; + struct String_vector children; + char buf[64]; + int len; + zhandle_t *zk = createClient(&ctx); + CPPUNIT_ASSERT(zk); + CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_create(zk, "/tenant", "", 0, + &ZOO_OPEN_ACL_UNSAFE, 0, 0, 0)); + CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_create(zk, "/tenant2", "", 0, + &ZOO_OPEN_ACL_UNSAFE, 0, 0, 0)); + CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_create(zk, "/tenantx", "", 0, + &ZOO_OPEN_ACL_UNSAFE, 0, 0, 0)); + zhandle_t *zk_ch = createchClient(&ctx_ch, "127.0.0.1:22181/tenant"); + CPPUNIT_ASSERT(zk_ch); + + const char *bad[] = { NULL, "", "x", "2", "2/x", "x/y" }; + for (size_t i = 0; i < sizeof(bad) / sizeof(bad[0]); i++) { + const char *path = bad[i]; + zhandle_t *handles[] = { zk, zk_ch }; + int multi_rc[2]; + for (size_t j = 0; j < 2; j++) { + zhandle_t *zh = handles[j]; + verifyCreateFails(path, zh); + verifyCreateFailsSeq(path, zh); + CPPUNIT_ASSERT_EQUAL((int)ZBADARGUMENTS, + zoo_exists(zh, path, 0, &stat)); + len = sizeof(buf); + CPPUNIT_ASSERT_EQUAL((int)ZBADARGUMENTS, + zoo_get(zh, path, 0, buf, &len, &stat)); + CPPUNIT_ASSERT_EQUAL((int)ZBADARGUMENTS, + zoo_set(zh, path, "x", 1, -1)); + CPPUNIT_ASSERT_EQUAL((int)ZBADARGUMENTS, + zoo_delete(zh, path, -1)); + CPPUNIT_ASSERT_EQUAL((int)ZBADARGUMENTS, + zoo_get_children(zh, path, 0, &children)); + CPPUNIT_ASSERT_EQUAL((int)ZBADARGUMENTS, + zoo_remove_all_watches(zh, path, ZWATCHTYPE_ANY, 1)); + zoo_op_t op; + zoo_op_result_t result; + zoo_delete_op_init(&op, path, -1); + multi_rc[j] = zoo_multi(zh, 1, &op, &result); + } + // zoo_multi reports a sub-request error as ZMARSHALLINGERROR. + // The two handles give the same error. + CPPUNIT_ASSERT(multi_rc[0] != ZOK); + CPPUNIT_ASSERT_EQUAL(multi_rc[0], multi_rc[1]); + } + + // Invalid paths did not change the chroot node or its sibling nodes. + CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_exists(zk, "/tenant", 0, &stat)); + CPPUNIT_ASSERT_EQUAL(0, stat.version); + CPPUNIT_ASSERT_EQUAL(0, stat.numChildren); + CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_exists(zk, "/tenant2", 0, &stat)); + CPPUNIT_ASSERT_EQUAL(0, stat.numChildren); + CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_exists(zk, "/tenantx", 0, &stat)); + CPPUNIT_ASSERT_EQUAL(0, stat.version); + + // A sequential create of the client root makes a child of the chroot + // node. A sequential create of "/" on a handle with no chroot has + // the same result at the cluster root. + char path_buffer[64]; + CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_create(zk_ch, "/", "", 0, + &ZOO_OPEN_ACL_UNSAFE, ZOO_SEQUENCE, path_buffer, + sizeof(path_buffer))); + CPPUNIT_ASSERT_EQUAL((size_t)11, strlen(path_buffer)); + CPPUNIT_ASSERT_EQUAL('/', path_buffer[0]); + string server_path = string("/tenant") + path_buffer; + CPPUNIT_ASSERT_EQUAL((int)ZOK, + zoo_exists(zk, server_path.c_str(), 0, &stat)); + CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_get_children(zk, "/", 0, &children)); + for (int i = 0; i < children.count; i++) { + CPPUNIT_ASSERT(strncmp(children.data[i], "tenant0", 7) != 0); + } + deallocate_String_vector(&children); + + // Other operations on the client root use the chroot node. + CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_exists(zk_ch, "/", 0, &stat)); + CPPUNIT_ASSERT_EQUAL(1, stat.numChildren); + CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_get_children(zk_ch, "/", 0, &children)); + CPPUNIT_ASSERT_EQUAL(1, (int)children.count); + CPPUNIT_ASSERT_EQUAL(string(path_buffer + 1), string(children.data[0])); + deallocate_String_vector(&children); + } + + // Cluster configuration is not in the chroot namespace. Synchronous and + // asynchronous reads get the same node as a handle with no chroot. + void testChrootGetConfig() { + watchctx_t ctx, ctx_ch; + char buf[1024], buf_ch[1024]; + int len = sizeof(buf); + int len_ch = sizeof(buf_ch); + struct Stat stat, stat_ch; + zhandle_t *zk = createClient(&ctx); + CPPUNIT_ASSERT(zk); + CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_create(zk, "/cfgchroot", "", 0, + &ZOO_OPEN_ACL_UNSAFE, 0, 0, 0)); + zhandle_t *zk_ch = createchClient(&ctx_ch, "127.0.0.1:22181/cfgchroot"); + CPPUNIT_ASSERT(zk_ch); + + CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_getconfig(zk, 0, buf, &len, &stat)); + CPPUNIT_ASSERT_EQUAL((int)ZOK, + zoo_getconfig(zk_ch, 0, buf_ch, &len_ch, &stat_ch)); + CPPUNIT_ASSERT_EQUAL(len, len_ch); + CPPUNIT_ASSERT(len <= 0 || memcmp(buf, buf_ch, len) == 0); + CPPUNIT_ASSERT_EQUAL(stat.czxid, stat_ch.czxid); + CPPUNIT_ASSERT_EQUAL(stat.mzxid, stat_ch.mzxid); + + len_ch = sizeof(buf_ch); + memset(&stat_ch, 0, sizeof(stat_ch)); + CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_wgetconfig(zk_ch, watcher, &ctx_ch, + buf_ch, &len_ch, &stat_ch)); + CPPUNIT_ASSERT_EQUAL(len, len_ch); + CPPUNIT_ASSERT_EQUAL(stat.mzxid, stat_ch.mzxid); + } + + static bool hasChrootPathError() { + for (list::iterator it = logMessages.begin(); + it != logMessages.end(); ++it) { + if (it->find("does not include chroot path") != string::npos) { + return true; + } + } + return false; + } + + // The chroot prefix of a server path ends at a path-component boundary. + // Cluster configuration that is not in the chroot does not cause an + // error. + void testChrootServerPathMapping() { + watchctx_t ctx; + logMessages.clear(); + zhandle_t *zh = zookeeper_init2("127.0.0.1:22181/zoo", watcher, 10000, + 0, &ctx, 0, logMessageHandler); + CPPUNIT_ASSERT(zh); + ctx.zh = zh; + + char *client_path = sub_string(zh, "/zoo"); + CPPUNIT_ASSERT_EQUAL(string("/"), string(client_path)); + free(client_path); + client_path = sub_string(zh, "/zoo/a/b"); + CPPUNIT_ASSERT_EQUAL(string("/a/b"), string(client_path)); + free(client_path); + CPPUNIT_ASSERT(!hasChrootPathError()); + + const char *config_path = ZOO_CONFIG_NODE; + CPPUNIT_ASSERT(sub_string(zh, config_path) == config_path); + CPPUNIT_ASSERT(!hasChrootPathError()); + + const char *sibling_path = "/zoo2/a"; + CPPUNIT_ASSERT(sub_string(zh, sibling_path) == sibling_path); + CPPUNIT_ASSERT(hasChrootPathError()); + + CPPUNIT_ASSERT(sub_string(zh, NULL) == NULL); + } + // Test creating normal handle via zookeeper_init then explicitly setting callback void testLogCallbackSet() {