Repository: pinpoint-apm/pinpoint-c-agent (language: python)

Issue:
signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x0
**Describe the bug**

We found the server crash with 

```
goroutine 23733 [syscall]:
runtime.cgocall(0xa7e790, 0xc02058beb0, 0x1)
	/usr/local/go/src/runtime/cgocall.go:154 +0x5b fp=0xc02058be80 sp=0xc02058be48 pc=0x406ebb
github.com/pinpoint-apm/go-aop-agent/common._Cfunc_pinpoint_end_trace(0x20bd, 0xc000000000)
	_cgo_gotypes.go:249 +0x45 fp=0xc02058beb0 sp=0xc02058be80 pc=0xa6b705
github.com/pinpoint-apm/go-aop-agent/common.Pinpoint_end_trace(...)
	/root/go/pkg/mod/github.com/pinpoint-apm/go-aop-agent@v1.0.5-0.20220619065050-436daf0d5e03/common/pinpointCommon.go:302
github.com/pinpoint-apm/go-aop-agent/common.PinTranscation.func2(0xc02058bf2d, 0xc0000020bd)
	/root/go/pkg/mod/github.com/pinpoint-apm/go-aop-agent@v1.0.5-0.20220619065050-436daf0d5e03/common/trace.go:192 +0x33 fp=0xc02058bee8 sp=0xc02058beb0 pc=0xa6d073
github.com/pinpoint-apm/go-aop-agent/common.PinTranscation(0xc020f1b080, 0xc020e584e0, 0xcc80c0, 0xc01ed79320)
	/root/go/pkg/mod/github.com/pinpoint-apm/go-aop-agent@v1.0.5-0.20220619065050-436daf0d5e03/common/trace.go:206 +0xbc fp=0xc02058bf88 sp=0xc02058bee8 pc=0xa6a77c
......
```

**To Reproduce**

Cannot reproduce it !!!


** version **

0.4.20





Discussion / hints:

Fix patch:
diff --git a/common/CMakeLists.txt b/common/CMakeLists.txt
index cacaa16dc..f3ec589ec 100644
--- a/common/CMakeLists.txt
+++ b/common/CMakeLists.txt
@@ -1,5 +1,5 @@
 cmake_minimum_required(VERSION 3.13)
-project(common VERSION 0.4.20 DESCRIPTION  "pinpoint common library")
+project(common VERSION 0.4.21 DESCRIPTION  "pinpoint common library")
 
 set(CMAKE_CXX_STANDARD 11)
 
diff --git a/common/src/NodePool/PoolManager.cpp b/common/src/NodePool/PoolManager.cpp
index b0adc5ff2..7c251bfee 100644
--- a/common/src/NodePool/PoolManager.cpp
+++ b/common/src/NodePool/PoolManager.cpp
@@ -32,40 +32,55 @@
 
 namespace NodePool
 {
-    void freeNodeTree(NodeID rootId)
+
+    void freeNodeTree(NodeID nodeId)
     {
-        if (rootId == E_INVALID_NODE || rootId == E_ROOT_NODE)
+        if (nodeId == E_INVALID_NODE || nodeId == E_ROOT_NODE)
         {
             return;
         }
 
-        NodeID child_id = PoolManager::getInstance().Restore(rootId);
-        while (child_id != E_INVALID_NODE)
+        NodeID child_id, next_id;
+
+        if (PoolManager::getInstance().Restore(nodeId, child_id, next_id))
         {
-            TraceNode &child = PoolManager::getInstance().Take(child_id);
-            child_id = child.mNextId;
-            freeNodeTree(child.mPoolIndex);
+            if (next_id != E_INVALID_NODE)
+            {
+                freeNodeTree(next_id);
+            }
+
+            if (child_id != E_INVALID_NODE)
+            {
+                freeNodeTree(child_id);
+            }
         }
-    }
 
-    NodeID PoolManager::Restore(NodeID id)
+        // while (child_id != E_INVALID_NODE)
+        // {
+        //     WrapperTraceNode r_node = PoolManager::getInstance().GetWrapperNode(child_id);
+        //     next_id = r_node->mNextId;
+        //     freeNodeTree(child_id);
+        //     child_id = next_id;
+        // }
+
+        // PoolManager::getInstance().Restore(rootId);
+    }
+    bool PoolManager::Restore(NodeID id, NodeID &child_id, NodeID &next_id)
     {
-        NodeID child = E_INVALID_NODE;
         for (int i = 0; i < 1000; i++)
         {
-            if (this->_restore(id, child, false))
+            if (this->_restore(id, child_id, next_id, false))
             {
-                return child;
+                return true;
             }
             std::this_thread::sleep_for(std::chrono::milliseconds(1));
         }
         pp_trace("[🐛]Restore node failed: #%d; node restore forcefully", id);
-        this->_restore(id, child, true);
-        return child;
+        return this->_restore(id, child_id, next_id, true);
     }
 
     // avoiding `locking and waitting`
