From f7332fa80bb0612e87bbc237a1dfa87cf3bde5ce Mon Sep 17 00:00:00 2001 From: Elias Kosunen Date: Wed, 1 Nov 2023 21:11:14 +0200 Subject: [PATCH] Fix issues found with ASan and UBSan --- CHANGELOG.md | 2 ++ include/scn/detail/vectored.h | 5 ++--- include/scn/reader/int.h | 5 +++-- include/scn/unicode/common.h | 2 -- include/scn/unicode/utf16.h | 7 ++++--- include/scn/util/memory.h | 24 +++++++++++++++++++++--- include/scn/util/small_vector.h | 16 +++++++++++----- test/each/integer_each.cpp | 8 ++++++-- 8 files changed, 49 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 42465b96..f13f193e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ _Released 2023-xx-xx_ * Fix `scn::wrap(std::string_view&&)` being ambiguous on gcc (reported in #83) * Fix compiler error in `scn::basic_string_view::substr` (reported in #86) + * Fix memory safety issues found with ASan and UBsan in + `small_vector`, `detail::utf16::validate_next`, and `detail::get_buffer`. * Improve error messages given from the float parser # 1.1.2 diff --git a/include/scn/detail/vectored.h b/include/scn/detail/vectored.h index f5c38684..91de7011 100644 --- a/include/scn/detail/vectored.h +++ b/include/scn/detail/vectored.h @@ -49,7 +49,7 @@ namespace scn { if (begin >= buf_it->begin() && begin < buf_it->end()) { break; } - if (begin == buf_it->end()) { + if (begin == buf_it->end() && buf_it + 1 != s.end()) { ++buf_it; begin = buf_it->begin(); break; @@ -156,8 +156,7 @@ namespace scn { ::scn::custom_ranges::detail::_requires< provides_buffer_access_concept, Range, - ::scn::ranges::iterator_t>::value> { - }; + ::scn::ranges::iterator_t>::value> {}; } // namespace detail SCN_END_NAMESPACE diff --git a/include/scn/reader/int.h b/include/scn/reader/int.h index 06259d96..bc923e9f 100644 --- a/include/scn/reader/int.h +++ b/include/scn/reader/int.h @@ -258,8 +258,9 @@ namespace scn { } auto it = r.value(); - std::basic_string str(to_address(it), - s.size()); + std::basic_string str( + to_address(it), + static_cast(std::distance(it, s.end()))); ret = ctx.locale().get_localized().read_num( tmp, str, static_cast(base)); diff --git a/include/scn/unicode/common.h b/include/scn/unicode/common.h index 38077937..fd65fde2 100644 --- a/include/scn/unicode/common.h +++ b/include/scn/unicode/common.h @@ -73,8 +73,6 @@ namespace scn { static constexpr const uint16_t trail_surrogate_max = 0xdfff; static constexpr const uint16_t lead_offset = lead_surrogate_min - (0x10000u >> 10); - static constexpr const uint32_t surrogate_offset = - 0x10000u - (lead_surrogate_min << 10) - trail_surrogate_min; static constexpr const uint32_t code_point_max = 0x10ffff; template diff --git a/include/scn/unicode/utf16.h b/include/scn/unicode/utf16.h index 8d8a4009..4e310d74 100644 --- a/include/scn/unicode/utf16.h +++ b/include/scn/unicode/utf16.h @@ -63,9 +63,10 @@ namespace scn { "Invalid utf16 trail surrogate"}; } ++it; - cp = static_cast( - static_cast(lead << 10u) + trail + - surrogate_offset); + auto lead32 = static_cast(lead) - 0xd800u; + auto trail32 = static_cast(trail) - 0xdc00u; + cp = static_cast(((lead32 << 10) | trail32) + + 0x10000u); return {}; } if (is_trail_surrogate(lead)) { diff --git a/include/scn/util/memory.h b/include/scn/util/memory.h index 9dfb9705..0bdc5a5a 100644 --- a/include/scn/util/memory.h +++ b/include/scn/util/memory.h @@ -176,6 +176,8 @@ namespace scn { constexpr bool B = std::is_trivially_copyable::value && std::is_pointer::value && sizeof(T) == 1; + SCN_EXPECT(detail::to_address(first) != nullptr); + SCN_EXPECT(detail::to_address(last) != nullptr); return uninitialized_fill(first, last, value, std::integral_constant{}); } @@ -186,6 +188,8 @@ namespace scn { { using value_type = typename std::iterator_traits::value_type; + SCN_EXPECT(detail::to_address(first) != nullptr); + SCN_EXPECT(detail::to_address(last) != nullptr); ForwardIt current = first; for (; current != last; ++current) { ::new (static_cast(std::addressof(*current))) value_type; @@ -198,6 +202,8 @@ namespace scn { using value_type = typename std::iterator_traits::value_type; ForwardIt current = first; + SCN_EXPECT(detail::to_address(first) != nullptr); + SCN_EXPECT(detail::to_address(last) != nullptr); for (; current != last; ++current) { ::new (static_cast(std::addressof(*current))) value_type(); @@ -216,6 +222,9 @@ namespace scn { using value_type = typename std::iterator_traits::value_type; ForwardIt current = d_first; + SCN_EXPECT(detail::to_address(first) != nullptr); + SCN_EXPECT(detail::to_address(last) != nullptr); + SCN_EXPECT(detail::to_address(d_first) != nullptr); for (; first != last; ++first, (void)++current) { ::new (static_cast(std::addressof(*current))) value_type(*first); @@ -234,6 +243,9 @@ namespace scn { using value_type = typename std::iterator_traits::value_type; using pointer = typename std::iterator_traits::pointer; + SCN_EXPECT(detail::to_address(first) != nullptr); + SCN_EXPECT(detail::to_address(last) != nullptr); + SCN_EXPECT(detail::to_address(d_first) != nullptr); auto ptr = std::memcpy(std::addressof(*d_first), std::addressof(*first), static_cast(std::distance(first, last)) * @@ -252,6 +264,9 @@ namespace scn { { using value_type = typename std::iterator_traits::value_type; + SCN_EXPECT(detail::to_address(first) != nullptr); + SCN_EXPECT(detail::to_address(last) != nullptr); + SCN_EXPECT(detail::to_address(d_first) != nullptr); ForwardIt current = d_first; for (; first != last; ++first, (void)++current) { ::new (static_cast(std::addressof(*current))) @@ -271,6 +286,9 @@ namespace scn { using value_type = typename std::iterator_traits::value_type; using pointer = typename std::iterator_traits::pointer; + SCN_EXPECT(detail::to_address(first) != nullptr); + SCN_EXPECT(detail::to_address(last) != nullptr); + SCN_EXPECT(detail::to_address(d_first) != nullptr); auto ptr = std::memcpy(std::addressof(*d_first), std::addressof(*first), static_cast(std::distance(first, last)) * @@ -289,12 +307,12 @@ namespace scn { erased_storage(T val) noexcept( std::is_nothrow_move_constructible::value) - : m_ptr(::new (static_cast(&m_data)) T(SCN_MOVE(val))) + : m_ptr(::new(static_cast(&m_data)) T(SCN_MOVE(val))) { } erased_storage(const erased_storage& other) - : m_ptr(other ? ::new (static_cast(&m_data)) + : m_ptr(other ? ::new(static_cast(&m_data)) T(other.get()) : nullptr) { @@ -309,7 +327,7 @@ namespace scn { } erased_storage(erased_storage&& other) noexcept - : m_ptr(other ? ::new (static_cast(&m_data)) + : m_ptr(other ? ::new(static_cast(&m_data)) T(SCN_MOVE(other.get())) : nullptr) { diff --git a/include/scn/util/small_vector.h b/include/scn/util/small_vector.h index 93a55147..de1b43bb 100644 --- a/include/scn/util/small_vector.h +++ b/include/scn/util/small_vector.h @@ -181,8 +181,7 @@ namespace scn { using reverse_iterator = std::reverse_iterator; using const_reverse_iterator = std::reverse_iterator; - struct stack_storage : basic_stack_storage { - }; + struct stack_storage : basic_stack_storage {}; struct heap_storage { size_type cap{0}; }; @@ -674,13 +673,15 @@ namespace scn { private: stack_storage& _construct_stack_storage() noexcept { - ::new (std::addressof(m_stack_storage)) stack_storage; + ::new (static_cast(std::addressof(m_stack_storage))) + stack_storage; m_ptr = m_stack_storage.reinterpret_unconstructed_data(); return m_stack_storage; } heap_storage& _construct_heap_storage() noexcept { - ::new (std::addressof(m_heap_storage)) heap_storage; + ::new (static_cast(std::addressof(m_heap_storage))) + heap_storage; m_ptr = nullptr; return m_heap_storage; } @@ -700,6 +701,9 @@ namespace scn { void _destruct_elements() noexcept { + if (!m_ptr) { + return; + } const auto s = size(); for (size_type i = 0; i != s; ++i) { m_ptr[i].~T(); @@ -724,7 +728,9 @@ namespace scn { auto ptr = static_cast(static_cast(storage_ptr)); auto n = size(); - uninitialized_move(begin(), end(), ptr); + if (!empty()) { + uninitialized_move(begin(), end(), ptr); + } _destruct(); auto& heap = [this]() -> heap_storage& { if (is_small()) { diff --git a/test/each/integer_each.cpp b/test/each/integer_each.cpp index 6ee56ab7..54e621f6 100644 --- a/test/each/integer_each.cpp +++ b/test/each/integer_each.cpp @@ -37,11 +37,15 @@ TEST_CASE_TEMPLATE_DEFINE("integer each", T, integer_each_test) CHECK(tmp == val); }; auto random_ints = [](size_t n, T a, T b) { + using dist_type = + typename std::conditional::value, long long, + unsigned long long>::type; std::default_random_engine rng(std::random_device{}()); - std::uniform_int_distribution dist(a, b); + std::uniform_int_distribution dist( + static_cast(a), static_cast(b)); std::vector vec; std::generate_n(std::back_inserter(vec), n, - [&]() { return dist(rng); }); + [&]() { return static_cast(dist(rng)); }); return vec; }; for (T i = min; i != min + 1000; ++i) {