[doris] branch branch-2.0 updated: [bugfix](fragment mgr) heap used after free in fragment manager when query is cancelled (#23817)

[yiguolei]

This is an automated email from the ASF dual-hosted git repository.

yiguolei pushed a commit to branch branch-2.0
in repository https://gitbox.apache.org/repos/asf/doris.git


The following commit(s) were added to refs/heads/branch-2.0 by this push:
     new e2a2194a50 [bugfix](fragment mgr) heap used after free in fragment 
manager when query is cancelled (#23817)
e2a2194a50 is described below

commit e2a2194a500fae1b4b3c431c47f37d2cf23f9c8a
Author: yiguolei <676222...@qq.com>
AuthorDate: Mon Sep 4 12:20:16 2023 +0800

    [bugfix](fragment mgr) heap used after free in fragment manager when query 
is cancelled (#23817)
    
    Co-authored-by: yiguolei <yiguo...@gmail.com>
---
 be/src/runtime/fragment_mgr.cpp | 8 +++++++-
 be/src/runtime/fragment_mgr.h   | 5 +++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/be/src/runtime/fragment_mgr.cpp b/be/src/runtime/fragment_mgr.cpp
index 5036878d75..409a32a128 100644
--- a/be/src/runtime/fragment_mgr.cpp
+++ b/be/src/runtime/fragment_mgr.cpp
@@ -798,7 +798,13 @@ Status FragmentMgr::exec_plan_fragment(const 
TExecPlanFragmentParams& params,
                                    params.query_options.enable_pipeline_engine;
     RETURN_IF_ERROR(
             _get_query_ctx(params, params.params.query_id, 
pipeline_engine_enabled, query_ctx));
-    query_ctx->fragment_ids.push_back(fragment_instance_id);
+    {
+        // Need lock here, because it will modify fragment ids and std::vector 
may resize and reallocate
+        // memory, but query_is_canncelled will traverse the vector, it will 
core.
+        // query_is_cancelled is called in allocator, we has to avoid dead 
lock.
+        std::lock_guard<std::mutex> lock(_lock);
+        query_ctx->fragment_ids.push_back(fragment_instance_id);
+    }
 
     exec_state.reset(
             new FragmentExecState(query_ctx->query_id, 
params.params.fragment_instance_id,
diff --git a/be/src/runtime/fragment_mgr.h b/be/src/runtime/fragment_mgr.h
index 8ca58ccffa..dca60b91e7 100644
--- a/be/src/runtime/fragment_mgr.h
+++ b/be/src/runtime/fragment_mgr.h
@@ -159,6 +159,11 @@ private:
     // This is input params
     ExecEnv* _exec_env;
 
+    // The lock should only be used to protect the structures in fragment 
manager. Has to be
+    // used in a very small scope because it may dead lock. For example, if 
the _lock is used
+    // in prepare stage, the call path is  prepare --> expr prepare --> may 
call allocator
+    // when allocate failed, allocator may call query_is_cancelled, query is 
callced will also
+    // call _lock, so that there is dead lock.
     std::mutex _lock;
 
     std::condition_variable _cv;


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For additional commands, e-mail: commits-h...@doris.apache.org