Handle cases where passing an overly long size could cause UB due to integer overflow

Author: kkaefer

.github/workflows/clang-tidy.yml

@@ -15,7 +15,7 @@ jobs:
           - image: "debian:testing"
             clang: 19
           - image: "debian:experimental"
-            clang: 20
+            clang: 22
     container:
       image: ${{ matrix.image }}
       env:
@@ -33,6 +33,7 @@ jobs:
           apt-get install -yq \
             clang-${{ matrix.clang }} \
             clang-tidy-${{ matrix.clang }} \
+            libstdc++-12-dev \
             cmake \
             libprotobuf-dev \
             make \

include/protozero/pbf_reader.hpp

@@ -30,6 +30,7 @@ documentation.
 #include <cstddef>
 #include <cstdint>
 #include <cstring>
+#include <limits>
 #include <string>
 #include <utility>
 
@@ -139,6 +140,20 @@ class pbf_reader {
                 T{m_data, m_data}};
     }
 
+    // Adds size to ptr safely, and returning ptr if overflow would occur.
+    const char* safe_ptr_add(const char* ptr, size_t length) {
+#if defined __has_builtin
+#if __has_builtin(__builtin_add_overflow)
+        uintptr_t result;
+        return __builtin_add_overflow(reinterpret_cast<uintptr_t>(ptr), length, &result) ? ptr : reinterpret_cast<const char*>(result);
+#endif
+#endif
+        if (length > std::numeric_limits<uintptr_t>::max() - reinterpret_cast<uintptr_t>(ptr)) {
+            return ptr;
+        }
+        return ptr + length;
+    }
+
 public:
 
     /**
@@ -153,7 +168,7 @@ class pbf_reader {
      */
     explicit pbf_reader(const data_view& view) noexcept
         : m_data{view.data()},
-          m_end{view.data() + view.size()} {
+          m_end{safe_ptr_add(view.data(), view.size())} {
     }
 
     /**
@@ -168,7 +183,7 @@ class pbf_reader {
      */
     pbf_reader(const char* data, std::size_t size) noexcept
         : m_data{data},
-          m_end{data + size} {
+          m_end{safe_ptr_add(data, size)} {
     }
 
 #ifndef PROTOZERO_STRICT_API
@@ -185,7 +200,7 @@ class pbf_reader {
      */
     explicit pbf_reader(const std::pair<const char*, std::size_t>& data) noexcept
         : m_data{data.first},
-          m_end{data.first + data.second} {
+          m_end{safe_ptr_add(data.first, data.second)} {
     }
 #endif
 
@@ -201,7 +216,7 @@ class pbf_reader {
      */
     explicit pbf_reader(const std::string& data) noexcept
         : m_data{data.data()},
-          m_end{data.data() + data.size()} {
+          m_end{safe_ptr_add(data.data(), data.size())} {
     }
 
     /**

include/protozero/varint.hpp

@@ -32,13 +32,16 @@ namespace detail {
 
     // from https://github.com/facebook/folly/blob/master/folly/Varint.h
     inline uint64_t decode_varint_impl(const char** data, const char* end) {
-        const auto* begin = reinterpret_cast<const int8_t*>(*data);
-        const auto* iend = reinterpret_cast<const int8_t*>(end);
-        const int8_t* p = begin;
+        const int8_t* p = reinterpret_cast<const int8_t*>(*data);
+        const int8_t* const iend = reinterpret_cast<const int8_t*>(end);
         uint64_t val = 0;
 
-        if (iend - begin >= max_varint_length) {  // fast path
+        if (iend - p >= max_varint_length) {  // fast path
             do {
+// GCC 12 has trouble understanding that we're not actually taking this branch for short buffers.
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Warray-bounds"
+#endif
                 int64_t b = *p++;
                           val  = ((static_cast<uint64_t>(b) & 0x7fU)       ); if (b >= 0) { break; }
                 b = *p++; val |= ((static_cast<uint64_t>(b) & 0x7fU) <<  7U); if (b >= 0) { break; }
@@ -50,6 +53,7 @@ namespace detail {
                 b = *p++; val |= ((static_cast<uint64_t>(b) & 0x7fU) << 49U); if (b >= 0) { break; }
                 b = *p++; val |= ((static_cast<uint64_t>(b) & 0x7fU) << 56U); if (b >= 0) { break; }
                 b = *p++; val |= ((static_cast<uint64_t>(b) & 0x01U) << 63U); if (b >= 0) { break; }
+#pragma GCC diagnostic pop
                 throw varint_too_long_exception{};
             } while (false);
         } else {

test/CMakeLists.txt

@@ -102,7 +102,7 @@ if(PROTOBUF_FOUND)
     target_link_libraries(writer_tests PRIVATE protobuf::libprotobuf-lite)
 
     if(NOT MSVC)
-        set_target_properties(writer_tests PROPERTIES COMPILE_FLAGS "-pthread")
+        set_target_properties(writer_tests PROPERTIES COMPILE_FLAGS "-pthread -Wno-nullability-extension")
         if(NOT APPLE)
             set_target_properties(writer_tests PROPERTIES LINK_FLAGS "-pthread")
         endif()

test/unit/test_basic.cpp

@@ -48,6 +48,20 @@ TEST_CASE("empty buffer in pbf_reader is okay") {
     REQUIRE_FALSE(item.next());
 }
 
+TEST_CASE("buffer that overflows address space") {
+    const char buffer[1] = {0};
+    // Create a pbf_reader with a length that overflows address space.
+    // This should be handled gracefully.
+    // We use a length of size_t(-1) which is the largest possible length.
+    // This should not cause any reads or writes outside of the provided buffer.
+    // The pbf_reader should behave as if the buffer is empty.
+    protozero::pbf_reader item{buffer, size_t(-1)};
+
+    REQUIRE(item.length() == 0);
+    REQUIRE_FALSE(item); // test operator bool()
+    REQUIRE_FALSE(item.next());
+}
+
 TEST_CASE("check every possible value for single byte in buffer") {
     char buffer[1];
     for (int i = 0; i <= 255; ++i) {