Support AddressSanitizer

.github/workflows/ci-linux.yml

@@ -61,7 +61,7 @@ jobs:
     - uses: ./.github/actions/install-all-dependences
     - uses: ./.github/actions/init-make-config
       with:
-       options: --cc=gcc --cxx=g++ --with-thrift --with-glog --with-rdma  --with-debug-bthread-sche-safety --with-debug-lock --with-bthread-tracer --werror
+       options: --cc=gcc --cxx=g++ --with-thrift --with-glog --with-rdma  --with-debug-bthread-sche-safety --with-debug-lock --with-bthread-tracer --werror --with-asan
     - name: compile
       run: |
         make -j ${{env.proc_num}}
@@ -76,7 +76,7 @@ jobs:
            export CC=gcc && export CXX=g++
            mkdir build
            cd build
-           cmake -DWITH_MESALINK=OFF -DWITH_GLOG=ON -DWITH_THRIFT=ON -DWITH_RDMA=ON -DWITH_DEBUG_BTHREAD_SCHE_SAFETY=ON -DWITH_DEBUG_LOCK=ON -WITH_BTHREAD_TRACER=ON ..
+           cmake -DWITH_MESALINK=OFF -DWITH_GLOG=ON -DWITH_THRIFT=ON -DWITH_RDMA=ON -DWITH_DEBUG_BTHREAD_SCHE_SAFETY=ON -DWITH_DEBUG_LOCK=ON -DWITH_BTHREAD_TRACER=ON -DWITH_ASAN=ON ..
     - name: compile
       run: |
            cd build
@@ -86,7 +86,7 @@ jobs:
     runs-on: ubuntu-22.04
     steps:
     - uses: actions/checkout@v2
-    - run: bazel build --verbose_failures --define with_mesalink=false --define with_glog=true --define with_thrift=true --define with_debug_bthread_sche_safety=true --define with_debug_lock=true -- //... -//example/...
+    - run: bazel build --verbose_failures --define with_mesalink=false --define with_glog=true --define with_thrift=true --define with_debug_bthread_sche_safety=true --define with_debug_lock=true --define with_asan=true -- //... -//example/...
 
   clang-compile-with-make:
     runs-on: ubuntu-22.04
@@ -135,7 +135,7 @@ jobs:
     - uses: ./.github/actions/install-all-dependences
     - uses: ./.github/actions/init-make-config
       with:
-        options: --cc=clang --cxx=clang++ --with-thrift --with-glog --with-rdma  --with-debug-bthread-sche-safety --with-debug-lock --with-bthread-tracer --werror
+        options: --cc=clang --cxx=clang++ --with-thrift --with-glog --with-rdma  --with-debug-bthread-sche-safety --with-debug-lock --with-bthread-tracer --werror --with-asan
     - name: compile
       run: |
            make -j ${{env.proc_num}}
@@ -150,7 +150,7 @@ jobs:
            export CC=clang && export CXX=clang++
            mkdir build
            cd build
-           cmake -DWITH_MESALINK=OFF -DWITH_GLOG=ON -DWITH_THRIFT=ON -DWITH_RDMA=ON -DWITH_DEBUG_BTHREAD_SCHE_SAFETY=ON -DWITH_DEBUG_LOCK=ON -WITH_BTHREAD_TRACER=ON ..
+           cmake -DWITH_MESALINK=OFF -DWITH_GLOG=ON -DWITH_THRIFT=ON -DWITH_RDMA=ON -DWITH_DEBUG_BTHREAD_SCHE_SAFETY=ON -DWITH_DEBUG_LOCK=ON -DWITH_BTHREAD_TRACER=ON -DWITH_ASAN=ON ..
     - name: compile
       run: |
            cd build
@@ -160,7 +160,7 @@ jobs:
     runs-on: ubuntu-22.04
     steps:
     - uses: actions/checkout@v2
