ambiorix: amxp_signal_read race condition

AmbiorixConnection::read_signal and the lambda in AmbiorixImpl::init_signal_loop may race when
multiple signals with the same path arrive at the same time, because the underlying amxp_signal_read
does not treat the expression evaluation procedure as a critical area, which free/alloc memory
to the expression binary tree when evaluate fields.

this fix adds a mutex to avoid amxp_signal_read race condition, the cost is performance. before
this commit, most signals which have different paths can race without any consequence, only those
have the same path possiblly trigger SIGSEGV. after this commit, all signals handling have to be
synchronized, even if they do not share the same path.

Signed-off-by: Lu Dai <lu.dai@mind.be>

framework/common/include/mapf/common/utils.h

@@ -9,6 +9,7 @@
 #ifndef MAPFUTILS_H_
 #define MAPFUTILS_H_
 
+#include <mutex>
 #include <string>
 
 namespace mapf {
@@ -48,4 +49,9 @@ std::string get_install_path();
 
 } // namespace utils
 } // namespace mapf
+
+namespace beerocks {
+
+extern std::mutex amxp_signal_read_mutex;
+} // namespace beerocks
 #endif // MAPFUTILS_H_

framework/common/utils.cpp

@@ -67,3 +67,9 @@ void copy_string(char *dst, const char *src, size_t dst_len)
 
 } // namespace utils
 } // namespace mapf
+
+namespace beerocks {
+
+std::mutex amxp_signal_read_mutex;
+
+} // namespace beerocks

framework/platform/nbapi/ambiorix_impl.cpp

@@ -15,6 +15,7 @@
 #include <amxd/amxd_transaction.h>
 
 #include <bcl/network/network_utils.h>
+#include <mapf/common/utils.h>
 #include <tlvf/tlvftypes.h>
 
 namespace beerocks {
@@ -189,6 +190,7 @@ bool AmbiorixImpl::init_signal_loop()
         .name = "ambiorix_signal",
         .on_read =
             [&](int fd, EventLoop &loop) {
+                std::lock_guard<std::mutex> guard(amxp_signal_read_mutex);
                 amxp_signal_read();
                 return true;
             },

framework/platform/wbapi/CMakeLists.txt

@@ -32,7 +32,7 @@ add_library(${PROJECT_NAME} ${wbapi_sources})
 set_target_properties(${PROJECT_NAME} PROPERTIES VERSION ${prplmesh_VERSION} SOVERSION ${prplmesh_VERSION_MAJOR})
 set_target_properties(${PROJECT_NAME} PROPERTIES LINK_FLAGS "-Wl,-z,defs")
 
-target_link_libraries(${PROJECT_NAME} PRIVATE ${WBAPI_LIBS} elpp bcl tlvf)
+target_link_libraries(${PROJECT_NAME} PRIVATE ${WBAPI_LIBS} mapfcommon elpp bcl tlvf)
 
 # Install
 target_include_directories(${PROJECT_NAME}

framework/platform/wbapi/ambiorix_connection.cpp

@@ -9,6 +9,7 @@
 #include <easylogging++.h>
 
 #include <bcl/beerocks_backport.h>
+#include <mapf/common/utils.h>
 
 #include "include/ambiorix_connection.h"
 
@@ -199,6 +200,7 @@ int AmbiorixConnection::read_signal()
     const std::lock_guard<std::recursive_mutex> lock(m_mutex);
     int ret;
     do {
+        std::lock_guard<std::mutex> guard(amxp_signal_read_mutex);
         ret = amxp_signal_read();
     } while (ret == 0);
     return ret;