From 044ce7240901832b58b96ed624b7d5ca4a0af451 Mon Sep 17 00:00:00 2001 From: QinZuoyan Date: Fri, 29 Jun 2018 16:12:11 +0800 Subject: [PATCH] rdsn: fix asio_rpc_session bug which may cause double free (#115) --- include/dsn/c/api_utilities.h | 1 + include/dsn/utility/autoref_ptr.h | 16 +++++++++++----- include/dsn/utility/endians.h | 9 +++------ src/core/tools/common/asio_net_provider.cpp | 5 ++++- src/core/tools/common/asio_rpc_session.cpp | 2 -- src/core/tools/common/thrift_message_parser.cpp | 1 + 6 files changed, 20 insertions(+), 14 deletions(-) diff --git a/include/dsn/c/api_utilities.h b/include/dsn/c/api_utilities.h index e153d19a7c..1f5a233c15 100644 --- a/include/dsn/c/api_utilities.h +++ b/include/dsn/c/api_utilities.h @@ -36,6 +36,7 @@ #pragma once #include +#include /*! @defgroup logging Logging Service diff --git a/include/dsn/utility/autoref_ptr.h b/include/dsn/utility/autoref_ptr.h index 67a3f5efd6..c525574cb6 100644 --- a/include/dsn/utility/autoref_ptr.h +++ b/include/dsn/utility/autoref_ptr.h @@ -46,15 +46,18 @@ class ref_counter virtual ~ref_counter() { - if (_magic != 0xdeadbeef) { // 3735928559 - assert(!"memory corrupted, could be double free or others"); - } else { - _magic = 0xfacedead; // 4207861421 - } + // 0xdeadbeef: 3735928559 + assert(_magic == 0xdeadbeef); + + // 0xfacedead: 4207861421 + _magic = 0xfacedead; } void add_ref() { + // 0xdeadbeef: 3735928559 + assert(_magic == 0xdeadbeef); + // Increasing the reference counter can always be done with memory_order_relaxed: // New references to an object can only be formed from an existing reference, // and passing an existing reference from one thread to another must already provide any @@ -64,6 +67,9 @@ class ref_counter void release_ref() { + // 0xdeadbeef: 3735928559 + assert(_magic == 0xdeadbeef); + // It is important to enforce any possible access to the object in one thread //(through an existing reference) to happen before deleting the object in a different // thread. diff --git a/include/dsn/utility/endians.h b/include/dsn/utility/endians.h index 40be705931..550d3426f5 100644 --- a/include/dsn/utility/endians.h +++ b/include/dsn/utility/endians.h @@ -5,8 +5,8 @@ #pragma once #include +#include #include -#include #include namespace dsn { @@ -57,7 +57,7 @@ class data_output void ensure(size_t sz) { size_t cap = _end - _ptr; - dassert(cap >= sz, "capacity %zu is not enough for %zu", cap, sz); + assert(cap >= sz); } private: @@ -107,10 +107,7 @@ class data_input _size -= sz; } - void ensure(size_t sz) - { - dassert(_size >= sz, " content(%zu) is not enough for reading %zu size", _size, sz); - } + void ensure(size_t sz) { assert(_size >= sz); } private: const char *_p{nullptr}; diff --git a/src/core/tools/common/asio_net_provider.cpp b/src/core/tools/common/asio_net_provider.cpp index 288a820f32..f723db23a3 100644 --- a/src/core/tools/common/asio_net_provider.cpp +++ b/src/core/tools/common/asio_net_provider.cpp @@ -137,7 +137,10 @@ void asio_network_provider::do_accept() (std::shared_ptr &)socket, null_parser, false); - this->on_server_session_accepted(s); + on_server_session_accepted(s); + + // we should start read immediately after the rpc session is completely created. + s->start_read_next(); } do_accept(); diff --git a/src/core/tools/common/asio_rpc_session.cpp b/src/core/tools/common/asio_rpc_session.cpp index bdb8da905a..fcbe04ba9b 100644 --- a/src/core/tools/common/asio_rpc_session.cpp +++ b/src/core/tools/common/asio_rpc_session.cpp @@ -170,8 +170,6 @@ asio_rpc_session::asio_rpc_session(asio_network_provider &net, : rpc_session(net, remote_addr, parser, is_client), _socket(socket) { set_options(); - if (!is_client) - start_read_next(); } void asio_rpc_session::on_failure(bool is_write) diff --git a/src/core/tools/common/thrift_message_parser.cpp b/src/core/tools/common/thrift_message_parser.cpp index f5c6716e85..2325e96311 100644 --- a/src/core/tools/common/thrift_message_parser.cpp +++ b/src/core/tools/common/thrift_message_parser.cpp @@ -268,6 +268,7 @@ dsn::message_ex *thrift_message_parser::parse_message(const thrift_message_heade dsn_hdr->id = seqid; strncpy(dsn_hdr->rpc_name, fname.c_str(), DSN_MAX_TASK_CODE_NAME_LENGTH); + dsn_hdr->rpc_name[DSN_MAX_TASK_CODE_NAME_LENGTH - 1] = '\0'; dsn_hdr->gpid.set_app_id(thrift_header.app_id); dsn_hdr->gpid.set_partition_index(thrift_header.partition_index); dsn_hdr->client.timeout_ms = thrift_header.client_timeout;