-    - run: bazel build --verbose_failures --action_env=CC=clang --define with_mesalink=false --define with_glog=true --define with_thrift=true --define with_debug_bthread_sche_safety=true --define with_debug_lock=true -- //... -//example/...
+    - run: bazel build --verbose_failures --action_env=CC=clang --define with_mesalink=false --define with_glog=true --define with_thrift=true --define with_debug_bthread_sche_safety=true --define with_debug_lock=true --define with_asan=true -- //... -//example/...
 
   clang-unittest:
     runs-on: ubuntu-22.04
@@ -179,3 +179,21 @@ jobs:
         run: |
           cd test
           sh ./run_tests.sh
+
+  clang-unittest-asan:
+    runs-on: ubuntu-22.04
+    steps:
+      - uses: actions/checkout@v2
+      - uses: ./.github/actions/install-essential-dependences
+      - uses: ./.github/actions/init-ut-make-config
+        with:
+          options: --cc=clang-12 --cxx=clang++-12 --with-bthread-tracer --with-asan
+      - name: compile tests
+        run: |
+          cat config.mk
+          cd test
+          make NEED_GPERFTOOLS=0 -j ${{env.proc_num}}
+      - name: run tests
+        run: |
+          cd test
+          sh ./run_tests.sh

BUILD.bazel

@@ -53,6 +53,9 @@ COPTS = [
 }) + select({
     "@platforms//cpu:x86_64": ["-DBRPC_BTHREAD_TRACER"],
     "//conditions:default": [],
+}) + select({
+    "//bazel/config:brpc_with_asan": ["-fsanitize=address"],
+    "//conditions:default": [""],
 })
 
 LINKOPTS = [
@@ -87,7 +90,10 @@ LINKOPTS = [
         "-libverbs",
     ],
     "//conditions:default": [],
-})
+}) + select({
+        "//bazel/config:brpc_with_asan": ["-fsanitize=address"],
+        "//conditions:default": [""],
+  })
 
 genrule(
     name = "config_h",

CMakeLists.txt

@@ -29,6 +29,7 @@ option(WITH_SNAPPY "With snappy" OFF)
 option(WITH_RDMA "With RDMA" OFF)
 option(WITH_DEBUG_BTHREAD_SCHE_SAFETY "With debugging bthread sche safety" OFF)
 option(WITH_DEBUG_LOCK "With debugging lock" OFF)
+option(WITH_ASAN "With AddressSanitizer" OFF)
 option(BUILD_UNIT_TESTS "Whether to build unit tests" OFF)
 option(BUILD_FUZZ_TESTS "Whether to build fuzz tests" OFF)
 option(BUILD_BRPC_TOOLS "Whether to build brpc tools" ON)
@@ -134,6 +135,10 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
 endif()
 
 set(CMAKE_CPP_FLAGS "${CMAKE_CPP_FLAGS} ${DEFINE_CLOCK_GETTIME} -DBRPC_WITH_GLOG=${WITH_GLOG_VAL} -DBRPC_WITH_RDMA=${WITH_RDMA_VAL} -DBRPC_DEBUG_BTHREAD_SCHE_SAFETY=${WITH_DEBUG_BTHREAD_SCHE_SAFETY_VAL} -DBRPC_DEBUG_LOCK=${WITH_DEBUG_LOCK_VAL}")
+if (WITH_ASAN)
+    set(CMAKE_CPP_FLAGS "${CMAKE_CPP_FLAGS} -fsanitize=address")
+    set(CMAKE_C_FLAGS "${CMAKE_CPP_FLAGS} -fsanitize=address")
+endif()
 if(WITH_MESALINK)
     set(CMAKE_CPP_FLAGS "${CMAKE_CPP_FLAGS} -DUSE_MESALINK")
 endif()

bazel/config/BUILD.bazel

@@ -120,4 +120,10 @@ config_setting(
     name = "brpc_with_debug_lock",
     define_values = {"with_debug_lock": "true"},
     visibility = ["//visibility:public"],
+)
+
+config_setting(
+    name = "brpc_with_asan",
+    define_values = {"with_asan": "true"},
+    visibility = ["//visibility:public"],
 )

config_brpc.sh

@@ -38,12 +38,13 @@ else
     LDD=ldd
 fi
 
-TEMP=`getopt -o v: --long headers:,libs:,cc:,cxx:,with-glog,with-thrift,with-rdma,with-mesalink,with-bthread-tracer,with-debug-bthread-sche-safety,with-debug-lock,nodebugsymbols,werror -n 'config_brpc' -- "$@"`
+TEMP=`getopt -o v: --long headers:,libs:,cc:,cxx:,with-glog,with-thrift,with-rdma,with-mesalink,with-bthread-tracer,with-debug-bthread-sche-safety,with-debug-lock,with-asan,nodebugsymbols,werror -n 'config_brpc' -- "$@"`
 WITH_GLOG=0
 WITH_THRIFT=0
 WITH_RDMA=0
 WITH_MESALINK=0
 WITH_BTHREAD_TRACER=0
+WITH_ASAN=0
 BRPC_DEBUG_BTHREAD_SCHE_SAFETY=0
 DEBUGSYMBOLS=-g
 WERROR=
@@ -74,6 +75,7 @@ while true; do
         --with-bthread-tracer) WITH_BTHREAD_TRACER=1; shift 1 ;;
         --with-debug-bthread-sche-safety ) BRPC_DEBUG_BTHREAD_SCHE_SAFETY=1; shift 1 ;;
         --with-debug-lock ) BRPC_DEBUG_LOCK=1; shift 1 ;;
