From 9d101d5ea08f4e618acd079179209f46f5307522 Mon Sep 17 00:00:00 2001
From: jeffro256 <jeffro256@tutanota.com>
Date: Wed, 21 Feb 2024 13:08:15 -0600
Subject: [PATCH] serialization: protect blob serialization from undefined
 behavior

There is currently no compiler protection when someone tries to
do (for example) `BLOB_SERIALIZER(std::vector<int>)`. You just
get runtime allocation errors. This has already eaten up dev time
before, so this PR adds a static assertion that the type must be
trivially copyable, as defined by the C++ standard. Types can
override this if applicable if they use `BLOB_SERIALIZER_FORCED`.
---
 src/ringct/rctTypes.h              |  2 +-
 src/serialization/crypto.h         |  2 +-
 src/serialization/serialization.h  | 26 ++++++++++++++++++++++++--
 tests/unit_tests/serialization.cpp |  5 +++++
 4 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/src/ringct/rctTypes.h b/src/ringct/rctTypes.h
index 585d5fb49..24fc5ad3a 100644
--- a/src/ringct/rctTypes.h
+++ b/src/ringct/rctTypes.h
@@ -767,7 +767,7 @@ namespace std
 BLOB_SERIALIZER(rct::key);
 BLOB_SERIALIZER(rct::key64);
 BLOB_SERIALIZER(rct::ctkey);
-BLOB_SERIALIZER(rct::multisig_kLRki);
+BLOB_SERIALIZER_FORCED(rct::multisig_kLRki);
 BLOB_SERIALIZER(rct::boroSig);
 
 VARIANT_TAG(debug_archive, rct::key, "rct::key");
diff --git a/src/serialization/crypto.h b/src/serialization/crypto.h
index 896d00583..2da26abe7 100644
--- a/src/serialization/crypto.h
+++ b/src/serialization/crypto.h
@@ -81,7 +81,7 @@ BLOB_SERIALIZER(crypto::chacha_iv);
 BLOB_SERIALIZER(crypto::hash);
 BLOB_SERIALIZER(crypto::hash8);
 BLOB_SERIALIZER(crypto::public_key);
-BLOB_SERIALIZER(crypto::secret_key);
+BLOB_SERIALIZER_FORCED(crypto::secret_key);
 BLOB_SERIALIZER(crypto::key_derivation);
 BLOB_SERIALIZER(crypto::key_image);
 BLOB_SERIALIZER(crypto::signature);
diff --git a/src/serialization/serialization.h b/src/serialization/serialization.h
index 2911aecb5..15ac3fca5 100644
--- a/src/serialization/serialization.h
+++ b/src/serialization/serialization.h
@@ -50,13 +50,16 @@
 #include <boost/type_traits/integral_constant.hpp>
 #include <boost/mpl/bool.hpp>
 
-/*! \struct is_blob_type 
+/*! \struct is_blob_type / is_blob_forced
  *
- * \brief a descriptor for dispatching serialize
+ * \brief descriptors for dispatching serialize: whether to take byte-wise copy/store to type
  */
 template <class T>
 struct is_blob_type { typedef boost::false_type type; };
 
+template <class T>
+struct is_blob_forced: std::false_type {};
+
 /*! \fn do_serialize(Archive &ar, T &v)
  *
  * \brief main function for dispatching serialization for a given pair of archive and value types
@@ -68,6 +71,8 @@ struct is_blob_type { typedef boost::false_type type; };
 template <class Archive, class T>
 inline std::enable_if_t<is_blob_type<T>::type::value, bool> do_serialize(Archive &ar, T &v)
 {
+  static_assert(std::is_trivially_copyable<T>() || is_blob_forced<T>(),
+    "sanity check: types that can't be trivially copied shouldn't be using the blob serializer");
   ar.serialize_blob(&v, sizeof(v));
   return true;
 }
@@ -94,6 +99,9 @@ inline bool do_serialize(Archive &ar, bool &v)
 /*! \macro BLOB_SERIALIZER
  *
  * \brief makes the type have a blob serializer trait defined
+ *
+ * In case your type is not a good candidate to be blob serialized, a static assertion may be thrown
+ * at compile-time.
  */
 #define BLOB_SERIALIZER(T)						\
   template<>								\
@@ -101,6 +109,20 @@ inline bool do_serialize(Archive &ar, bool &v)
     typedef boost::true_type type;					\
   }
 
+/*! \macro BLOB_SERIALIZER_FORCED
+ *
+ * \brief makes the type have a blob serializer trait defined, even if it isn't trivially copyable
+ *
+ * Caution: do NOT use this macro for your type <T>, unless you are absolutely sure that moving raw
+ * bytes in/out of this type will not cause undefined behavior. Any types with managed memory
+ * (e.g. vector, string, etc) will segfault and/or cause memory errors if you use this macro with
+ * that type.
+ */
+#define BLOB_SERIALIZER_FORCED(T) \
+  BLOB_SERIALIZER(T);             \
+  template<>                      \
+  struct is_blob_forced<T>: std::true_type {};
+
 /*! \macro VARIANT_TAG
  *
  * \brief Adds the tag \tag to the \a Archive of \a Type
diff --git a/tests/unit_tests/serialization.cpp b/tests/unit_tests/serialization.cpp
index ab81acefc..34dda00ab 100644
--- a/tests/unit_tests/serialization.cpp
+++ b/tests/unit_tests/serialization.cpp
@@ -51,6 +51,11 @@
 using namespace std;
 using namespace crypto;
 
+static_assert(!std::is_trivially_copyable<std::vector<unsigned char>>(),
+  "should fail to compile when applying blob serializer");
+static_assert(!std::is_trivially_copyable<std::string>(),
+  "should fail to compile when applying blob serializer");
+
 struct Struct
 {
   int32_t a;