From 6082d31843baa9991ae62524fb1f9bc6acd50e47 Mon Sep 17 00:00:00 2001
From: Matej Ferencevic <matej.ferencevic@memgraph.io>
Date: Wed, 15 May 2019 15:31:14 +0200
Subject: [PATCH] Fix communication context SSL leaks

Summary:
A `valgrind` run of a modified client that connects multiple times to the
database now shows no memory leaks, previously the context created with
`SSL_CTX_new` was leaked every time.

Reviewers: teon.banek, mtomic

Reviewed By: teon.banek, mtomic

Subscribers: pullbot

Differential Revision: https://phabricator.memgraph.io/D2052
---
 src/communication/context.cpp | 62 +++++++++++++++++++++++++++++++++++
 src/communication/context.hpp | 36 ++++++++++++++++++++
 2 files changed, 98 insertions(+)

diff --git a/src/communication/context.cpp b/src/communication/context.cpp
index 03db468ee..77a407639 100644
--- a/src/communication/context.cpp
+++ b/src/communication/context.cpp
@@ -32,6 +32,37 @@ ClientContext::ClientContext(const std::string &key_file,
   }
 }
 
+ClientContext::ClientContext(ClientContext &&other) noexcept
+    : use_ssl_(other.use_ssl_), ctx_(other.ctx_) {
+  other.use_ssl_ = false;
+  other.ctx_ = nullptr;
+}
+
+ClientContext &ClientContext::operator=(ClientContext &&other) noexcept {
+  if (this == &other) return *this;
+
+  // destroy my objects
+  if (use_ssl_) {
+    SSL_CTX_free(ctx_);
+  }
+
+  // move other objects to self
+  use_ssl_ = other.use_ssl_;
+  ctx_ = other.ctx_;
+
+  // reset other objects
+  other.use_ssl_ = false;
+  other.ctx_ = nullptr;
+
+  return *this;
+}
+
+ClientContext::~ClientContext() {
+  if (use_ssl_) {
+    SSL_CTX_free(ctx_);
+  }
+}
+
 SSL_CTX *ClientContext::context() { return ctx_; }
 
 bool ClientContext::use_ssl() { return use_ssl_; }
@@ -83,6 +114,37 @@ ServerContext::ServerContext(const std::string &key_file,
   }
 }
 
+ServerContext::ServerContext(ServerContext &&other) noexcept
+    : use_ssl_(other.use_ssl_), ctx_(other.ctx_) {
+  other.use_ssl_ = false;
+  other.ctx_ = nullptr;
+}
+
+ServerContext &ServerContext::operator=(ServerContext &&other) noexcept {
+  if (this == &other) return *this;
+
+  // destroy my objects
+  if (use_ssl_) {
+    SSL_CTX_free(ctx_);
+  }
+
+  // move other objects to self
+  use_ssl_ = other.use_ssl_;
+  ctx_ = other.ctx_;
+
+  // reset other objects
+  other.use_ssl_ = false;
+  other.ctx_ = nullptr;
+
+  return *this;
+}
+
+ServerContext::~ServerContext() {
+  if (use_ssl_) {
+    SSL_CTX_free(ctx_);
+  }
+}
+
 SSL_CTX *ServerContext::context() { return ctx_; }
 
 bool ServerContext::use_ssl() { return use_ssl_; }
diff --git a/src/communication/context.hpp b/src/communication/context.hpp
index 3af50e69e..a91712849 100644
--- a/src/communication/context.hpp
+++ b/src/communication/context.hpp
@@ -6,6 +6,12 @@
 
 namespace communication {
 
+/**
+ * This class represents a context that should be used with network clients. One
+ * context can be reused between multiple clients (note: this mainly depends on
+ * the underlying OpenSSL implementation [see `SSL_new` in
+ * `openssl/ssl/ssl_lib.c`]).
+ */
 class ClientContext final {
  public:
   /**
@@ -23,6 +29,18 @@ class ClientContext final {
    */
   ClientContext(const std::string &key_file, const std::string &cert_file);
 
+  // This object can't be copied because the underlying SSL implementation is
+  // messy and ownership can't be handled correctly.
+  ClientContext(const ClientContext &) = delete;
+  ClientContext &operator=(const ClientContext &) = delete;
+
+  // Move constructor/assignment that handle ownership change correctly.
+  ClientContext(ClientContext &&other) noexcept;
+  ClientContext &operator=(ClientContext &&other) noexcept;
+
+  // Destructor that handles ownership of the SSL object.
+  ~ClientContext();
+
   SSL_CTX *context();
 
   bool use_ssl();
@@ -32,6 +50,12 @@ class ClientContext final {
   SSL_CTX *ctx_;
 };
 
+/**
+ * This class represents a context that should be used with network servers. One
+ * context can be reused between multiple servers (note: this mainly depends on
+ * the underlying OpenSSL implementation [see `SSL_new` in
+ * `openssl/ssl/ssl_lib.c`]).
+ */
 class ServerContext final {
  public:
   /**
@@ -51,6 +75,18 @@ class ServerContext final {
   ServerContext(const std::string &key_file, const std::string &cert_file,
                 const std::string &ca_file = "", bool verify_peer = false);
 
+  // This object can't be copied because the underlying SSL implementation is
+  // messy and ownership can't be handled correctly.
+  ServerContext(const ServerContext &) = delete;
+  ServerContext &operator=(const ServerContext &) = delete;
+
+  // Move constructor/assignment that handle ownership change correctly.
+  ServerContext(ServerContext &&other) noexcept;
+  ServerContext &operator=(ServerContext &&other) noexcept;
+
+  // Destructor that handles ownership of the SSL object.
+  ~ServerContext();
+
   SSL_CTX *context();
 
   bool use_ssl();