-    bool PoolManager::_restore(NodeID id, NodeID &child, bool force)
+    bool PoolManager::_restore(NodeID id, NodeID &child_id, NodeID &next_id, bool force)
     {
         std::lock_guard<std::mutex> _safe(this->_lock);
 
@@ -74,7 +89,8 @@ namespace NodePool
         if (this->indexInAliveVec(index) == false)
         {
             pp_trace("%d not alive !!!", id);
-            child = E_INVALID_NODE;
+            child_id = E_INVALID_NODE;
+            next_id = E_INVALID_NODE;
             return true;
         }
 
@@ -83,12 +99,14 @@ namespace NodePool
 
         if (node.checkZoreRef() == false && force == false)
         {
+            // DO NOT TOUCH out id
             return false;
         }
         else
         {
             this->_aliveNodeSet[index] = false;
-            child = node.mChildHeadIndex;
+            child_id = node.mChildHeadId;
+            next_id = node.mNextId;
             this->_freeNodeList.push(index);
             return true;
         }
diff --git a/common/src/NodePool/PoolManager.h b/common/src/NodePool/PoolManager.h
index cf09c1a81..cac2f8d0f 100644
--- a/common/src/NodePool/PoolManager.h
+++ b/common/src/NodePool/PoolManager.h
@@ -31,6 +31,7 @@
 
 namespace NodePool
 {
+
     class PoolManager
     {
     private:
@@ -40,9 +41,15 @@ namespace NodePool
 
         TraceNode &_take(NodeID id);
 
-        bool _restore(NodeID id, NodeID &child, bool force);
+        bool _restore(NodeID id, NodeID &child_id, NodeID &next_id, bool force);
 
     public:
+        /**
+         * @brief take is not safe, you should avoid use under MT
+         * note: try PoolManager::getInstance().GetWrapperNode
+         * @param id
+         * @return TraceNode&
+         */
         inline TraceNode &Take(NodeID id = E_ROOT_NODE)
         {
             std::lock_guard<std::mutex> _safe(this->_lock);
@@ -55,12 +62,21 @@ namespace NodePool
             TraceNode &e = this->_take(id);
             return WrapperTraceNode(&e);
         }
-
-        NodeID Restore(NodeID id);
-
-        inline void Restore(TraceNode &node)
+        /**
+         * @brief restore id->traceNode to pool
+         *
+         * @param id
+         * @param child_id child id of `id`, if exist
+         * @param next_id next id of `id`, if exist
+         * @return true
+         * @return false
+         */
+        bool Restore(NodeID id, NodeID &child_id, NodeID &next_id);
+
+        inline bool Restore(TraceNode &node)
         {
-            this->Restore(node.getId());
+            NodeID node1, node2;
+            return this->Restore(node.getId(), node1, node2);
         }
 
         uint32_t totoalNodesCount()
diff --git a/common/src/NodePool/TraceNode.cpp b/common/src/NodePool/TraceNode.cpp
index 44816b70d..91eb1e9db 100644
--- a/common/src/NodePool/TraceNode.cpp
+++ b/common/src/NodePool/TraceNode.cpp
@@ -67,12 +67,12 @@ namespace NodePool
     {
         std::lock_guard<std::mutex> _safe(this->mlock);
 
-        if (this->mChildHeadIndex != E_INVALID_NODE)
-            child->mNextId = this->mChildHeadIndex;
+        if (this->mChildHeadId != E_INVALID_NODE)
+            child->mNextId = this->mChildHeadId;
 
-        this->mChildHeadIndex = child->mPoolIndex;
+        this->mChildHeadId = child->mPoolIndex;
 
-        child->mParentIndex = this->mPoolIndex;
+        child->mParentId = this->mPoolIndex;
         child->mRootIndex = this->mRootIndex;
         child->root_start_time = this->root_start_time;
     }
@@ -108,7 +108,7 @@ namespace NodePool
     // {
     //     std::lock_guard<std::mutex> _safe(this->mlock);
     //     this->mRootIndex = root->mPoolIndex;
-    //     this->mParentIndex = parent->mPoolIndex;
+    //     this->mParentId = parent->mPoolIndex;
     //     this->root_start_time = root->root_start_time;
     // }
 
@@ -127,7 +127,7 @@ namespace NodePool
             int64_t min = std::stoll(value);
             auto cb = [=]() -> bool
             {
-                pp_trace("checkOpt: TraceMinTimeMs:%ld cumulative_time:%lu", min, this->cumulative_time);
+                pp_trace("checkOpt: #%d TraceMinTimeMs:%ld cumulative_time:%lu", this->mPoolIndex, min, this->cumulative_time);
                 if ((int64_t)this->cumulative_time >= min)
                     return true;
                 return false;
diff --git a/common/src/NodePool/TraceNode.h b/common/src/NodePool/TraceNode.h
index 3d3d7d031..b2b687117 100644
--- a/common/src/NodePool/TraceNode.h
+++ b/common/src/NodePool/TraceNode.h
@@ -40,6 +40,13 @@ namespace NodePool
     using Context::LongContextType;
     using Context::StringContextType;
     class TraceNode;
+
+    typedef enum
+    {
+        E_RET_CHILD_ID,
+        E_RET_NEXT_ID,
+    } E_OUT_TYPE;
+
     class WrapperTraceNode
     {
     public:
@@ -67,11 +74,11 @@ namespace NodePool
     class TraceNode
     {
     public:
-        NodeID mNextId;         // equal brother node
-        NodeID mChildHeadIndex; // subtree/child tree
+        NodeID mNextId;      // equal brother node
+        NodeID mChildHeadId; // subtree/child tree
 
-        NodeID mParentIndex; // parent Id [end_trace] avoiding re-add
-        NodeID mRootIndex;   // highway to root node
+        NodeID mParentId;  // parent Id [end_trace] avoiding re-add
+        NodeID mRootIndex; // highway to root node
         NodeID mPoolIndex;
         // int64_t mUID;
         // int64_t mParentUID;
@@ -103,23 +110,23 @@ namespace NodePool
 
         inline bool isLeaf() const
         {
-            return this->mChildHeadIndex == E_INVALID_NODE;
+            return this->mChildHeadId == E_INVALID_NODE;
         }
 
         // private:
-        //     inline bool isRelated(NodeID mParentIndex)
+        //     inline bool isRelated(NodeID mParentId)
         //     {
-        //         assert(mParentIndex != mPoolIndex);
+        //         assert(mParentId != mPoolIndex);
         //         assert(this->mStartParentId != E_INVALID_NODE);
-        //         assert(mParentIndex != E_INVALID_NODE);
-        //         if (this->mParentIndex == this->mStartParentId)
+        //         assert(mParentId != E_INVALID_NODE);
+        //         if (this->mParentId == this->mStartParentId)
         //         {
-        //             assert(this->mParentIndex == mParentIndex);
+        //             assert(this->mParentId == mParentId);
         //             return true;
         //         }
         //         else
         //         {
-        //             assert(this->mParentIndex == mPoolIndex);
+        //             assert(this->mParentId == mPoolIndex);
         //             return false;
         //         }
         //     }
@@ -186,6 +193,20 @@ namespace NodePool
             this->_context[key] = context;
         }
 
+        NodeID fetchOutType(E_OUT_TYPE type)
+        {
+            switch (type)
+            {
+            case E_RET_CHILD_ID:
+                return this->mChildHeadId;
+            case E_RET_NEXT_ID:
+                return this->mNextId;
+            default:
+                pp_trace("[🐛]fetchOutType node:%d unknow type ", this->mPoolIndex);
+                return E_INVALID_NODE;
+            }
+        }
+
     public:
         TraceNode &operator=(const TraceNode &) = delete;
         TraceNode(const TraceNode &) = delete;
@@ -245,8 +266,8 @@ namespace NodePool
         inline void resetRelative()
         {
             this->mNextId = E_INVALID_NODE;
-            this->mChildHeadIndex = E_INVALID_NODE;
-            this->mParentIndex = mPoolIndex;
+            this->mChildHeadId = E_INVALID_NODE;
+            this->mParentId = mPoolIndex;
             this->mRootIndex = mPoolIndex;
         }
 
@@ -287,12 +308,12 @@ namespace NodePool
         {
             std::lock_guard<std::mutex> _safe(this->mlock);
             char pbuf[1024] = {0};
-            int len = snprintf(pbuf, 1024, "mNextId:%d mChildListHeaderId:%d mParentIndex:%d mRootIndex:%d mPoolIndex:%d \n"
+            int len = snprintf(pbuf, 1024, "mNextId:%d mChildListHeaderId:%d mParentId:%d mRootIndex:%d mPoolIndex:%d \n"
                                            "start_time:%lu,fetal_error_time:%lu,limit:%lu,cumulative_time:%lu,root_start_time:%lu,mHasExp:%d \n"
                                            "_mRef:%d\n"
                                            "_value:%s \n"
                                            "_context size:%lu,_callback:%lu \n",
-                               (int)this->mNextId, (int)this->mChildHeadIndex, (int)this->mParentIndex, (int)this->mRootIndex, (int)this->mPoolIndex,
+                               (int)this->mNextId, (int)this->mChildHeadId, (int)this->mParentId, (int)this->mRootIndex, (int)this->mPoolIndex,
                                this->start_time, this->fetal_error_time, this->limit, this->cumulative_time, this->root_start_time, this->mHasExp,
                                this->_mRef.load(),
                                this->_value.toStyledString().c_str(),
diff --git a/common/src/Util/Helper.cpp b/common/src/Util/Helper.cpp
index 1e0a3fbfb..a1b1c630a 100644
--- a/common/src/Util/Helper.cpp
+++ b/common/src/Util/Helper.cpp
@@ -30,10 +30,11 @@ namespace Helper
     namespace Json = AliasJson;
     using Cache::NodeTreeWriter;
     using NodePool::PoolManager;
+
     static NodeTreeWriter _writer;
     static ConnectionPool::SpanConnectionPool _con_pool;
     static std::once_flag _pool_init_flag;
-    static Json::Value mergeChildren(TraceNode &node);
+    static Json::Value mergeChildren(WrapperTraceNode &node);
     uint64_t get_current_msec_stamp() noexcept
     {
         std::chrono::time_point<std::chrono::system_clock> now = std::chrono::system_clock::now();
@@ -46,11 +47,12 @@ namespace Helper
         return _writer.write(value);
     }
 
-    static void gatcherChildDetailByReverse(Json::Value &detail, TraceNode &head)
+    static void gatcherChildDetailByReverse(Json::Value &detail, WrapperTraceNode &head)
     {
-        if (head.mNextId != E_INVALID_NODE)
+        if (head->mNextId != E_INVALID_NODE)
         {
-            TraceNode &next = PoolManager::getInstance().Take(head.mNextId);
+            // TraceNode &next = PoolManager::getInstance().Take(head.mNextId);
+            WrapperTraceNode next = PoolManager::getInstance().GetWrapperNode(head->mNextId);
             gatcherChildDetailByReverse(detail, next);
         }
         Json::Value childrenDetail = mergeChildren(head);
@@ -60,32 +62,41 @@ namespace Helper
         }
     }
 
-    Json::Value mergeChildren(TraceNode &node)
+    Json::Value mergeChildren(WrapperTraceNode &node)
     {
-        if (node.checkOpt() == false)
+        if (node->checkOpt() == false)
         {
             return Json::Value();
         }
-        else if (!node.isLeaf())
+        else if (!node->isLeaf())
         {
-            TraceNode &child = PoolManager::getInstance().Take(node.mChildHeadIndex);
+            // TraceNode &child = PoolManager::getInstance().Take(node.mChildHeadIndex);
+            WrapperTraceNode child = PoolManager::getInstance().GetWrapperNode(node->mChildHeadId);
+
             Json::Value childTraceDetail;
             gatcherChildDetailByReverse(childTraceDetail, child);
-            node.AddTraceDetail("calls", childTraceDetail);
+            node->AddTraceDetail("calls", childTraceDetail);
         }
-        return node.getJsValue();
+        return node->getJsValue();
     }
 
-    Json::Value mergeTraceNodeTree(NodeID &Id) noexcept
+    Json::Value mergeTraceNodeTree(NodeID Id) noexcept
     {
-        return mergeTraceNodeTree(PoolManager::getInstance().Take(Id));
+        WrapperTraceNode root = PoolManager::getInstance().GetWrapperNode(Id);
+        return mergeTraceNodeTree(root);
     }
 
-    Json::Value mergeTraceNodeTree(TraceNode &root)
+    Json::Value mergeTraceNodeTree(WrapperTraceNode &root)
     {
         return mergeChildren(root);
     }
 
+    Json::Value mergeTraceNodeTree(TraceNode &root)
+    {
+        WrapperTraceNode wroot(&root);
+        return mergeChildren(wroot);
+    }
+
     TransConnection getConnection()
     {
 
diff --git a/common/src/Util/Helper.h b/common/src/Util/Helper.h
index 365dd3b3f..08477b1d6 100644
--- a/common/src/Util/Helper.h
+++ b/common/src/Util/Helper.h
@@ -42,6 +42,7 @@ namespace Helper
     namespace Json = AliasJson;
     using ConnectionPool::TransConnection;
     using NodePool::TraceNode;
+    using NodePool::WrapperTraceNode;
     using std::chrono::duration;
     using std::chrono::duration_cast;
     using std::chrono::milliseconds;
@@ -70,8 +71,9 @@ namespace Helper
     std::string node_tree_to_string(const Json::Value &value);
 
     Json::Value mergeTraceNodeTree(TraceNode &root);
+    Json::Value mergeTraceNodeTree(WrapperTraceNode &root);
 
-    Json::Value mergeTraceNodeTree(NodeID &Id) noexcept;
+    Json::Value mergeTraceNodeTree(NodeID Id) noexcept;
     TransConnection getConnection();
 
     void freeConnection(TransConnection &);
diff --git a/common/src/common.cpp b/common/src/common.cpp
index 2b5aa04cc..03183e3e9 100644
--- a/common/src/common.cpp
+++ b/common/src/common.cpp
@@ -149,7 +149,7 @@ NodeID do_end_trace(NodeID Id)
     {
         r_node->endTimer();
         r_node->convertToSpanEvent();
-        return r_node->mParentIndex;
+        return r_node->mParentId;
         // check opt
         // if (r_node->checkOpt() == true)
         // {


Test patch:
diff --git a/common/test/test_node_pool.cc b/common/test/test_node_pool.cc
index c02108352..c248cf0ce 100644
--- a/common/test/test_node_pool.cc
+++ b/common/test/test_node_pool.cc
@@ -13,8 +13,11 @@ TEST(poolManger, get_and_give_back)
     TraceNode &_node = pool.Take();
     void *p = &_node;
     NodeID id = _node.getId();
+    NodeID child, next;
     // give back
-    pool.Restore(id);
+    pool.Restore(id, child, next);
+    EXPECT_EQ(child, E_INVALID_NODE);
+    EXPECT_EQ(next, E_INVALID_NODE);
     TraceNode &_node_01 = pool.Take();
 
     // ref current
diff --git a/common/test/test_util.cc b/common/test/test_util.cc
index dc8d4d01b..a2f0176c7 100644
--- a/common/test/test_util.cc
+++ b/common/test/test_util.cc
@@ -124,12 +124,12 @@ TEST(util, mergeTraceNodeTree)
     TraceNode &n3 = PoolManager::getInstance().Take();
     TraceNode &n4 = PoolManager::getInstance().Take();
 
-    n2.mParentIndex = n1.mPoolIndex;
-    n1.mChildHeadIndex = n2.mPoolIndex;
-    n3.mParentIndex = n2.mPoolIndex;
-    n4.mParentIndex = n2.mPoolIndex;
+    n2.mParentId = n1.mPoolIndex;
+    n1.mChildHeadId = n2.mPoolIndex;
+    n3.mParentId = n2.mPoolIndex;
+    n4.mParentId = n2.mPoolIndex;
     n3.mNextId = n4.mPoolIndex;
-    n2.mChildHeadIndex = n3.mPoolIndex;
+    n2.mChildHeadId = n3.mPoolIndex;
 
     Json::Value var = Helper::mergeTraceNodeTree(n1);
     std::cout << var.toStyledString();