This is an automated email from the ASF dual-hosted git repository.
prozsa pushed a commit to branch branch-4.5.0
in repository https://gitbox.apache.org/repos/asf/impala.git
commit fa07a4bd7c8e679a7458297d058419e4be9c25e2
Author: jasonmfehr <jf...@cloudera.com>
AuthorDate: Wed Feb 5 09:06:19 2025 -0800
IMPALA-13736: Fix Use-After-Free in ExecutorGroup.RemoveExecutor
The RemoveExecutor() function within the ExecutorGroup class has a
potential use-after-free bug. Since the function takes an object
reference as input, the iterator that erases the backend could erase
the object references by the function input.
This change fixes the issue by storing the necessary data from the
provided input object and then referencing that stored data after the
erase has occurred.
Change-Id: If14a3c89ee631ebb05efc9a47745f7e63ab98690
Reviewed-on: http://gerrit.cloudera.org:8080/22453
Reviewed-by: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
Tested-by: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
---
be/src/scheduling/executor-group-test.cc | 8 ++++++++
be/src/scheduling/executor-group.cc | 14 +++++++++++---
be/src/scheduling/executor-group.h | 2 +-
3 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/be/src/scheduling/executor-group-test.cc
b/be/src/scheduling/executor-group-test.cc
index 96a744136..e2f926c48 100644
--- a/be/src/scheduling/executor-group-test.cc
+++ b/be/src/scheduling/executor-group-test.cc
@@ -81,6 +81,14 @@ TEST(ExecutorGroupTest, RemoveExecutor) {
ASSERT_TRUE(group1.LookUpExecutorIp("host_1", &backend_ip));
EXPECT_EQ("10.0.0.1", backend_ip);
ASSERT_FALSE(group1.LookUpExecutorIp("host_2", &backend_ip));
+
+ // When the last executor for a host is be removed from the group, that host
is also
+ // removed from the group. Asserts that process completes successfully even
when the
+ // RemoveExecutor function is provided the same object that will be erased.
+ const BackendDescriptorPB* actual_exec1 =
group1.LookUpBackendDesc(executor1.address());
+ ASSERT_NE(nullptr, actual_exec1);
+ group1.RemoveExecutor(*actual_exec1);
+ ASSERT_EQ(0, group1.NumExecutors());
}
/// Test removing one of multiple backends on the same host (IMPALA-3944).
diff --git a/be/src/scheduling/executor-group.cc
b/be/src/scheduling/executor-group.cc
index ba2b597ee..b99eeed86 100644
--- a/be/src/scheduling/executor-group.cc
+++ b/be/src/scheduling/executor-group.cc
@@ -151,14 +151,22 @@ void ExecutorGroup::RemoveExecutor(const
BackendDescriptorPB& be_desc) {
<< be_desc.krpc_address();
return;
}
+
+ // Copy the data necessary to update the internal state variables since the
erase call
+ // has the potential to destroy the object referenced by be_desc.
+ const std::string be_hostname = be_desc.address().hostname();
+ const std::string be_ip_address = be_desc.ip_address();
+ const int64_t be_admin_mem_limit = be_desc.admit_mem_limit();
+
be_descs.erase(remove_it);
- if (per_executor_admit_mem_limit_ == be_desc.admit_mem_limit()) {
+
+ if (per_executor_admit_mem_limit_ == be_admin_mem_limit) {
CalculatePerExecutorMemLimitForAdmission();
}
if (be_descs.empty()) {
executor_map_.erase(be_descs_it);
- executor_ip_map_.erase(be_desc.address().hostname());
- executor_ip_hash_ring_.RemoveNode(be_desc.ip_address());
+ executor_ip_map_.erase(be_hostname);
+ executor_ip_hash_ring_.RemoveNode(be_ip_address);
}
}
diff --git a/be/src/scheduling/executor-group.h
b/be/src/scheduling/executor-group.h
index 208120d09..1e390e020 100644
--- a/be/src/scheduling/executor-group.h
+++ b/be/src/scheduling/executor-group.h
@@ -146,7 +146,7 @@ class ExecutorGroup {
ExecutorIpAddressMap executor_ip_map_;
/// All executors are kept in a hash ring to allow a consistent mapping from
filenames
- /// to executors.
+ /// to executors. Must be updated whenever executor_map_ changes.
HashRing executor_ip_hash_ring_;
/// The minimum memory limit in bytes for admission across all backends.
Used by
