Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix a use-after-move in partition_balancer #7406

Merged
merged 1 commit into from
Nov 22, 2022
Merged

Fix a use-after-move in partition_balancer #7406

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 25 additions & 24 deletions src/v/cluster/topic_updates_dispatcher.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,32 +53,33 @@ topic_updates_dispatcher::apply_update(model::record_batch b) {
del_cmd.key, *topic_assignments);
}
return dispatch_updates_to_cores(del_cmd, base_offset)
.then([this,
tp_ns = std::move(del_cmd.key),
topic_assignments = std::move(topic_assignments),
in_progress = std::move(in_progress),
allocation_domain = get_allocation_domain(
del_cmd.key)](std::error_code ec) {
if (ec == errc::success) {
vassert(
topic_assignments.has_value(),
"Topic had to exist before successful delete");
deallocate_topic(
*topic_assignments, in_progress, allocation_domain);
.then(
[this,
tp_ns = std::move(del_cmd.key),
topic_assignments = std::move(topic_assignments),
in_progress = std::move(in_progress)](std::error_code ec) {
if (ec == errc::success) {
Comment on lines -60 to +61
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch.

prior to this change get_allocation_domain ran before the continuation (when the capture list was created). but now it runs when the continuation is scheduled. is that something to be concerned with? in this case it might make sense to remove std::move from tp_ns in the capture list and pay a small string copy cost to keep things simple and preserve the semantics.

Copy link
Contributor Author

@dlex dlex Nov 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No concern here. tp_ns is not changed (the capture is not mutable), the lambda is not a coro, and get_allocation_domain() is a pure function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No concern here. tp_ns is not changed (the capture is not mutable), the lambda is not a coro, and get_allocation_domain() is a pure function.

awesome. thanks for the explanation

vassert(
topic_assignments.has_value(),
"Topic had to exist before successful delete");
deallocate_topic(
*topic_assignments,
in_progress,
get_allocation_domain(tp_ns));

for (const auto& p_as : *topic_assignments) {
_partition_balancer_state.local()
.handle_ntp_update(
tp_ns.ns,
tp_ns.tp,
p_as.id,
p_as.replicas,
{});
}
}
for (const auto& p_as : *topic_assignments) {
_partition_balancer_state.local()
.handle_ntp_update(
tp_ns.ns,
tp_ns.tp,
p_as.id,
p_as.replicas,
{});
}
}

return ec;
});
return ec;
});
},
[this, base_offset](create_topic_cmd create_cmd) {
return dispatch_updates_to_cores(create_cmd, base_offset)
Expand Down