Try to fix flex-vector-st fuzzer

arximboldi · Feb 14, 2024 · 71d16f0 · 71d16f0
1 parent 0bcee97
commit 71d16f0
Show file tree

Hide file tree

Showing 6 changed files with 100 additions and 21 deletions.
diff --git a/extra/fuzzer/CMakeLists.txt b/extra/fuzzer/CMakeLists.txt
@@ -25,6 +25,7 @@ foreach(_file IN LISTS immer_fuzzers)
   add_executable(${_target} EXCLUDE_FROM_ALL "${_file}")
   set_target_properties(${_target} PROPERTIES OUTPUT_NAME ${_output})
   target_compile_options(${_target} PUBLIC ${immer_fuzzing_engine})
+  target_compile_definitions(${_target} PUBLIC IMMER_THROW_ON_INVALID_STATE=1)
   target_link_libraries(${_target} PUBLIC ${immer_fuzzing_engine} immer-dev)
   add_dependencies(fuzzers ${_target})
   if(CHECK_FUZZERS)

diff --git a/extra/fuzzer/flex-vector-st.cpp b/extra/fuzzer/flex-vector-st.cpp
@@ -42,8 +42,25 @@ extern "C" int LLVMFuzzerTestOneInput(const std::uint8_t* data,
     auto is_valid_size = [](auto& v) {
         return [&](auto idx) { return idx >= 0 && idx <= v.size(); };
     };
-    auto can_concat = [](auto&& v1, auto&& v2) {
-        return v1.size() + v2.size() < vector_t::max_size() / 4;
+    auto can_concat = [](const auto& v1, const auto& v2) {
+        // First, check max_size
+        if (v1.size() + v2.size() > vector_t::max_size()) {
+            return false;
+        }
+
+        // But just checking max_size is not sufficient, because there are other
+        // conditions for the validity of the tree, like shift constraints, for
+        // example.
+        try {
+            // Try to concat and catch an exception if it fails
+            const auto v3 = v1 + v2;
+            if (v3.size()) {
+                return true;
+            }
+        } catch (const immer::detail::rbts::invalid_tree&) {
+            return false;
+        }
+        return true;
     };
     auto can_compare = [](auto&& v) {
         // avoid comparing vectors that are too big, and hence, slow to compare

diff --git a/immer/config.hpp b/immer/config.hpp
@@ -121,6 +121,10 @@
 #endif
 #endif
 
+#ifndef IMMER_THROW_ON_INVALID_STATE
+#define IMMER_THROW_ON_INVALID_STATE 0
+#endif
+
 namespace immer {
 
 const auto default_bits           = 5;

diff --git a/immer/detail/rbts/rrbtree.hpp b/immer/detail/rbts/rrbtree.hpp
@@ -25,6 +25,16 @@ namespace immer {
 namespace detail {
 namespace rbts {
 
+#if IMMER_THROW_ON_INVALID_STATE
+struct invalid_tree : std::exception
+{};
+#define IMMER_INVALID_STATE_ASSERT(expr)                                       \
+    if (!(expr))                                                               \
+    IMMER_THROW(invalid_tree{})
+#else
+#define IMMER_INVALID_STATE_ASSERT(expr) assert(expr)
+#endif
+
 template <typename T, typename MemoryPolicy, bits_t B, bits_t BL>
 struct rrbtree_iterator;
 
@@ -107,13 +117,32 @@ struct rrbtree
         assert(check_tree());
     }
 
-    rrbtree(size_t sz, shift_t sh, node_t* r, node_t* t) noexcept
+    rrbtree(size_t sz, shift_t sh, node_t* r, node_t* t)
+#if IMMER_THROW_ON_INVALID_STATE
+#else
+        noexcept
+#endif
         : size{sz}
         , shift{sh}
         , root{r}
         , tail{t}
     {
+#if IMMER_THROW_ON_INVALID_STATE
+        // assert only happens in the Debug build, but when
+        // IMMER_THROW_ON_INVALID_STATE is activated, we want to just check_tree
+        // even in Release.
+        try {
+            check_tree();
+        } catch (...) {
+            // This not fully constructed rrbtree owns the nodes already, have
+            // to dec them.
+            if (r && t)
+                dec();
+            throw;
+        }
+#else
         assert(check_tree());
+#endif
     }
 
     rrbtree(const rrbtree& other) noexcept
@@ -1339,13 +1368,13 @@ struct rrbtree
 
     bool check_tree() const
     {
-        assert(shift <= sizeof(size_t) * 8 - BL);
-        assert(shift >= BL);
-        assert(tail_offset() <= size);
-        assert(tail_size() <= branches<BL>);
+        IMMER_INVALID_STATE_ASSERT(shift <= sizeof(size_t) * 8 - BL);
+        IMMER_INVALID_STATE_ASSERT(shift >= BL);
+        IMMER_INVALID_STATE_ASSERT(tail_offset() <= size);
+        IMMER_INVALID_STATE_ASSERT(tail_size() <= branches<BL>);
 #if IMMER_DEBUG_DEEP_CHECK
-        assert(check_root());
-        assert(check_tail());
+        IMMER_INVALID_STATE_ASSERT(check_root());
+        IMMER_INVALID_STATE_ASSERT(check_tail());
 #endif
         return true;
     }

diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
@@ -27,8 +27,8 @@ foreach(_file IN LISTS immer_unit_tests)
   add_dependencies(tests ${_target})
   target_compile_definitions(
     ${_target}
-    PUBLIC
-      -DIMMER_OSS_FUZZ_DATA_PATH="${CMAKE_CURRENT_SOURCE_DIR}/oss-fuzz/data")
+    PUBLIC IMMER_OSS_FUZZ_DATA_PATH="${CMAKE_CURRENT_SOURCE_DIR}/oss-fuzz/data"
+           IMMER_THROW_ON_INVALID_STATE=1)
   target_link_libraries(${_target} PUBLIC immer-dev Catch2::Catch2WithMain)
   set_target_properties(${_target} PROPERTIES CXX_VISIBILITY_PRESET hidden)
   add_test("test/${_output}" ${_output})

diff --git a/test/oss-fuzz/flex-vector-st-0.cpp b/test/oss-fuzz/flex-vector-st-0.cpp
@@ -63,16 +63,25 @@ int run_input(const std::uint8_t* data, std::size_t size)
     auto is_valid_size = [](auto& v) {
         return [&](auto idx) { return idx >= 0 && idx <= v.size(); };
     };
-    auto can_concat = [](auto&& v1, auto&& v2) {
-        const bool result = v1.size() + v2.size() < vector_t::max_size() / 4;
-        if (result) {
-            IMMER_FUZZED_TRACE("can_concat {} and {} (total is {}), max is {}",
-                               v1.size(),
-                               v2.size(),
-                               v1.size() + v2.size(),
-                               vector_t::max_size() / 4);
-        }
-        return result;
+    auto can_concat = [](const auto& v1, const auto& v2) {
+        // First, check max_size
+        if (v1.size() + v2.size() > vector_t::max_size()) {
+            return false;
+        }
+
+        // But just checking max_size is not sufficient, because there are other
+        // conditions for the validity of the tree, like shift constraints, for
+        // example.
+        try {
+            // Try to concat and catch an exception if it fails
+            const auto v3 = v1 + v2;
+            if (v3.size()) {
+                return true;
+            }
+        } catch (const immer::detail::rbts::invalid_tree&) {
+            return false;
+        }
+        return true;
     };
     auto can_compare = [](auto&& v) {
         // avoid comparing vectors that are too big, and hence, slow to compare
@@ -241,3 +250,22 @@ TEST_CASE("some fuzzer crash I don't have a link for")
         CHECK(run_input(input.data(), input.size()) == 0);
     }
 }
+
+TEST_CASE("Creating rrbtree with invalid parameters shouldn't terminate but "
+          "should throw")
+{
+    SECTION("Invalid shift")
+    {
+        // assert(shift <= sizeof(size_t) * 8 - BL);
+        // assert(shift >= BL);
+        const auto create = [] {
+            const auto shift = immer::detail::rbts::shift_t{999};
+            return immer::detail::rbts::
+                rrbtree<int, st_memory, immer::default_bits, 1>{
+                    0, shift, nullptr, nullptr};
+        };
+
+        REQUIRE(IMMER_THROW_ON_INVALID_STATE == 1);
+        REQUIRE_THROWS_AS(create(), immer::detail::rbts::invalid_tree);
+    }
+}