Merge pull request #16482 from vespa-engine/toregge/reenable-boundary-check-for-lid-mapping

Reenable boundary checks when mapping lid to lid for referenced document type.

6 files changed, 44 insertions, 3 deletions

diff --git a/searchlib/src/tests/attribute/imported_attribute_vector/imported_attribute_vector_test.cpp b/searchlib/src/tests/attribute/imported_attribute_vector/imported_attribute_vector_test.cpp
index 2d30f89015d..3a74a16794d 100644
--- a/searchlib/src/tests/attribute/imported_attribute_vector/imported_attribute_vector_test.cpp
+++ b/searchlib/src/tests/attribute/imported_attribute_vector/imported_attribute_vector_test.cpp
@@ -226,6 +226,20 @@ TEST_F("isUndefined() works for primitive attribute type", Fixture) {
     EXPECT_TRUE(f.get_imported_attr()->isUndefined(DocId(2))); // Not mapped
 }
 
+TEST_F("original lid range is used by read guard", Fixture)
+{
+    reset_with_single_value_reference_mappings<IntegerAttribute, int32_t>(
+            f, BasicType::INT32,
+            {{DocId(1), dummy_gid(3), DocId(3), 1234}});
+    auto first_guard = f.get_imported_attr();
+    add_n_docs_with_undefined_values(*f.reference_attr, 1);
+    f.map_reference(DocId(10), dummy_gid(3), DocId(3));
+    auto second_guard = f.get_imported_attr();
+    EXPECT_EQUAL(1234, second_guard->getInt(DocId(10)));
+    EXPECT_NOT_EQUAL(1234, first_guard->getInt(DocId(10)));
+    EXPECT_EQUAL(getUndefined<int>(), first_guard->getInt(DocId(10)));
+}
+
 struct SingleStringAttrFixture : Fixture {
     SingleStringAttrFixture() : Fixture() {
         setup();
diff --git a/searchlib/src/tests/attribute/imported_search_context/imported_search_context_test.cpp b/searchlib/src/tests/attribute/imported_search_context/imported_search_context_test.cpp
index 1bc87ef0da5..0be6d58bb8f 100644
--- a/searchlib/src/tests/attribute/imported_search_context/imported_search_context_test.cpp
+++ b/searchlib/src/tests/attribute/imported_search_context/imported_search_context_test.cpp
@@ -362,6 +362,16 @@ TEST_F("Multiple iterators can be created from the same context", SingleValueFix
     EXPECT_TRUE(is_hit_with_weight(*iter2, match2, DocId(3), 1));
 }
 
+TEST_F("original lid range is used by search context", SingleValueFixture)
+{
+    auto first_ctx = f.create_context(word_term("5678"));
+    add_n_docs_with_undefined_values(*f.reference_attr, 1);
+    f.map_reference(DocId(10), dummy_gid(5), DocId(5));
+    auto second_ctx = f.create_context(word_term("5678"));
+    EXPECT_FALSE(first_ctx->matches(DocId(10)));
+    EXPECT_TRUE(second_ctx->matches(DocId(10)));
+} 
+
 // Note: this uses an underlying string attribute, as queryTerm() does not seem to
 // implemented at all for (single) numeric attributes. Intentional?
 TEST_F("queryTerm() returns term context was created with", WsetValueFixture) {
diff --git a/searchlib/src/tests/attribute/reference_attribute/reference_attribute_test.cpp b/searchlib/src/tests/attribute/reference_attribute/reference_attribute_test.cpp
index 793cb3b4cef..b6b8e0a60e8 100644
--- a/searchlib/src/tests/attribute/reference_attribute/reference_attribute_test.cpp
+++ b/searchlib/src/tests/attribute/reference_attribute/reference_attribute_test.cpp
@@ -256,6 +256,18 @@ TEST_F(ReferenceAttributeTest, reference_for_a_document_can_be_cleared)
     assertNoRef(2);
 }
 
+TEST_F(ReferenceAttributeTest, lid_beyond_range_is_mapped_to_zero)
+{
+    auto factory = std::make_shared<MyGidToLidMapperFactory>();
+    setGidToLidMapperFactory(factory);
+    ensureDocIdLimit(5);
+    _attr->addDocs(1);
+    set(5, toGid(doc2));
+    EXPECT_EQ(0, _attr->getTargetLid(5));
+    _attr->commit();
+    EXPECT_EQ(17, _attr->getTargetLid(5));
+}
+
 TEST_F(ReferenceAttributeTest, read_guard_protects_references)
 {
     ensureDocIdLimit(5);
diff --git a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.h b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.h
index b31b11b8c74..cb803735b89 100644
--- a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.h
+++ b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.h
@@ -41,7 +41,8 @@ protected:
 
 protected:
     uint32_t getTargetLid(uint32_t lid) const {
-        return _targetLids[lid];
+        // Check range to avoid reading memory beyond end of mapping array
+        return lid < _targetLids.size() ? _targetLids[lid] : 0u;
     }
 
 public:
diff --git a/searchlib/src/vespa/searchlib/attribute/imported_search_context.h b/searchlib/src/vespa/searchlib/attribute/imported_search_context.h
index 4c3b6a89a14..caf9b2f3c11 100644
--- a/searchlib/src/vespa/searchlib/attribute/imported_search_context.h
+++ b/searchlib/src/vespa/searchlib/attribute/imported_search_context.h
@@ -40,7 +40,8 @@ class ImportedSearchContext : public ISearchContext {
     SearchContextParams                             _params;
 
     uint32_t getTargetLid(uint32_t lid) const {
-        return _targetLids[lid];
+        // Check range to avoid reading memory beyond end of mapping array
+        return lid < _targetLids.size() ? _targetLids[lid] : 0u;
     }
 
     void makeMergedPostings(bool isFilter);
diff --git a/searchlib/src/vespa/searchlib/attribute/reference_mappings.h b/searchlib/src/vespa/searchlib/attribute/reference_mappings.h
index 9923747577f..84dea276740 100644
--- a/searchlib/src/vespa/searchlib/attribute/reference_mappings.h
+++ b/searchlib/src/vespa/searchlib/attribute/reference_mappings.h
@@ -86,7 +86,10 @@ public:
         std::atomic_thread_fence(std::memory_order_acquire);
         return TargetLids(&_targetLids[0], committedDocIdLimit);
     }
-    uint32_t getTargetLid(uint32_t doc) const { return _targetLids[doc]; }
+    uint32_t getTargetLid(uint32_t doc) const {
+        // Check limit to avoid reading memory beyond end of valid mapping array
+        return doc < _committedDocIdLimit ? _targetLids[doc] : 0u;
+    }
     ReverseMappingRefs getReverseMappingRefs() const {
         uint32_t targetLidLimit = _targetLidLimit;
         std::atomic_thread_fence(std::memory_order_acquire);