+        --with-asan) WITH_ASAN=1; shift 1 ;;
         --nodebugsymbols ) DEBUGSYMBOLS=; shift 1 ;;
         --werror ) WERROR=-Werror; shift 1 ;;
         -- ) shift; break ;;
@@ -345,10 +347,15 @@ else
     CXXFLAGS="-std=c++0x"
 fi
 
-LEVELDB_HDR=$(find_dir_of_header_or_die leveldb/db.h)
-
 CPPFLAGS=
 
+if [ $WITH_ASAN != 0 ]; then
+  CPPFLAGS="${CPPFLAGS} -fsanitize=address"
+  DYNAMIC_LINKINGS="$DYNAMIC_LINKINGS -fsanitize=address"
+fi
+
+LEVELDB_HDR=$(find_dir_of_header_or_die leveldb/db.h)
+
 if [ $WITH_BTHREAD_TRACER != 0 ]; then
     if [ "$SYSTEM" != "Linux" ] || [ "$(uname -m)" != "x86_64" ]; then
         >&2 $ECHO "bthread tracer is only supported on Linux x86_64 platform"
@@ -425,7 +432,7 @@ append_to_output "STATIC_LINKINGS=$STATIC_LINKINGS"
 append_to_output "DYNAMIC_LINKINGS=$DYNAMIC_LINKINGS"
 
 # CPP means C PreProcessing, not C PlusPlus
-CPPFLAGS="${CPPFLAGS}  -DBRPC_WITH_GLOG=$WITH_GLOG -DBRPC_DEBUG_BTHREAD_SCHE_SAFETY=$BRPC_DEBUG_BTHREAD_SCHE_SAFETY -DBRPC_DEBUG_LOCK=$BRPC_DEBUG_LOCK"
+CPPFLAGS="${CPPFLAGS} -DBRPC_WITH_GLOG=$WITH_GLOG -DBRPC_DEBUG_BTHREAD_SCHE_SAFETY=$BRPC_DEBUG_BTHREAD_SCHE_SAFETY -DBRPC_DEBUG_LOCK=$BRPC_DEBUG_LOCK"
 
 # Avoid over-optimizations of TLS variables by GCC>=4.8
 # See: https://github.com/apache/brpc/issues/1693
@@ -523,7 +530,10 @@ append_to_output "ifeq (\$(NEED_GPERFTOOLS), 1)"
 TCMALLOC_LIB=$(find_dir_of_lib tcmalloc_and_profiler)
 if [ -z "$TCMALLOC_LIB" ]; then
     append_to_output "    \$(error \"Fail to find gperftools\")"
+elif [ $WITH_ASAN != 0 ]; then
+    append_to_output "    \$(error \"gperftools is not compatible with ASAN\")"
 else
