You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by "empiredan (via GitHub)" <gi...@apache.org> on 2025/09/25 06:46:56 UTC
[PR] fix(zookeeper): fix potential dangling path pointer issue when calling ZooKeeper APIs [incubator-pegasus]
empiredan opened a new pull request, #2302:
URL: https://github.com/apache/incubator-pegasus/pull/2302
(no comment)
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] fix(zookeeper): fix potential dangling path pointer issue when calling ZooKeeper APIs [incubator-pegasus]
Posted by "empiredan (via GitHub)" <gi...@apache.org>.
empiredan commented on code in PR #2302:
URL: https://github.com/apache/incubator-pegasus/pull/2302#discussion_r2464313374
##########
src/zookeeper/zookeeper_session.cpp:
##########
@@ -398,58 +399,64 @@ void zookeeper_session::visit(zoo_opcontext *ctx)
_watchers.back().watcher_callback = std::move(ctx->_input._watcher_callback);
};
- // TODO: the read ops from zookeeper might get the staled data, need to fix
+ // TODO(clang-tidy): the read ops from zookeeper might get the staled data, need to fix
int ec = ZOK;
zoo_input &input = ctx->_input;
- const char *path = input._path.c_str();
+
+ // A local variable is needed here to hold a reference to `_path` to prevent it from
+ // being freed in the thread executing the completion callback, which could otherwise
+ // lead to a dangling pointer issue.
+ const std::shared_ptr<std::string> path(input._path);
Review Comment:
Yes !
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] fix(zookeeper): fix potential dangling path pointer issue when calling ZooKeeper APIs [incubator-pegasus]
Posted by "empiredan (via GitHub)" <gi...@apache.org>.
empiredan commented on code in PR #2302:
URL: https://github.com/apache/incubator-pegasus/pull/2302#discussion_r2446656723
##########
src/meta/test/meta_state/meta_state_service.cpp:
##########
@@ -61,87 +62,112 @@ void provider_basic_test(const service_creator_func &service_creator,
const service_deleter_func &service_deleter)
{
// environment
- auto service = service_creator();
+ auto *service = service_creator();
// bondary check
- service->node_exist("/", META_STATE_SERVICE_SIMPLE_TEST_CALLBACK, expect_ok)->wait();
Review Comment:
`node_exist` here is a **virtual function** that implements a pure virtual function declared in `meta_state_service`.
However, having **default arguments** in a virtual function violates the linter rule — `clang-tidy` check would fail due to the following error: `warning: default arguments on virtual or override methods are prohibited [google-default-arguments]`.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] fix(zookeeper): fix potential dangling path pointer issue when calling ZooKeeper APIs [incubator-pegasus]
Posted by "empiredan (via GitHub)" <gi...@apache.org>.
empiredan commented on code in PR #2302:
URL: https://github.com/apache/incubator-pegasus/pull/2302#discussion_r2446640977
##########
src/zookeeper/lock_struct.cpp:
##########
@@ -637,20 +644,23 @@ void lock_struct::create_locknode()
}
if (op->_output.error != ZOK) {
- EXECUTE(
- std::bind(&lock_struct::after_create_locknode, _this, op->_output.error, nullptr),
+ enqueue(
+ [_this, err = op->_output.error]() {
+ lock_struct::after_create_locknode(_this, err, nullptr);
+ },
_this);
return;
}
std::shared_ptr<std::string> path(new std::string(op->_output.create_op._created_path));
- EXECUTE(std::bind(&lock_struct::after_create_locknode, _this, ZOK, path), _this);
+ enqueue([_this, path]() { lock_struct::after_create_locknode(_this, ZOK, path); }, _this);
};
zookeeper_session::zoo_input &input = op->_input;
- input._path = _lock_dir + "/" + distributed_lock_service_zookeeper::LOCK_NODE_PREFIX;
+ input._path = std::make_shared<std::string>(
+ fmt::format("{}/{}", _lock_dir, distributed_lock_service_zookeeper::LOCK_NODE_PREFIX));
input._value.assign(_myself._node_value.c_str(), 0, _myself._node_value.length());
- input._flags = ZOO_EPHEMERAL | ZOO_SEQUENCE;
+ input._flags = ZOO_EPHEMERAL_SEQUENTIAL;
Review Comment:
For the following reasons, we must use `ZOO_EPHEMERAL_SEQUENTIAL` instead of `ZOO_EPHEMERAL | ZOO_SEQUENCE`:
* We rely on ZooKeeper C client **3.9.4**, which defines `ZOO_EPHEMERAL_SEQUENTIAL` as a replacement for `ZOO_EPHEMERAL | ZOO_SEQUENCE`.
* `ZOO_SEQUENCE` has already been **deprecated** — if it were to be used, it should be replaced by `ZOO_PERSISTENT_SEQUENTIAL`.
* `ZOO_EPHEMERAL | ZOO_SEQUENCE` cannot pass the linter check; **clang-tidy** reports the following warning:
`warning: use of a signed integer operand with a binary bitwise operator [hicpp-signed-bitwise] input._flags = ZOO_EPHEMERAL | ZOO_SEQUENCE;`
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] fix(zookeeper): fix potential dangling path pointer issue when calling ZooKeeper APIs [incubator-pegasus]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2302:
URL: https://github.com/apache/incubator-pegasus/pull/2302#discussion_r2445410302
##########
src/meta/meta_state_service_zookeeper.cpp:
##########
@@ -199,7 +200,7 @@ meta_state_service_zookeeper::new_transaction_entries(unsigned int capacity)
tsk, \
std::placeholders::_1); \
op->_optype = op_type; \
- input->_path = node;
+ input->_path = std::make_shared<std::string>(node)
Review Comment:
Missing semicolon at the end of the statement. This will cause a compilation error.
```suggestion
input->_path = std::make_shared<std::string>(node);
```
##########
src/zookeeper/lock_struct.cpp:
##########
@@ -637,20 +644,23 @@ void lock_struct::create_locknode()
}
if (op->_output.error != ZOK) {
- EXECUTE(
- std::bind(&lock_struct::after_create_locknode, _this, op->_output.error, nullptr),
+ enqueue(
+ [_this, err = op->_output.error]() {
+ lock_struct::after_create_locknode(_this, err, nullptr);
+ },
_this);
return;
}
std::shared_ptr<std::string> path(new std::string(op->_output.create_op._created_path));
- EXECUTE(std::bind(&lock_struct::after_create_locknode, _this, ZOK, path), _this);
+ enqueue([_this, path]() { lock_struct::after_create_locknode(_this, ZOK, path); }, _this);
};
zookeeper_session::zoo_input &input = op->_input;
- input._path = _lock_dir + "/" + distributed_lock_service_zookeeper::LOCK_NODE_PREFIX;
+ input._path = std::make_shared<std::string>(
+ fmt::format("{}/{}", _lock_dir, distributed_lock_service_zookeeper::LOCK_NODE_PREFIX));
input._value.assign(_myself._node_value.c_str(), 0, _myself._node_value.length());
- input._flags = ZOO_EPHEMERAL | ZOO_SEQUENCE;
+ input._flags = ZOO_EPHEMERAL_SEQUENTIAL;
Review Comment:
The flag value `ZOO_EPHEMERAL_SEQUENTIAL` does not exist in the ZooKeeper C API. The correct combination is `ZOO_EPHEMERAL | ZOO_SEQUENCE`. This change will cause a compilation error or incorrect behavior.
```suggestion
input._flags = ZOO_EPHEMERAL | ZOO_SEQUENCE;
```
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] fix(zookeeper): fix potential dangling path pointer issue when calling ZooKeeper APIs [incubator-pegasus]
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on code in PR #2302:
URL: https://github.com/apache/incubator-pegasus/pull/2302#discussion_r2463333516
##########
src/meta/test/meta_state/meta_state_service.cpp:
##########
@@ -61,87 +62,112 @@ void provider_basic_test(const service_creator_func &service_creator,
const service_deleter_func &service_deleter)
{
// environment
- auto service = service_creator();
+ auto *service = service_creator();
// bondary check
- service->node_exist("/", META_STATE_SERVICE_SIMPLE_TEST_CALLBACK, expect_ok)->wait();
Review Comment:
Yes, the google-default-arguments rule is aim to prevent different default values on base class and derived class, but if we ensure they are the same deterministically, maybe we can use this.
##########
src/zookeeper/zookeeper_session.cpp:
##########
@@ -398,58 +399,64 @@ void zookeeper_session::visit(zoo_opcontext *ctx)
_watchers.back().watcher_callback = std::move(ctx->_input._watcher_callback);
};
- // TODO: the read ops from zookeeper might get the staled data, need to fix
+ // TODO(clang-tidy): the read ops from zookeeper might get the staled data, need to fix
int ec = ZOK;
zoo_input &input = ctx->_input;
- const char *path = input._path.c_str();
+
+ // A local variable is needed here to hold a reference to `_path` to prevent it from
+ // being freed in the thread executing the completion callback, which could otherwise
+ // lead to a dangling pointer issue.
+ const std::shared_ptr<std::string> path(input._path);
Review Comment:
Is this the key changes of this PR?
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] fix(zookeeper): fix potential dangling path pointer issue when calling ZooKeeper APIs [incubator-pegasus]
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on code in PR #2302:
URL: https://github.com/apache/incubator-pegasus/pull/2302#discussion_r2445422337
##########
src/meta/test/meta_state/meta_state_service.cpp:
##########
@@ -61,87 +62,112 @@ void provider_basic_test(const service_creator_func &service_creator,
const service_deleter_func &service_deleter)
{
// environment
- auto service = service_creator();
+ auto *service = service_creator();
// bondary check
- service->node_exist("/", META_STATE_SERVICE_SIMPLE_TEST_CALLBACK, expect_ok)->wait();
Review Comment:
Is it possible to keep use a default parameter?
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] fix(zookeeper): fix potential dangling path pointer issue when calling ZooKeeper APIs [incubator-pegasus]
Posted by "empiredan (via GitHub)" <gi...@apache.org>.
empiredan commented on code in PR #2302:
URL: https://github.com/apache/incubator-pegasus/pull/2302#discussion_r2464320586
##########
src/meta/test/meta_state/meta_state_service.cpp:
##########
@@ -61,87 +62,112 @@ void provider_basic_test(const service_creator_func &service_creator,
const service_deleter_func &service_deleter)
{
// environment
- auto service = service_creator();
+ auto *service = service_creator();
// bondary check
- service->node_exist("/", META_STATE_SERVICE_SIMPLE_TEST_CALLBACK, expect_ok)->wait();
Review Comment:
Indeed. At first, I didn’t plan to modify it — I thought it would be fine to keep the default argument.
However, since the virtual function in the `meta_state_service` class isn’t called in many places and can be easily updated, I decided to follow `clang-tidy`’s suggestion and make the change.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] fix(zookeeper): fix potential dangling path pointer issue when calling ZooKeeper APIs [incubator-pegasus]
Posted by "empiredan (via GitHub)" <gi...@apache.org>.
empiredan merged PR #2302:
URL: https://github.com/apache/incubator-pegasus/pull/2302
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] fix(zookeeper): fix potential dangling path pointer issue when calling ZooKeeper APIs [incubator-pegasus]
Posted by "empiredan (via GitHub)" <gi...@apache.org>.
empiredan commented on code in PR #2302:
URL: https://github.com/apache/incubator-pegasus/pull/2302#discussion_r2446649612
##########
src/meta/meta_state_service_zookeeper.cpp:
##########
@@ -199,7 +200,7 @@ meta_state_service_zookeeper::new_transaction_entries(unsigned int capacity)
tsk, \
std::placeholders::_1); \
op->_optype = op_type; \
- input->_path = node;
+ input->_path = std::make_shared<std::string>(node)
Review Comment:
`input->_path = std::make_shared<std::string>(node)` is the **last statement** in the function-like macro **`VISIT_INIT(tsk, op_type, node)`**. This macro is always used in the form `VISIT_INIT(...);`, i.e., followed by a semicolon. Therefore, the final statement inside the macro **should not** end with a semicolon.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org