Add limitations to bit_cast

Using bit_cast on pointers does not accomplish anything useful, and
obscures what might be incorrect behavior. Do not allow base::bit_cast
to be misused in that way.

Bug: 1506769
Change-Id: Ib9f3acf954c5537c7520f72827854de81afda382
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5094596
Reviewed-by: Hongchan Choi <hongchan@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Dominic Battre <battre@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1236404}

PRESUBMIT.py

@@ -1189,6 +1189,15 @@ _BANNED_CPP_FUNCTIONS : Sequence[BanRule] = (
       True,
       [_THIRD_PARTY_EXCEPT_BLINK],  # Don't warn in third_party folders.
     ),
+    BanRule(
+      r'/\bstd::bit_cast\b',
+      (
+        'std::bit_cast is banned; use base::bit_cast instead for values and '
+        'standard C++ casting when pointers are involved.',
+      ),
+      True,
+      [_THIRD_PARTY_EXCEPT_BLINK],  # Don't warn in third_party folders.
+    ),
     BanRule(
       r'/\bstd::(c8rtomb|mbrtoc8)\b',
       (

base/bit_cast.h

@@ -7,40 +7,37 @@
 
 #include <type_traits>
 
-#include "base/compiler_specific.h"
-
-#if !HAS_BUILTIN(__builtin_bit_cast)
-#include <string.h>  // memcpy
-#endif
-
 namespace base {
 
-// This is C++20's std::bit_cast<>(). It morally does what
-// `*reinterpret_cast<Dest*>(&source)` does, but the cast/deref pair is
-// undefined behavior, while bit_cast<>() isn't.
-template <class Dest, class Source>
-#if HAS_BUILTIN(__builtin_bit_cast)
-constexpr
-#else
-inline
-#endif
-    Dest
-    bit_cast(const Source& source) {
-#if HAS_BUILTIN(__builtin_bit_cast)
-  // TODO(thakis): Keep only this codepath once nacl is gone or updated.
-  return __builtin_bit_cast(Dest, source);
-#else
-  static_assert(sizeof(Dest) == sizeof(Source),
-                "bit_cast requires source and destination to be the same size");
-  static_assert(std::is_trivially_copyable_v<Dest>,
-                "bit_cast requires the destination type to be copyable");
-  static_assert(std::is_trivially_copyable_v<Source>,
-                "bit_cast requires the source type to be copyable");
+// This is an equivalent to C++20's std::bit_cast<>(), but with additional
+// warnings. It morally does what `*reinterpret_cast<Dest*>(&source)` does, but
+// the cast/deref pair is undefined behavior, while bit_cast<>() isn't.
+//
+// This is not a magic "get out of UB free" card. This must only be used on
+// values, not on references or pointers. For pointers, use
+// reinterpret_cast<>(), and then look at https://eel.is/c++draft/basic.lval#11
+// as that's probably UB also.
 
-  Dest dest;
-  memcpy(&dest, &source, sizeof(dest));
-  return dest;
-#endif
+template <class Dest, class Source>
+constexpr Dest bit_cast(const Source& source) {
+  static_assert(!std::is_pointer_v<Source>,
+                "bit_cast must not be used on pointer types");
+  static_assert(!std::is_pointer_v<Dest>,
+                "bit_cast must not be used on pointer types");
+  static_assert(!std::is_reference_v<Source>,
+                "bit_cast must not be used on reference types");
+  static_assert(!std::is_reference_v<Dest>,
+                "bit_cast must not be used on reference types");
+  static_assert(
+      sizeof(Dest) == sizeof(Source),
+      "bit_cast requires source and destination types to be the same size");
+  static_assert(std::is_trivially_copyable_v<Source>,
+                "bit_cast requires the source type to be trivially copyable");
+  static_assert(
+      std::is_trivially_copyable_v<Dest>,
+      "bit_cast requires the destination type to be trivially copyable");
+
+  return __builtin_bit_cast(Dest, source);
 }
 
 }  // namespace base

net/filter/brotli_source_stream.cc

@@ -6,7 +6,6 @@
 
 #include "net/filter/brotli_source_stream.h"
 
-#include "base/bit_cast.h"
 #include "base/check_op.h"
 #include "base/functional/bind.h"
 #include "base/memory/raw_ptr.h"
@@ -108,9 +107,9 @@ class BrotliSourceStream : public FilterSourceStream {
     if (decoding_status_ != DecodingStatus::DECODING_IN_PROGRESS)
       return base::unexpected(ERR_CONTENT_DECODING_FAILED);
 
-    const uint8_t* next_in = base::bit_cast<uint8_t*>(input_buffer->data());
+    const uint8_t* next_in = reinterpret_cast<uint8_t*>(input_buffer->data());
     size_t available_in = input_buffer_size;
-    uint8_t* next_out = base::bit_cast<uint8_t*>(output_buffer->data());
+    uint8_t* next_out = reinterpret_cast<uint8_t*>(output_buffer->data());
     size_t available_out = output_buffer_size;
 
     BrotliDecoderResult result =

net/filter/filter_source_stream_test_util.cc

@@ -6,7 +6,6 @@
 
 #include <cstring>
 
-#include "base/bit_cast.h"
 #include "base/check_op.h"
 #include "third_party/zlib/zlib.h"
 
@@ -52,9 +51,9 @@ void CompressGzip(const char* source,
     dest_left -= sizeof(gzip_header);
   }
 
-  zlib_stream.next_in = base::bit_cast<Bytef*>(source);
+  zlib_stream.next_in = reinterpret_cast<Bytef*>(const_cast<char*>(source));
   zlib_stream.avail_in = source_len;
-  zlib_stream.next_out = base::bit_cast<Bytef*>(dest);
+  zlib_stream.next_out = reinterpret_cast<Bytef*>(dest);
   zlib_stream.avail_out = dest_left;
 
   code = deflate(&zlib_stream, Z_FINISH);

net/filter/gzip_source_stream.cc

@@ -8,7 +8,6 @@
 #include <memory>
 #include <utility>
 
-#include "base/bit_cast.h"
 #include "base/check_op.h"
 #include "base/functional/bind.h"
 #include "base/memory/ptr_util.h"
@@ -133,10 +132,10 @@ base::expected<size_t, Error> GzipSourceStream::FilterData(
       case STATE_SNIFFING_DEFLATE_HEADER: {
         DCHECK_EQ(TYPE_DEFLATE, type());
 
-        zlib_stream_.get()->next_in = base::bit_cast<Bytef*>(input_data);
+        zlib_stream_.get()->next_in = reinterpret_cast<Bytef*>(input_data);
         zlib_stream_.get()->avail_in = input_data_size;
         zlib_stream_.get()->next_out =
-            base::bit_cast<Bytef*>(output_buffer->data());
+            reinterpret_cast<Bytef*>(output_buffer->data());
         zlib_stream_.get()->avail_out = output_buffer_size;
 
         int ret = inflate(zlib_stream_.get(), Z_NO_FLUSH);
@@ -211,10 +210,10 @@ base::expected<size_t, Error> GzipSourceStream::FilterData(
         DCHECK(!state_compressed_entered);
 
         state_compressed_entered = true;
-        zlib_stream_.get()->next_in = base::bit_cast<Bytef*>(input_data);
+        zlib_stream_.get()->next_in = reinterpret_cast<Bytef*>(input_data);
         zlib_stream_.get()->avail_in = input_data_size;
         zlib_stream_.get()->next_out =
-            base::bit_cast<Bytef*>(output_buffer->data());
+            reinterpret_cast<Bytef*>(output_buffer->data());
         zlib_stream_.get()->avail_out = output_buffer_size;
 
         int ret = inflate(zlib_stream_.get(), Z_NO_FLUSH);
@@ -256,9 +255,9 @@ bool GzipSourceStream::InsertZlibHeader() {
   char dummy_output[4];
 
   inflateReset(zlib_stream_.get());
-  zlib_stream_.get()->next_in = base::bit_cast<Bytef*>(&dummy_header[0]);
+  zlib_stream_.get()->next_in = reinterpret_cast<Bytef*>(&dummy_header[0]);
   zlib_stream_.get()->avail_in = sizeof(dummy_header);
-  zlib_stream_.get()->next_out = base::bit_cast<Bytef*>(&dummy_output[0]);
+  zlib_stream_.get()->next_out = reinterpret_cast<Bytef*>(&dummy_output[0]);
   zlib_stream_.get()->avail_out = sizeof(dummy_output);
 
   int ret = inflate(zlib_stream_.get(), Z_NO_FLUSH);

sandbox/win/src/service_resolver_32.cc

@@ -12,8 +12,6 @@
 
 #include <memory>
 
-#include "base/bit_cast.h"
-
 namespace {
 #pragma pack(push, 1)
 
@@ -233,7 +231,7 @@ NTSTATUS ServiceResolverThunk::PerformPatch(void* local_thunk,
   intercepted_code.service_id = full_local_thunk->original.service_id;
   intercepted_code.mov_edx = kMovEdx;
   intercepted_code.mov_edx_param =
-      base::bit_cast<ULONG>(&full_remote_thunk->internal_thunk);
+      reinterpret_cast<ULONG>(&full_remote_thunk->internal_thunk);
   intercepted_code.call_edx = kJmpEdx;
   bytes_to_write = kMinServiceSize;
 
@@ -291,8 +289,8 @@ bool ServiceResolverThunk::SaveOriginalFunction(void* local_thunk,
     ULONG relative = function_code.service_id;
 
     // First, fix our copy of their patch.
-    relative +=
-        base::bit_cast<ULONG>(target_) - base::bit_cast<ULONG>(remote_thunk);
+    relative += reinterpret_cast<ULONG>(target_) -
+                reinterpret_cast<ULONG>(remote_thunk);
 
     function_code.service_id = relative;
 
@@ -302,8 +300,8 @@ bool ServiceResolverThunk::SaveOriginalFunction(void* local_thunk,
 
     const ULONG kJmp32Size = 5;
 
-    relative_jump_ = base::bit_cast<ULONG>(&full_thunk->internal_thunk) -
-                     base::bit_cast<ULONG>(target_) - kJmp32Size;
+    relative_jump_ = reinterpret_cast<ULONG>(&full_thunk->internal_thunk) -
+                     reinterpret_cast<ULONG>(target_) - kJmp32Size;
   }
 
   // Save the verified code
@@ -320,8 +318,8 @@ bool ServiceResolverThunk::VerifyJumpTargetForTesting(
     return false;
   }
 
-  ULONG source_addr = base::bit_cast<ULONG>(target_);
-  ULONG target_addr = base::bit_cast<ULONG>(thunk_storage);
+  ULONG source_addr = reinterpret_cast<ULONG>(target_);
+  ULONG target_addr = reinterpret_cast<ULONG>(thunk_storage);
   return target_addr + kMaxServiceSize - kJmp32Size - source_addr ==
          patched->service_id;
 }

services/data_decoder/gzipper.cc

@@ -4,7 +4,6 @@
 
 #include "services/data_decoder/gzipper.h"
 
-#include "base/bit_cast.h"
 #include "base/containers/span.h"
 #include "base/strings/string_piece.h"
 #include "mojo/public/cpp/base/big_buffer.h"
@@ -29,7 +28,7 @@ void Gzipper::Deflate(mojo_base::BigBuffer data, DeflateCallback callback) {
   std::vector<uint8_t> compressed_data(compressed_data_size);
   if (zlib_internal::CompressHelper(
           zlib_internal::ZRAW, compressed_data.data(), &compressed_data_size,
-          base::bit_cast<const Bytef*>(data.data()), data.size(),
+          reinterpret_cast<const Bytef*>(data.data()), data.size(),
           Z_DEFAULT_COMPRESSION,
           /*malloc_fn=*/nullptr, /*free_fn=*/nullptr) != Z_OK) {
     std::move(callback).Run(absl::nullopt);

services/network/network_service_memory_cache.cc

@@ -8,7 +8,6 @@
 #include <limits>
 #include <string_view>
 
-#include "base/bit_cast.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/metrics/field_trial_params.h"
@@ -137,7 +136,7 @@ bool CheckCrossOriginReadBlocking(const ResourceRequest& resource_request,
     const size_t size =
         std::min(static_cast<size_t>(net::kMaxBytesToSniff), content.size());
     decision = analyzer->Sniff(
-        std::string_view(base::bit_cast<const char*>(content.front()), size));
+        std::string_view(reinterpret_cast<const char*>(content.front()), size));
     if (decision == corb::ResponseAnalyzer::Decision::kSniffMore)
       decision = analyzer->HandleEndOfSniffableResponseBody();
     DCHECK_NE(decision, corb::ResponseAnalyzer::Decision::kSniffMore);

services/network/network_service_memory_cache_url_loader.cc

@@ -4,7 +4,6 @@
 
 #include "services/network/network_service_memory_cache_url_loader.h"
 
-#include "base/bit_cast.h"
 #include "base/task/sequenced_task_runner.h"
 #include "base/trace_event/common/trace_event_common.h"
 #include "base/trace_event/trace_event.h"
@@ -194,8 +193,8 @@ void NetworkServiceMemoryCacheURLLoader::WriteMore() {
   if (net_log_.IsCapturing()) {
     net_log_.AddByteTransferEvent(
         net::NetLogEventType::IN_MEMORY_CACHE_BYTES_READ, total_write_size,
-        base::bit_cast<const char*>(content_->data().data() +
-                                    original_write_position));
+        reinterpret_cast<const char*>(content_->data().data() +
+                                      original_write_position));
   }
 
   if (write_completed) {

styleguide/c++/c++-features.md

@@ -2196,6 +2196,32 @@ to `absl::bind_front`.
 Overlaps with `base::Bind`.
 ***
 
+### std::bit_cast <sup>[banned]</sup>
+
+```c++
+float quake_rsqrt(float number) {
+  long i = std::bit_cast<long>(number);
+  i = 0x5f3759df - (i >> 1);  // wtf?
+  float y = std::bit_cast<float>(i);
+  return y * (1.5f - (0.5f * number * y * y));
+}
+```
+
+**Description:** Returns an value constructed with the same bits as an value of
+a different type.
+
+**Documentation:**
+[`std::bit_cast`](https://en.cppreference.com/w/cpp/numeric/bit_cast)
+
+**Notes:**
+*** promo
+The `std::` version of `bit_cast` allows casting of pointer and reference types,
+which is both useless in that it doesn't avoid UB, and dangerous in that it
+allows arbitrary casting away of modifiers like `const`. Instead of using
+`bit_cast` on pointers, use standard C++ casts. For use on values, use
+`base::bit_cast` which does not allow this unwanted usage.
+***
+
 ### std::{c8rtomb,mbrtoc8} <sup>[banned]</sup>
 
 ```c++

third_party/blink/renderer/platform/wtf/hash_functions.h

@@ -127,7 +127,7 @@ bool FloatEqualForHash(T a, T b) {
 
 template <typename T>
 unsigned HashPointer(T* key) {
-  return HashInt(base::bit_cast<internal::IntHashBits<T*>>(key));
+  return HashInt(reinterpret_cast<internal::IntHashBits<T*>>(key));
 }
 
 // Useful compounding hash functions.

third_party/fdlibm/ieee754.cc

@@ -21,6 +21,7 @@
 #include <limits>
 
 #include "base/bit_cast.h"
+#include "base/compiler_specific.h"
 #include "build/build_config.h"
 #include "third_party/fdlibm/overflowing-math.h"
 

third_party/zlib/google/compression_utils.cc

@@ -4,7 +4,6 @@
 
 #include "third_party/zlib/google/compression_utils.h"
 
-#include "base/bit_cast.h"
 #include "base/check_op.h"
 #include "base/process/memory.h"
 #include "base/sys_byteorder.h"
@@ -24,8 +23,8 @@ bool GzipCompress(base::span<const char> input,
   // uLongf can be larger than size_t.
   uLongf compressed_size_long = static_cast<uLongf>(output_buffer_size);
   if (zlib_internal::GzipCompressHelper(
-          base::bit_cast<Bytef*>(output_buffer), &compressed_size_long,
-          base::bit_cast<const Bytef*>(input.data()),
+          reinterpret_cast<Bytef*>(output_buffer), &compressed_size_long,
+          reinterpret_cast<const Bytef*>(input.data()),
           static_cast<uLongf>(input.size()), malloc_fn, free_fn) != Z_OK) {
     return false;
   }
@@ -55,7 +54,7 @@ bool GzipCompress(base::span<const uint8_t> input, std::string* output) {
 
   if (zlib_internal::GzipCompressHelper(
           compressed_data, &compressed_data_size,
-          base::bit_cast<const Bytef*>(input.data()), input_size, nullptr,
+          reinterpret_cast<const Bytef*>(input.data()), input_size, nullptr,
           nullptr) != Z_OK) {
     free(compressed_data);
     return false;
@@ -82,8 +81,8 @@ bool GzipUncompress(const std::string& input, std::string* output) {
 
   uncompressed_output.resize(uncompressed_size);
   if (zlib_internal::GzipUncompressHelper(
-          base::bit_cast<Bytef*>(uncompressed_output.data()),
-          &uncompressed_size, base::bit_cast<const Bytef*>(input.data()),
+          reinterpret_cast<Bytef*>(uncompressed_output.data()),
+          &uncompressed_size, reinterpret_cast<const Bytef*>(input.data()),
           static_cast<uLongf>(input.length())) == Z_OK) {
     output->swap(uncompressed_output);
     return true;
@@ -102,8 +101,8 @@ bool GzipUncompress(base::span<const uint8_t> input,
   if (uncompressed_size > output.size())
     return false;
   return zlib_internal::GzipUncompressHelper(
-             base::bit_cast<Bytef*>(output.data()), &uncompressed_size,
-             base::bit_cast<const Bytef*>(input.data()),
+             reinterpret_cast<Bytef*>(const_cast<uint8_t*>(output.data())),
+             &uncompressed_size, reinterpret_cast<const Bytef*>(input.data()),
              static_cast<uLongf>(input.size())) == Z_OK;
 }
 
@@ -117,8 +116,8 @@ bool GzipUncompress(base::span<const uint8_t> input, std::string* output) {
   uLongf uncompressed_size = GetUncompressedSize(input);
   output->resize(uncompressed_size);
   return zlib_internal::GzipUncompressHelper(
-             base::bit_cast<Bytef*>(output->data()), &uncompressed_size,
-             base::bit_cast<const Bytef*>(input.data()),
+             reinterpret_cast<Bytef*>(output->data()), &uncompressed_size,
+             reinterpret_cast<const Bytef*>(input.data()),
              static_cast<uLongf>(input.size())) == Z_OK;
 }
 
@@ -128,7 +127,8 @@ uint32_t GetUncompressedSize(base::span<const char> compressed_data) {
 
 uint32_t GetUncompressedSize(base::span<const uint8_t> compressed_data) {
   return zlib_internal::GetGzipUncompressedSize(
-      base::bit_cast<Bytef*>(compressed_data.data()), compressed_data.size());
+      reinterpret_cast<const Bytef*>(compressed_data.data()),
+      compressed_data.size());
 }
 
 }  // namespace compression