From 032450edf6f11744fd68795680131a6e3d994971 Mon Sep 17 00:00:00 2001
From: Zachary Dremann <dremann@gmail.com>
Date: Thu, 19 Sep 2024 19:52:17 -0400
Subject: [PATCH] fix: possible segfault in 64 bit deserialize_safe (#663)

---
 src/roaring64.c                   | 21 +++++++++++++++-
 tests/roaring64_serialization.cpp | 41 +++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/src/roaring64.c b/src/roaring64.c
index 43c8ccaff..14011c649 100644
--- a/src/roaring64.c
+++ b/src/roaring64.c
@@ -1965,7 +1965,8 @@ roaring64_bitmap_t *roaring64_bitmap_portable_deserialize_safe(
         memcpy(&high32, buf, sizeof(high32));
         buf += sizeof(high32);
         read_bytes += sizeof(high32);
-        if (high32 < previous_high32) {
+        // High 32 bits must be strictly increasing.
+        if (high32 <= previous_high32) {
             roaring64_bitmap_free(r);
             return NULL;
         }
@@ -1987,6 +1988,24 @@ roaring64_bitmap_t *roaring64_bitmap_portable_deserialize_safe(
         buf += bitmap32_size;
         read_bytes += bitmap32_size;
 
+        // While we don't attempt to validate much, we must ensure that there
+        // is no duplication in the high 48 bits - inserting into the ART
+        // assumes (or UB) no duplicate keys. The top 32 bits must be unique
+        // because we check for strict increasing values of  high32, but we
+        // must also ensure the top 16 bits within each 32-bit bitmap are also
+        // at least unique (we ensure they're strictly increasing as well,
+        // which they must be for a _valid_ bitmap, since it's cheaper to check)
+        int32_t last_bitmap_key = -1;
+        for (int i = 0; i < bitmap32->high_low_container.size; i++) {
+            uint16_t key = bitmap32->high_low_container.keys[i];
+            if (key <= last_bitmap_key) {
+                roaring_bitmap_free(bitmap32);
+                roaring64_bitmap_free(r);
+                return NULL;
+            }
+            last_bitmap_key = key;
+        }
+
         // Insert all containers of the 32-bit bitmap into the 64-bit bitmap.
         move_from_roaring32_offset(r, bitmap32, high32);
         roaring_bitmap_free(bitmap32);
diff --git a/tests/roaring64_serialization.cpp b/tests/roaring64_serialization.cpp
index 2364088ea..d71ea2a7a 100644
--- a/tests/roaring64_serialization.cpp
+++ b/tests/roaring64_serialization.cpp
@@ -85,6 +85,46 @@ DEFINE_TEST(test_64mapspreadvals) {
     assert_true(test_serialization("64mapspreadvals.bin"));
 }
 
+DEFINE_TEST(test_64deseroverlappingkeys) {
+    // clang-format off
+    char simple_bitmap[] = {
+        // Number of 32 bit bitmaps
+        1, 0, 0, 0, 0, 0, 0, 0,
+        // Top 32 bits of the first bitmap
+        0, 0, 0, 0,
+        // Serial Cookie
+        0x3B, 0x30,
+        // Container count - 1
+        1, 0,
+        // Run Flag Bitset (no runs)
+        0, 0,
+        // Upper 16 bits of the first container
+        0, 0,
+        // Cardinality - 1 of the first container
+        0, 0,
+        // Upper 16 bits of the second container - DUPLICATE
+        0, 0,
+        // Cardinality - 1 of the second container
+        0, 0,
+        // Only value of first container
+        0, 0,
+        // Only value of second container
+        0, 0,
+    };
+    // clang-format on
+
+    roaring64_bitmap_t* r = roaring64_bitmap_portable_deserialize_safe(
+        simple_bitmap, sizeof(simple_bitmap));
+    const char* reason = nullptr;
+    if (r != nullptr) {
+        if (roaring64_bitmap_internal_validate(r, &reason)) {
+            fail_msg(
+                "Validation must fail if a bitmap was returned, duplicate keys "
+                "are not allowed.");
+        }
+        roaring64_bitmap_free(r);
+    }
+}
 }  // namespace
 
 int main() {
@@ -101,6 +141,7 @@ int main() {
         cmocka_unit_test(test_64mapkeytoosmall),
         cmocka_unit_test(test_64mapsizetoosmall),
         cmocka_unit_test(test_64mapspreadvals),
+        cmocka_unit_test(test_64deseroverlappingkeys),
     };
     return cmocka_run_group_tests(tests, NULL, NULL);
 #endif  // CROARING_IS_BIG_ENDIAN