From 2f839917d4f1005098b926f975456d177cbd419a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Meusel?= Date: Fri, 14 Jun 2024 16:19:40 +0200 Subject: [PATCH] FIX: store_be(size_t(42)) would not compile on Xcode The affected platform defines uint64_t as "unsigned long long" and size_t as "unsigned long int". Both are 64bit types, but the compiler failed to call reverse_bytes() for size_t regardless. It claimed the call is ambiguous. --- src/lib/utils/bswap.h | 56 +++++++++++++++++----------------------- src/tests/test_utils.cpp | 31 ++++++++++++++++++++++ 2 files changed, 55 insertions(+), 32 deletions(-) diff --git a/src/lib/utils/bswap.h b/src/lib/utils/bswap.h index 2a7045a4363..20d9cf6d34f 100644 --- a/src/lib/utils/bswap.h +++ b/src/lib/utils/bswap.h @@ -2,6 +2,7 @@ * Byte Swapping Operations * (C) 1999-2011,2018 Jack Lloyd * (C) 2007 Yves Jerschow +* (C) 2024 René Meusel - Rohde & Schwarz Cybersecurity * * TODO: C++23: replace this entire implementation with std::byteswap * @@ -16,49 +17,40 @@ namespace Botan { /** -* Swap a 16 bit integer -*/ -inline constexpr uint16_t reverse_bytes(uint16_t x) { + * Swap the byte order of an unsigned integer + */ +template + requires(sizeof(T) == 1 || sizeof(T) == 2 || sizeof(T) == 4 || sizeof(T) == 8) +inline constexpr T reverse_bytes(T x) { + if constexpr(sizeof(T) == 1) { + return x; + } else if constexpr(sizeof(T) == 2) { #if BOTAN_COMPILER_HAS_BUILTIN(__builtin_bswap16) - return __builtin_bswap16(x); + return static_cast(__builtin_bswap16(x)); #else - return static_cast((x << 8) | (x >> 8)); + return static_cast((x << 8) | (x >> 8)); #endif -} - -/** -* Swap a 32 bit integer -* -* We cannot use MSVC's _byteswap_ulong because it does not consider -* the builtin to be constexpr. -*/ -inline constexpr uint32_t reverse_bytes(uint32_t x) { + } else if constexpr(sizeof(T) == 4) { #if BOTAN_COMPILER_HAS_BUILTIN(__builtin_bswap32) - return __builtin_bswap32(x); + return static_cast(__builtin_bswap32(x)); #else - // MSVC at least recognizes this as a bswap - return ((x & 0x000000FF) << 24) | ((x & 0x0000FF00) << 8) | ((x & 0x00FF0000) >> 8) | ((x & 0xFF000000) >> 24); + // MSVC at least recognizes this as a bswap + return static_cast(((x & 0x000000FF) << 24) | ((x & 0x0000FF00) << 8) | ((x & 0x00FF0000) >> 8) | + ((x & 0xFF000000) >> 24)); #endif -} - -/** -* Swap a 64 bit integer -* -* We cannot use MSVC's _byteswap_uint64 because it does not consider -* the builtin to be constexpr. -*/ -inline constexpr uint64_t reverse_bytes(uint64_t x) { + } else if constexpr(sizeof(T) == 8) { #if BOTAN_COMPILER_HAS_BUILTIN(__builtin_bswap64) - return __builtin_bswap64(x); + return static_cast(__builtin_bswap64(x)); #else - uint32_t hi = static_cast(x >> 32); - uint32_t lo = static_cast(x); + uint32_t hi = static_cast(x >> 32); + uint32_t lo = static_cast(x); - hi = reverse_bytes(hi); - lo = reverse_bytes(lo); + hi = reverse_bytes(hi); + lo = reverse_bytes(lo); - return (static_cast(lo) << 32) | hi; + return (static_cast(lo) << 32) | hi; #endif + } } } // namespace Botan diff --git a/src/tests/test_utils.cpp b/src/tests/test_utils.cpp index af2e8757cee..751639c23fb 100644 --- a/src/tests/test_utils.cpp +++ b/src/tests/test_utils.cpp @@ -44,6 +44,7 @@ class Utility_Function_Tests final : public Test { results.push_back(test_checked_cast()); results.push_back(test_round_up()); results.push_back(test_loadstore()); + results.push_back(test_loadstore_ambiguity()); results.push_back(test_loadstore_fallback()); results.push_back(test_loadstore_constexpr()); return Botan::concat(results, test_copy_out_be_le()); @@ -496,6 +497,36 @@ class Utility_Function_Tests final : public Test { template using a = std::array; + static Test::Result test_loadstore_ambiguity() { + // This is a regression test for a (probable) compiler bug in Xcode 15 + // where it would fail to compile the load/store functions for size_t + // + // It seems that this platform defines uint64_t as "unsigned long long" + // and size_t as "unsigned long". Both are 64-bits but the compiler + // was unable to disambiguate the two in reverse_bytes in bswap.h + + const uint32_t in32 = 0x01234567; + const uint64_t in64 = 0x0123456789ABCDEF; + const size_t inszt = 0x87654321; + + Test::Result result("Util load/store ambiguity"); + const auto out_be_32 = Botan::store_be(in32); + const auto out_le_32 = Botan::store_le(in32); + const auto out_be_64 = Botan::store_be(in64); + const auto out_le_64 = Botan::store_le(in64); + const auto out_be_szt = Botan::store_be(inszt); + const auto out_le_szt = Botan::store_le(inszt); + + result.test_is_eq("be 32", Botan::load_be(out_be_32), in32); + result.test_is_eq("le 32", Botan::load_le(out_le_32), in32); + result.test_is_eq("be 64", Botan::load_be(out_be_64), in64); + result.test_is_eq("le 64", Botan::load_le(out_le_64), in64); + result.test_is_eq("be szt", Botan::load_be(out_be_szt), inszt); + result.test_is_eq("le szt", Botan::load_le(out_le_szt), inszt); + + return result; + } + static Test::Result test_loadstore_fallback() { // The fallback implementation is only used if we don't know the // endianness of the target at compile time. This makes sure that the