kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From t...@apache.org
Subject incubator-kudu git commit: Fix use-after free in accessing InboundCall's MethodInfo
Date Thu, 21 Apr 2016 21:16:03 GMT
Repository: incubator-kudu
Updated Branches:
  refs/heads/master bde7294fc -> b6bab2092


Fix use-after free in accessing InboundCall's MethodInfo

This fixes a regression from 38e16b71b881ad1f276da918cae8977a383906a5 in which
a call is being responded to after the underlying service has shut down. In
this case, the call was holding on to a raw pointer to the RpcMethodInfo
object, and that object was being destroyed when the service stopped.

The fix here is to make it reference-counted. This has a slight penalty of some
extra atomic refcount operations, but any other fix would likely be much more
complex.

No new test since client-test was already failing nearly 50% in ASAN/TSAN
builds.

Change-Id: I30304fe6bd2074578e5cfc09ee18ef616d43e687
Reviewed-on: http://gerrit.cloudera.org:8080/2833
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <adar@cloudera.com>


Project: http://git-wip-us.apache.org/repos/asf/incubator-kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-kudu/commit/b6bab209
Tree: http://git-wip-us.apache.org/repos/asf/incubator-kudu/tree/b6bab209
Diff: http://git-wip-us.apache.org/repos/asf/incubator-kudu/diff/b6bab209

Branch: refs/heads/master
Commit: b6bab20922def643c9b57edef7313ea940dda068
Parents: bde7294
Author: Todd Lipcon <todd@apache.org>
Authored: Thu Apr 21 11:10:57 2016 -0700
Committer: Todd Lipcon <todd@apache.org>
Committed: Thu Apr 21 21:10:25 2016 +0000

----------------------------------------------------------------------
 src/kudu/rpc/inbound_call.h     | 9 +++++----
 src/kudu/rpc/protoc-gen-krpc.cc | 2 +-
 src/kudu/rpc/service_if.h       | 4 ++--
 3 files changed, 8 insertions(+), 7 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/b6bab209/src/kudu/rpc/inbound_call.h
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/inbound_call.h b/src/kudu/rpc/inbound_call.h
index cc35383..7796ef9 100644
--- a/src/kudu/rpc/inbound_call.h
+++ b/src/kudu/rpc/inbound_call.h
@@ -26,6 +26,7 @@
 #include "kudu/gutil/macros.h"
 #include "kudu/gutil/ref_counted.h"
 #include "kudu/rpc/remote_method.h"
+#include "kudu/rpc/service_if.h"
 #include "kudu/rpc/rpc_header.pb.h"
 #include "kudu/rpc/transfer.h"
 #include "kudu/util/faststring.h"
@@ -147,15 +148,15 @@ class InboundCall {
 
   // Associate this call with a particular method that will be invoked
   // by the service.
-  void set_method_info(RpcMethodInfo* info) {
-    method_info_ = info;
+  void set_method_info(scoped_refptr<RpcMethodInfo> info) {
+    method_info_ = std::move(info);
   }
 
   // Return the method associated with this call. This is set just before
   // the call is enqueued onto the service queue, and therefore may be
   // 'nullptr' for much of the lifecycle of a call.
   RpcMethodInfo* method_info() {
-    return method_info_;
+    return method_info_.get();
   }
 
   // When this InboundCall was received (instantiated).
@@ -242,7 +243,7 @@ class InboundCall {
   // After the method has been looked up within the service, this is filled in
   // to point to the information about this method. Acts as a pointer back to
   // per-method info such as tracing.
-  RpcMethodInfo* method_info_;
+  scoped_refptr<RpcMethodInfo> method_info_;
 
   DISALLOW_COPY_AND_ASSIGN(InboundCall);
 };

http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/b6bab209/src/kudu/rpc/protoc-gen-krpc.cc
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/protoc-gen-krpc.cc b/src/kudu/rpc/protoc-gen-krpc.cc
index 0f8c014..fe05e90 100644
--- a/src/kudu/rpc/protoc-gen-krpc.cc
+++ b/src/kudu/rpc/protoc-gen-krpc.cc
@@ -451,7 +451,7 @@ class CodeGenerator : public ::google::protobuf::compiler::CodeGenerator
{
 
         Print(printer, *subs,
               "  {\n"
-              "    unique_ptr<RpcMethodInfo> mi(new RpcMethodInfo());\n"
+              "    scoped_refptr<RpcMethodInfo> mi(new RpcMethodInfo());\n"
               "    mi->req_prototype.reset(new $request$());\n"
               "    mi->resp_prototype.reset(new $response$());\n"
               "    mi->handler_latency_histogram =\n"

http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/b6bab209/src/kudu/rpc/service_if.h
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/service_if.h b/src/kudu/rpc/service_if.h
index 85d012f..582eab1 100644
--- a/src/kudu/rpc/service_if.h
+++ b/src/kudu/rpc/service_if.h
@@ -45,7 +45,7 @@ class RpcContext;
 // method that they implement. The generic server code implemented
 // by GeneratedServiceIf look up the RpcMethodInfo in order to handle
 // each RPC.
-struct RpcMethodInfo {
+struct RpcMethodInfo : public RefCountedThreadSafe<RpcMethodInfo> {
   // Prototype protobufs for requests and responses.
   // These are empty protobufs which are cloned in order to provide an
   // instance for each request.
@@ -104,7 +104,7 @@ class GeneratedServiceIf : public ServiceIf {
   // call. Methods are inserted by the constructor of the generated subclass.
   // After construction, this map is accessed by multiple threads and therefore
   // must not be modified.
-  std::unordered_map<std::string, std::unique_ptr<RpcMethodInfo>> methods_by_name_;
+  std::unordered_map<std::string, scoped_refptr<RpcMethodInfo>> methods_by_name_;
 };
 
 } // namespace rpc


Mime
View raw message