Smart pointer constructors for Secure Session (#378)

* Smart pointer constructors for Secure Session Using legacy C++03 interface of Secure Session may be a bit hard with modern practices introduced by C++11. Let's provide a more idiomatic interface where Secure Session assumes ownership over the provided instance of "secure_session_callback_interface_t" via smart pointers. We do so using the shared_ptr instance. This allows both shared and unique ownership over the provided callback interface. For example, if the users need only get_public_key_for_id() interface then they can share an instance of secure_session_callback_interface_t between all Secure Sessions. However, if they need to use the transport interface (send_data/receive_data) then they are likely to provide a unique instance for each Secure Session. We keep the old non-owning constructor for backwards compatibility. However, we mark is deprecated to incite the users to move on to a more safe and idiomatic interface with smart pointers. We still need to test the old interface so silence the relevant warning in this file.
cossacklabs · Feb 14, 2019 · e8c9ee3 · e8c9ee3
1 parent d8591de
commit e8c9ee3
Show file tree

Hide file tree

Showing 3 changed files with 96 additions and 13 deletions.
diff --git a/src/soter/soter_error.h b/src/soter/soter_error.h
@@ -67,6 +67,16 @@ typedef int32_t soter_status_t;
 	#define UNUSED(x) (void)(x)
 #endif
 
+#ifndef DEPRECATED
+#if __cplusplus >= 201402L
+#define DEPRECATED(msg) [[deprecated(msg)]]
+#elif defined(__GNUC__) || defined(__clang__)
+#define DEPRECATED(msg) __attribute__((deprecated(msg)))
+#else
+#define DEPRECATED(msg)
+#endif
+#endif
+
 /**@}*/
 
 /**

diff --git a/src/wrappers/themis/themispp/secure_session.hpp b/src/wrappers/themis/themispp/secure_session.hpp
@@ -18,6 +18,9 @@
 #define THEMISPP_SECURE_SESSION_HPP_
 
 #include <cstring>
+#if __cplusplus >= 201103L
+#include <memory>
+#endif
 #include <vector>
 #include <themis/themis.h>
 #include "exception.hpp"
@@ -67,21 +70,14 @@ namespace themispp{
       _callback(NULL){
     }
 
+#if __cplusplus >= 201103L
+    DEPRECATED("please use std::shared_ptr<secure_session_callback_interface_t> constructor instead")
+#endif
     secure_session_t(const data_t& id, const data_t& priv_key, secure_session_callback_interface_t* callbacks):
       _session(NULL),
       _callback(NULL),
       _res(0){
-      _callback=new secure_session_user_callbacks_t();
-      _callback->get_public_key_for_id=themispp::get_public_key_for_id_callback;
-      _callback->send_data=themispp::send_callback;
-      _callback->receive_data=themispp::receive_callback;
-      _callback->state_changed=NULL;
-      _callback->user_data=callbacks;
-      _session=secure_session_create(&id[0], id.size(), &priv_key[0], priv_key.size(), _callback);
-      if(!_session){
-        delete _callback;
-        throw themispp::exception_t("Secure Session failed creating");
-      }
+      initialize_session(id, priv_key, callbacks);
     }
 
     virtual ~secure_session_t(){
@@ -94,13 +90,22 @@ namespace themispp{
     }
 
 #if __cplusplus >= 201103L
+    secure_session_t(const data_t& id, const data_t& priv_key, std::shared_ptr<secure_session_callback_interface_t> &&callbacks):
+      _session(nullptr),
+      _callback(nullptr),
+      _res(0),
+      _interface(std::move(callbacks)){
+      initialize_session(id, priv_key, _interface.get());
+    }
+
     secure_session_t(const secure_session_t&) = delete;
     secure_session_t& operator=(const secure_session_t&) = delete;
 
     secure_session_t(secure_session_t&& other){
       _session=other._session;
       _callback=other._callback;
-      _res=other._res;
+      _res=std::move(other._res);
+      _interface=std::move(other._interface);
       other._session=nullptr;
       other._callback=nullptr;
     }
@@ -113,7 +118,8 @@ namespace themispp{
         delete _callback;
         _session=other._session;
         _callback=other._callback;
-        _res=other._res;
+        _res=std::move(other._res);
+        _interface=std::move(other._interface);
         other._session=nullptr;
         other._callback=nullptr;
       }
@@ -204,10 +210,29 @@ namespace themispp{
       if(send_size<=0)
 	throw themispp::exception_t("Secure Session failed sending");
     }
+
+  private:
+    void initialize_session(const data_t& id, const data_t& priv_key, secure_session_callback_interface_t* callbacks){
+      _callback=new secure_session_user_callbacks_t();
+      _callback->get_public_key_for_id=themispp::get_public_key_for_id_callback;
+      _callback->send_data=themispp::send_callback;
+      _callback->receive_data=themispp::receive_callback;
+      _callback->state_changed=NULL;
+      _callback->user_data=callbacks;
+      _session=secure_session_create(&id[0], id.size(), &priv_key[0], priv_key.size(), _callback);
+      if(!_session){
+        delete _callback;
+        throw themispp::exception_t("Secure Session failed creating");
+      }
+    }
+
   private:
     ::secure_session_t* _session;
     ::secure_session_user_callbacks_t *_callback;
     std::vector<uint8_t> _res;
+#if __cplusplus >= 201103L
+    std::shared_ptr<secure_session_callback_interface_t> _interface;
+#endif
   };
 }// ns themis
 

diff --git a/tests/themispp/secure_session_test.hpp b/tests/themispp/secure_session_test.hpp
@@ -25,6 +25,12 @@
 #include <unordered_map>
 #endif
 
+// Allow usage of non-owning Secure Session constructor for testing
+#if defined(__GNUC__) || defined(__clang__)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
+#endif
+
 namespace themispp{
   namespace secure_session_test{
 
@@ -113,19 +119,61 @@ namespace themispp{
 
       sput_fail_if(client.is_established(), "using moved session again", __LINE__);
     }
+
+    void secure_session_ownership(){
+      std::string client_id("client");
+
+      auto client_callbacks_shared = std::make_shared<callback>();
+      auto client_callbacks_unique = std::unique_ptr<callback>(new callback());
+
+      themispp::secure_session_t client_shared(std::vector<uint8_t>(client_id.c_str(), client_id.c_str()+client_id.length()), std::vector<uint8_t>(client_priv, client_priv+sizeof(client_priv)), std::move(client_callbacks_shared));
+      themispp::secure_session_t client_unique(std::vector<uint8_t>(client_id.c_str(), client_id.c_str()+client_id.length()), std::vector<uint8_t>(client_priv, client_priv+sizeof(client_priv)), std::move(client_callbacks_unique));
+
+      themispp::secure_session_t client_shared_moved = std::move(client_shared);
+      themispp::secure_session_t client_unique_moved = std::move(client_unique);
+
+      sput_fail_if(client_shared_moved.is_established(), "using shared session", __LINE__);
+      sput_fail_if(client_unique_moved.is_established(), "using unique session", __LINE__);
+
+      bool shared_throws=false;
+      bool unique_throws=false;
+      try{
+        client_shared.is_established();
+      }
+      catch(const themispp::exception_t&){
+        shared_throws=true;
+      }
+      try{
+        client_unique.is_established();
+      }
+      catch(const themispp::exception_t&){
+        unique_throws=true;
+      }
+      sput_fail_unless(shared_throws, "using shared session after move", __LINE__);
+      sput_fail_unless(unique_throws, "using unique session after move", __LINE__);
+    }
 #else
     void secure_session_moved(){
       sput_fail_if(false, "move semantics (disabled for C++03)", __LINE__);
     }
+
+    void secure_session_ownership(){
+      sput_fail_if(false, "ownership transfer (disabled for C++03)", __LINE__);
+    }
 #endif
 
     int run_secure_session_test(){
       sput_enter_suite("ThemisPP secure session test");
       sput_run_test(secure_session_test, "secure_session_test", __FILE__);
       sput_run_test(secure_session_uninitialized, "secure_session_uninitialized", __FILE__);
       sput_run_test(secure_session_moved, "secure_session_moved", __FILE__);
+      sput_run_test(secure_session_ownership, "secure_session_ownership", __FILE__);
       return 0;
     }
   }
 }
 #endif
+
+#if defined(__GNUC__) || defined(__clang__)
+#pragma GCC diagnostic pop
+#endif