+    append_to_output "    CPPFLAGS+=-DBRPC_ENABLE_CPU_PROFILER"
     append_to_output_libs "$TCMALLOC_LIB" "    "
     if [ -f $TCMALLOC_LIB/libtcmalloc.$SO ]; then
         append_to_output "    DYNAMIC_LINKINGS+=-ltcmalloc_and_profiler"

docs/cn/getting_started.md

@@ -352,8 +352,6 @@ bRPC 中使用了 protobuf 内部 API，上游不保证相关 API 的兼容性
 
 [1.8.0](https://github.com/apache/brpc/releases/tag/1.8.0) 中 [#2406](https://github.com/apache/brpc/pull/2406) 和 [#2493](https://github.com/apache/brpc/pull/2493)引入了部分 proto3 语法，所以目前 bRPC 不再兼容 protobuf 2.x 版本。如果你希望使用 2.x 版本，可以使用 1.8.0 之前的 bRPC 版本。
 
-pb 3.x中的Arena至今没被支持。
-
 ## gflags: 2.1-2.2.2
 
 2.1.1 中存在一处已知问题，需要[补丁](https://github.com/gflags/gflags/commit/408061b46974cc8377a8a794a048ecae359ad887)。

docs/cn/sanitizers.md

@@ -0,0 +1,32 @@
+# Sanitizers
+
+新版本的GCC/Clang支持[sanitizers](https://github.com/google/sanitizers)，方便开发者排查代码中的bug。 bRPC对sanitizers提供了一定的支持。
+
+## AddressSanitizer(ASan)
+
+ASan提供了[对协程的支持](https://reviews.llvm.org/D20913)。 在bthread创建、切换、销毁时，让ASan知道当前bthread的栈信息，主要用于维护[fake stack](https://github.com/google/sanitizers/wiki/AddressSanitizerUseAfterReturn)。
+
+bRPC中启用ASan的方法：给config_brpc.sh增加`--with-glog`选项、给cmake增加`-DWITH_GLOG=ON`选项或者给bazel增加`--define with_asan=true`选项。
+
+另外需要注意的是，ASan没法检测非ASan分配内存或者对象池复用内存。所以我们封装了两个宏，让ASan知道内存块是否能被使用。在非ASan环境下，这两个宏什么也不做，没有开销。
+
+```c++
+#include <butil/debug/address_annotations.h>
+
+BUTIL_ASAN_POISON_MEMORY_REGION(addr, size);
+BUTIL_ASAN_UNPOISON_MEMORY_REGION(addr, size);
+```
+
+其他问题：如果ASan报告中new/delete的调用栈不完整，可以通过设置`fast_unwind_on_malloc=0`回溯出完整的调用栈了。需要注意的是`fast_unwind_on_malloc=0`很耗性能。
+
+## ThreadSanitizer(TSan)
+
+待支持（TODO）
+
+## LeakSanitizer(LSan) / MemorySanitizer(MSan) / UndefinedBehaviorSanitizer(UBSan) 
+
+bRPC默认支持这三种sanitizers，编译及链接时加上对应的选项即可启用对应的sanitizers：
+
+1. LSan: `-fsanitize=leak`；
+2. MSan: `-fsanitize=memory`；
+3. UBSan: `-fsanitize=undefined`；

docs/en/getting_started.md

@@ -347,8 +347,6 @@ Please [submit issue](https://github.com/apache/brpc/issues) if you have any pro
 
 [#2406](https://github.com/apache/brpc/pull/2406) and [#2493](https://github.com/apache/brpc/pull/2493) in [version 1.8.0]((https://github.com/apache/brpc/releases/tag/1.8.0)) introduce some proto3 syntax, so currently bRPC is no longer compatible with pb 2.x version. If you want to use pb 2.x version, you can use bRPC version before 1.8.0.
 
-Arena in pb 3.x is not supported yet.
-
 ## gflags: 2.0-2.2.2
 
 [gflags patch](https://github.com/gflags/gflags/commit/408061b46974cc8377a8a794a048ecae359ad887) is required when compiled with 2.1.1.

src/brpc/details/http_message.h

@@ -88,7 +88,7 @@ class HttpMessage {
     bool read_body_progressively() const { return _read_body_progressively; }
 
     void set_read_body_progressively(bool read_body_progressively) {
-        this->_read_body_progressively = read_body_progressively;
+        _read_body_progressively = read_body_progressively;
     }
 
     // Send new parts of the body to the reader. If the body already has some

src/brpc/details/http_parser.cpp

@@ -2191,13 +2191,13 @@ http_parser_init (http_parser *parser, enum http_parser_type t)
 
 const char *
 http_errno_name(enum http_errno err) {
-  assert(err < (sizeof(http_strerror_tab)/sizeof(http_strerror_tab[0])));
+  assert(err < (http_errno)(sizeof(http_strerror_tab)/sizeof(http_strerror_tab[0])));
   return http_strerror_tab[err].name;
 }
 
 const char *
 http_errno_description(enum http_errno err) {
-  assert(err < (sizeof(http_strerror_tab)/sizeof(http_strerror_tab[0])));
+  assert(err < (http_errno)(sizeof(http_strerror_tab)/sizeof(http_strerror_tab[0])));
   return http_strerror_tab[err].description;
 }
 

src/brpc/policy/consistent_hashing_load_balancer.cpp

@@ -19,6 +19,7 @@
 #include <algorithm>                                           // std::set_union
 #include <array>
 #include <gflags/gflags.h>
+#include <openssl/md5.h>
 #include "butil/containers/flat_map.h"
 #include "butil/errno.h"
 #include "butil/strings/string_number_conversions.h"
@@ -102,10 +103,10 @@ bool KetamaReplicaPolicy::Build(ServerId server,
     CHECK(num_replicas % points_per_hash == 0)
         << "Ketam hash replicas number(" << num_replicas << ") should be n*4";
     for (size_t i = 0; i < num_replicas / points_per_hash; ++i) {
-        char host[32];
+        char host[256];
         int len = snprintf(host, sizeof(host), "%s-%lu",
                            endpoint2str(ptr->remote_side()).c_str(), i);
-        unsigned char digest[16];
+        unsigned char digest[MD5_DIGEST_LENGTH];
         MD5HashSignature(host, len, digest);
         for (size_t j = 0; j < points_per_hash; ++j) {
             ConsistentHashingLoadBalancer::Node node;

src/brpc/policy/hasher.cpp

@@ -28,12 +28,12 @@ namespace policy {
 void MD5HashSignature(const void* key, size_t len, unsigned char* results) {
     MD5_CTX my_md5;
     MD5_Init(&my_md5);
-    MD5_Update(&my_md5, (const unsigned char *)key, len);
+    MD5_Update(&my_md5, key, len);
     MD5_Final(results, &my_md5);
 }
 
 uint32_t MD5Hash32(const void* key, size_t len) {
-    unsigned char results[16];
+    unsigned char results[MD5_DIGEST_LENGTH];
     MD5HashSignature(key, len, results);
     return ((uint32_t) (results[3] & 0xFF) << 24) 
             | ((uint32_t) (results[2] & 0xFF) << 16) 
@@ -48,7 +48,7 @@ uint32_t MD5Hash32V(const butil::StringPiece* keys, size_t num_keys) {
         MD5_Update(&ctx, (const unsigned char *)keys[i].data(),
                    keys[i].size());
     }
-    unsigned char results[16];
+    unsigned char results[MD5_DIGEST_LENGTH];
     MD5_Final(results, &ctx);
     return ((uint32_t) (results[3] & 0xFF) << 24) 
             | ((uint32_t) (results[2] & 0xFF) << 16) 

src/brpc/policy/http_rpc_protocol.cpp

@@ -1698,7 +1698,7 @@ void HttpContext::CheckProgressiveRead(const void* arg, Socket *socket) {
         header().uri().path(), (Server *)arg,
         const_cast<std::string *>(&header().unresolved_path()));
     if (sp != NULL && sp->params.enable_progressive_read) {
-        this->set_read_body_progressively(true);
+        set_read_body_progressively(true);
         socket->read_will_be_progressive(CONNECTION_TYPE_SHORT);
     }
 }

src/brpc/socket.cpp

@@ -508,9 +508,11 @@ void Socket::ReturnSuccessfulWriteRequest(Socket::WriteRequest* p) {
     DCHECK(p->data.empty());
     AddOutputMessages(1);
     const bthread_id_t id_wait = p->id_wait;
+    bool is_notify_on_success = p->is_notify_on_success();
     butil::return_object(p);
+    // Do not access `p' after it is returned to ObjectPool.
     if (id_wait != INVALID_BTHREAD_ID) {
-        if (p->is_notify_on_success() && !Failed()) {
+        if (is_notify_on_success && !Failed()) {
             bthread_id_error(id_wait, 0);
         } else {
             NotifyOnFailed(id_wait);

src/bthread/bthread.cpp

@@ -350,7 +350,7 @@ bthread_t bthread_self(void) {
     bthread::TaskGroup* g = bthread::tls_task_group;
     // note: return 0 for main tasks now, which include main thread and
     // all work threads. So that we can identify main tasks from logs
-    // more easily. This is probably questionable in future.
+    // more easily. This is probably questionable in the future.
     if (g != NULL && !g->is_current_main_task()/*note*/) {
         return g->current_tid();
     }
@@ -361,6 +361,13 @@ int bthread_equal(bthread_t t1, bthread_t t2) {
     return t1 == t2;
 }
 
+#ifdef BUTIL_USE_ASAN
+// Fixme!!!
+// The noreturn `bthread_exit' may cause a warning of ASan, but does not abort the program.
+//
+// ==94463==WARNING: ASan is ignoring requested __asan_handle_no_return: stack type: default top: 0x00016dd7f000; bottom 0x00010b1a4000; size: 0x000062bdb000 (1656598528)
+// False positive error reports may follow
+#endif // BUTIL_USE_ASAN
 void bthread_exit(void* retval) {
     bthread::TaskGroup* g = bthread::tls_task_group;
     if (g != NULL && !g->is_current_main_task()) {

src/bthread/execution_queue.cpp

@@ -22,7 +22,6 @@
 #include "bthread/execution_queue.h"
 
 #include "butil/memory/singleton_on_pthread_once.h"
-#include "butil/object_pool.h"           // butil::get_object
 #include "butil/resource_pool.h"         // butil::get_resource
 #include "butil/threading/platform_thread.h"
 

src/bthread/execution_queue_inl.h

@@ -27,8 +27,9 @@
 #include "butil/memory/scoped_ptr.h"     // butil::scoped_ptr
 #include "butil/logging.h"               // LOG
 #include "butil/time.h"                  // butil::cpuwide_time_ns
-#include "bvar/bvar.h"                  // bvar::Adder
-#include "bthread/butex.h"              // butex_construct
+#include "butil/object_pool.h"           // butil::get_object
+#include "bvar/bvar.h"                   // bvar::Adder
+#include "bthread/butex.h"               // butex_construct
 #include "butil/synchronization/condition_variable.h"
 
 namespace bthread {
@@ -585,4 +586,11 @@ inline int ExecutionQueueBase::dereference() {
 
 }  // namespace bthread
 
+namespace butil {
+// `TaskNode::cancel' may access the TaskNode object returned to the ObjectPool<TaskNode>,
+// so ObjectPool<TaskNode> can not poison the memory region of TaskNode.
+template <>
+struct ObjectPoolWithASanPoison<bthread::TaskNode> : false_type {};
+} // namespace butil
+
 #endif  //BTHREAD_EXECUTION_QUEUE_INL_H

src/bthread/stack.cpp

@@ -139,7 +139,7 @@ void deallocate_stack_storage(StackStorage* s) {
         return;
     }
     s_stack_count.fetch_sub(1, butil::memory_order_relaxed);
-    if (s->guardsize <= 0) {
+    if (s->guardsize == 0) {
         free((char*)s->bottom - memsize);
     } else {
         munmap((char*)s->bottom - memsize, memsize);