treewide: get rid of incorrect reinterpret casts

In some places we use the `*reinterpret_cast<const net::packed<T>*>(&x)`
pattern to reinterpret memory. This is a violation of C++'s aliasing rules,
which invokes undefined behaviour.

The blessed way to correctly reinterpret memory is to copy it into a new
object. Let's do that.

Note: the reinterpret_cast way has no performance advantage. Compilers
recognize the memory copy pattern and optimize it away.

sstables/sstables.cc

@@ -79,6 +79,7 @@
 #include "tracing/traced_file.hh"
 #include "kl/reader.hh"
 #include "mx/reader.hh"
+#include "utils/bit_cast.hh"
 
 thread_local disk_error_signal_type sstable_read_error;
 thread_local disk_error_signal_type sstable_write_error;
@@ -215,54 +216,35 @@ static void check_buf_size(temporary_buffer<char>& buf, size_t expected) {
     }
 }
 
-// Base parser, parses an integer type
 template <typename T>
-typename std::enable_if_t<std::is_integral<T>::value, void>
-read_integer(temporary_buffer<char>& buf, T& i) {
-    auto *nr = reinterpret_cast<const net::packed<T> *>(buf.get());
-    i = net::ntoh(*nr);
-}
-
-template <typename T>
-typename std::enable_if_t<std::is_integral<T>::value, future<>>
-parse(const schema&, sstable_version_types v, random_access_reader& in, T& i) {
+requires std::is_integral_v<T>
+future<> parse(const schema&, sstable_version_types v, random_access_reader& in, T& i) {
     return in.read_exactly(sizeof(T)).then([&i] (auto buf) {
         check_buf_size(buf, sizeof(T));
-
-        read_integer(buf, i);
+        i = net::ntoh(read_unaligned<T>(buf.get()));
         return make_ready_future<>();
     });
 }
 
-
 template <typename T>
-typename std::enable_if_t<std::is_enum<T>::value, future<>>
-parse(const schema& s, sstable_version_types v, random_access_reader& in, T& i) {
-    return parse(s, v, in, reinterpret_cast<typename std::underlying_type<T>::type&>(i));
+requires std::is_enum_v<T>
+future<> parse(const schema& s, sstable_version_types v, random_access_reader& in, T& i) {
+    return in.read_exactly(sizeof(T)).then([&i] (auto buf) {
+        check_buf_size(buf, sizeof(T));
+        i = static_cast<T>(net::ntoh(read_unaligned<std::underlying_type_t<T>>(buf.get())));
+        return make_ready_future<>();
+    });
 }
 
 future<> parse(const schema& s, sstable_version_types v, random_access_reader& in, bool& i) {
     return parse(s, v, in, reinterpret_cast<uint8_t&>(i));
 }
 
-template <typename To, typename From>
-static inline To convert(From f) {
-    static_assert(sizeof(To) == sizeof(From), "Sizes must match");
-    union {
-        To to;
-        From from;
-    } conv;
-
-    conv.from = f;
-    return conv.to;
-}
-
 future<> parse(const schema&, sstable_version_types, random_access_reader& in, double& d) {
     return in.read_exactly(sizeof(double)).then([&d] (auto buf) {
         check_buf_size(buf, sizeof(double));
-
-        auto *nr = reinterpret_cast<const net::packed<unsigned long> *>(buf.get());
-        d = convert<double>(net::ntoh(*nr));
+        unsigned long nr = read_unaligned<unsigned long>(buf.get());
+        d = bit_cast<double>(net::ntoh(nr));
         return make_ready_future<>();
     });
 }
@@ -372,9 +354,8 @@ parse(const schema&, sstable_version_types, random_access_reader& in, Size& len,
         return in.read_exactly(now * sizeof(Members)).then([&arr, len, now, done] (auto buf) {
             check_buf_size(buf, now * sizeof(Members));
 
-            auto *nr = reinterpret_cast<const net::packed<Members> *>(buf.get());
             for (size_t i = 0; i < now; ++i) {
-                arr.push_back(net::ntoh(nr[i]));
+                arr.push_back(net::ntoh(read_unaligned<Members>(buf.get() + i * sizeof(Members))));
             }
             *done += now;
             return make_ready_future<stop_iteration>(*done == len ? stop_iteration::yes : stop_iteration::no);
@@ -693,10 +674,9 @@ future<> parse(const schema& s, sstable_version_types v, random_access_reader& i
             check_buf_size(buf, length * type_size);
 
             return do_with(size_t(0), std::move(buf), [&eh, length] (size_t& j, auto& buf) mutable {
-                auto *nr = reinterpret_cast<const net::packed<uint64_t> *>(buf.get());
-                return do_until([&eh, length] { return eh.buckets.size() == length; }, [nr, &eh, &j] () mutable {
-                    auto offset = net::ntoh(nr[j++]);
-                    auto bucket = net::ntoh(nr[j++]);
+                return do_until([&eh, length] { return eh.buckets.size() == length; }, [&eh, &j, &buf] () mutable {
+                    auto offset = net::ntoh(read_unaligned<uint64_t>(buf.get() + (j++) * sizeof(uint64_t)));
+                    auto bucket = net::ntoh(read_unaligned<uint64_t>(buf.get() + (j++) * sizeof(uint64_t)));
                     if (eh.buckets.size() > 0) {
                         eh.bucket_offsets.push_back(offset);
                     }
@@ -714,14 +694,14 @@ void write(sstable_version_types v, file_writer& out, const utils::estimated_his
 
     write(v, out, len);
     struct element {
-        uint64_t offsets;
-        uint64_t buckets;
+        int64_t offsets;
+        int64_t buckets;
     };
     std::vector<element> elements;
     elements.reserve(eh.buckets.size());
 
-    auto *offsets_nr = reinterpret_cast<const net::packed<uint64_t> *>(eh.bucket_offsets.data());
-    auto *buckets_nr = reinterpret_cast<const net::packed<uint64_t> *>(eh.buckets.data());
+    const int64_t* offsets_nr = eh.bucket_offsets.data();
+    const int64_t* buckets_nr = eh.buckets.data();
     for (size_t i = 0; i < eh.buckets.size(); i++) {
         auto offsets = net::hton(offsets_nr[i == 0 ? 0 : i - 1]);
         auto buckets = net::hton(buckets_nr[i]);
@@ -813,9 +793,8 @@ future<> parse(const schema& s, sstable_version_types v, random_access_reader& i
             return do_until(eoarr, [&in, &c, &len, &offsets] () {
                 auto now = std::min(len - c.offsets.size(), 100000 / sizeof(uint64_t));
                 return in.read_exactly(now * sizeof(uint64_t)).then([&offsets, now] (auto buf) {
-                    uint64_t value;
                     for (size_t i = 0; i < now; ++i) {
-                        std::copy_n(buf.get() + i * sizeof(uint64_t), sizeof(uint64_t), reinterpret_cast<char*>(&value));
+                        uint64_t value = read_unaligned<uint64_t>(buf.get() + i * sizeof(uint64_t));
                         offsets.push_back(net::ntoh(value));
                     }
                 });

sstables/writer.hh

@@ -31,6 +31,7 @@
 #include "version.hh"
 #include "counters.hh"
 #include "service/storage_service.hh"
+#include "utils/bit_cast.hh"
 
 namespace sstables {
 
@@ -262,10 +263,8 @@ template <typename T, typename W>
 requires Writer<W>
 inline typename std::enable_if_t<std::is_integral<T>::value, void>
 write(sstable_version_types v, W& out, T i) {
-    auto *nr = reinterpret_cast<const net::packed<T> *>(&i);
-    i = net::hton(*nr);
-    auto p = reinterpret_cast<const char*>(&i);
-    out.write(p, sizeof(T));
+    i = net::hton(i);
+    out.write(reinterpret_cast<const char*>(&i), sizeof(T));
 }
 
 template <typename T, typename W>
@@ -283,10 +282,8 @@ inline void write(sstable_version_types v, W& out, bool i) {
 }
 
 inline void write(sstable_version_types v, file_writer& out, double d) {
-    auto *nr = reinterpret_cast<const net::packed<unsigned long> *>(&d);
-    auto tmp = net::hton(*nr);
-    auto p = reinterpret_cast<const char*>(&tmp);
-    out.write(p, sizeof(unsigned long));
+    unsigned long tmp = net::hton(bit_cast<unsigned long>(d));
+    out.write(reinterpret_cast<const char*>(&tmp), sizeof(unsigned long));
 }
 
 

transport/server.cc

@@ -67,6 +67,7 @@
 #include "types/user.hh"
 
 #include "transport/cql_protocol_extension.hh"
+#include "utils/bit_cast.hh"
 
 namespace cql_transport {
 
@@ -379,8 +380,8 @@ cql_server::connection::parse_frame(temporary_buffer<char> buf) const {
     switch (_version) {
     case 1:
     case 2: {
-        auto raw = reinterpret_cast<const cql_binary_frame_v1*>(buf.get());
-        auto cooked = net::ntoh(*raw);
+        cql_binary_frame_v1 raw = read_unaligned<cql_binary_frame_v1>(buf.get());
+        auto cooked = net::ntoh(raw);
         v3.version = cooked.version;
         v3.flags = cooked.flags;
         v3.opcode = cooked.opcode;
@@ -390,7 +391,8 @@ cql_server::connection::parse_frame(temporary_buffer<char> buf) const {
     }
     case 3:
     case 4: {
-        v3 = net::ntoh(*reinterpret_cast<const cql_binary_frame_v3*>(buf.get()));
+        cql_binary_frame_v3 raw = read_unaligned<cql_binary_frame_v3>(buf.get());
+        v3 = net::ntoh(raw);
         break;
     }
     default:

types.cc

@@ -175,7 +175,7 @@ integer_type_impl<T>::integer_type_impl(
 
 template <typename T> static bytes decompose_value(T v) {
     bytes b(bytes::initialized_later(), sizeof(v));
-    *reinterpret_cast<T*>(b.begin()) = (T)net::hton(v);
+    write_unaligned<T>(b.begin(), net::hton(v));
     return b;
 }
 

types.hh

@@ -50,6 +50,7 @@
 #include "utils/fragmented_temporary_buffer.hh"
 #include "utils/exceptions.hh"
 #include "utils/managed_bytes.hh"
+#include "utils/bit_cast.hh"
 
 class tuple_type_impl;
 class big_decimal;
@@ -1165,7 +1166,7 @@ T read_simple(bytes_view& v) {
     }
     auto p = v.begin();
     v.remove_prefix(sizeof(T));
-    return net::ntoh(*reinterpret_cast<const net::packed<T>*>(p));
+    return net::ntoh(read_unaligned<T>(p));
 }
 
 template<typename T>
@@ -1174,7 +1175,7 @@ T read_simple_exactly(bytes_view v) {
         throw_with_backtrace<marshal_exception>(format("read_simple_exactly - size mismatch (expected {:d}, got {:d})", sizeof(T), v.size()));
     }
     auto p = v.begin();
-    return net::ntoh(*reinterpret_cast<const net::packed<T>*>(p));
+    return net::ntoh(read_unaligned<T>(p));
 }
 
 inline

utils/bit_cast.hh

@@ -0,0 +1,52 @@
+/*
+ * Copyright (C) 2021 ScyllaDB
+ */
+
+/*
+ * This file is part of Scylla.
+ *
+ * Scylla is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * Scylla is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Scylla.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#pragma once
+
+#include <cstring>
+#include <cstddef>
+#include <type_traits>
+
+template <class T> concept Trivial = std::is_trivial_v<T>;
+template <class T> concept TriviallyCopyable = std::is_trivially_copyable_v<T>;
+
+// C++20 has std::bit_cast, which does the same thing,
+// (but better, because it supports constexpr through compiler magic)
+// but it hasn't made its way to our libstdc++ yet.
+template <Trivial To, TriviallyCopyable From>
+requires (sizeof(To) == sizeof(From))
+To bit_cast(const From& src) noexcept {
+    To dst;
+    std::memcpy(&dst, &src, sizeof(To));
+    return dst;
+}
+
+template <Trivial To>
+To read_unaligned(const void* src) {
+    To dst;
+    std::memcpy(&dst, src, sizeof(To));
+    return dst;
+}
+
+template <TriviallyCopyable From>
+void write_unaligned(void* dst, const From& src) {
+    std::memcpy(dst, &src, sizeof(From));
+}

utils/crc.hh

@@ -105,12 +105,12 @@ public:
             --size;
         }
         if ((reinterpret_cast<uintptr_t>(in) & 3) && size >= 2) {
-            process_le(*reinterpret_cast<const uint16_t*>(in));
+            process_le(seastar::read_le<uint16_t>(reinterpret_cast<const char*>(in)));
             in += 2;
             size -= 2;
         }
         if ((reinterpret_cast<uintptr_t>(in) & 7) && size >= 4) {
-            process_le(*reinterpret_cast<const uint32_t*>(in));
+            process_le(seastar::read_le<uint32_t>(reinterpret_cast<const char*>(in)));
             in += 4;
             size -= 4;
         }
@@ -146,17 +146,17 @@ public:
         }
 
         while (size >= 8) {
-            process_le(*reinterpret_cast<const uint64_t*>(in));
+            process_le(seastar::read_le<uint64_t>(reinterpret_cast<const char*>(in)));
             in += 8;
             size -= 8;
         }
         if (size >= 4) {
-            process_le(*reinterpret_cast<const uint32_t*>(in));
+            process_le(seastar::read_le<uint32_t>(reinterpret_cast<const char*>(in)));
             in += 4;
             size -= 4;
         }
         if (size >= 2) {
-            process_le(*reinterpret_cast<const uint16_t*>(in));
+            process_le(seastar::read_le<uint16_t>(reinterpret_cast<const char*>(in)));
             in += 2;
             size -= 2;
         }

utils/fragment_range.hh

@@ -29,6 +29,7 @@
 
 #include "marshal_exception.hh"
 #include "bytes.hh"
+#include "utils/bit_cast.hh"
 
 enum class mutable_view { no, yes, };
 
@@ -346,7 +347,7 @@ T read_simple_native(View& v) {
     if (v.current_fragment().size() >= sizeof(T)) [[likely]] {
         auto p = v.current_fragment().data();
         v.remove_prefix(sizeof(T));
-        return *reinterpret_cast<const net::packed<T>*>(p);
+        return read_unaligned<T>(p);
     } else if (v.size_bytes() >= sizeof(T)) {
         T buf;
         read_fragmented(v, sizeof(T), reinterpret_cast<bytes::value_type*>(&buf));
@@ -361,7 +362,7 @@ T read_simple(View& v) {
     if (v.current_fragment().size() >= sizeof(T)) [[likely]] {
         auto p = v.current_fragment().data();
         v.remove_prefix(sizeof(T));
-        return net::ntoh(*reinterpret_cast<const net::packed<T>*>(p));
+        return net::ntoh(read_unaligned<T>(p));
     } else if (v.size_bytes() >= sizeof(T)) {
         T buf;
         read_fragmented(v, sizeof(T), reinterpret_cast<bytes::value_type*>(&buf));
@@ -375,7 +376,7 @@ template<typename T, FragmentedView View>
 T read_simple_exactly(View v) {
     if (v.current_fragment().size() == sizeof(T)) [[likely]] {
         auto p = v.current_fragment().data();
-        return net::ntoh(*reinterpret_cast<const net::packed<T>*>(p));
+        return net::ntoh(read_unaligned<T>(p));
     } else if (v.size_bytes() == sizeof(T)) {
         T buf;
         read_fragmented(v, sizeof(T), reinterpret_cast<bytes::value_type*>(&buf));

utils/linearizing_input_stream.hh

@@ -27,6 +27,7 @@
 #include <seastar/util/backtrace.hh>
 
 #include "utils/fragment_range.hh"
+#include "utils/bit_cast.hh"
 
 namespace utils {
 
@@ -112,7 +113,7 @@ public:
     requires std::is_trivial_v<Type>
     Type read_trivial() {
         auto [bv, linearized] = do_read(sizeof(Type));
-        auto ret = net::ntoh(*reinterpret_cast<const net::packed<Type>*>(bv.begin()));
+        auto ret = net::ntoh(::read_unaligned<net::packed<Type>>(bv.begin()));
         if (linearized) {
             _linearized_values.pop_back();
         }