Merge pull request #30294 from vespa-engine/toregge/check-buffer-size-when-loading-predicate-attribute

Check buffer size when loading predicate attribute.

2 files changed, 68 insertions, 12 deletions

diff --git a/searchlib/src/tests/attribute/predicate_attribute/predicate_attribute_test.cpp b/searchlib/src/tests/attribute/predicate_attribute/predicate_attribute_test.cpp
index a3466be5e0e..a0d69f0ff85 100644
--- a/searchlib/src/tests/attribute/predicate_attribute/predicate_attribute_test.cpp
+++ b/searchlib/src/tests/attribute/predicate_attribute/predicate_attribute_test.cpp
@@ -2,11 +2,16 @@
 
 #include <vespa/document/fieldvalue/predicatefieldvalue.h>
 #include <vespa/document/predicate/predicate_slime_builder.h>
+#include <vespa/fastos/file.h>
 #include <vespa/searchcommon/attribute/config.h>
 #include <vespa/searchlib/attribute/attributefactory.h>
 #include <vespa/searchlib/attribute/attributevector.h>
 #include <vespa/searchlib/attribute/predicate_attribute.h>
+#include <vespa/searchlib/common/fileheadertags.h>
+#include <vespa/searchlib/util/file_settings.h>
+#include <vespa/vespalib/data/fileheader.h>
 #include <vespa/vespalib/gtest/gtest.h>
+#include <vespa/vespalib/util/exceptions.h>
 #include <filesystem>
 #include <memory>
 #include <sstream>
@@ -16,9 +21,13 @@ using document::PredicateSlimeBuilder;
 using document::PredicateFieldValue;
 using search::AttributeFactory;
 using search::AttributeVector;
+using search::FileSettings;
 using search::PredicateAttribute;
 using search::attribute::Config;
 using search::attribute::BasicType;
+using search::tags::FILE_BIT_SIZE;
+using vespalib::IllegalStateException;
+using Tag = vespalib::GenericHeader::Tag;
 
 namespace {
 
@@ -62,6 +71,40 @@ fv_as_string(const FieldValue& val)
     return os.str();
 }
 
+std::shared_ptr<AttributeVector>
+make_sample_predicate_attribute()
+{
+    auto cfg = get_predicate_with_arity(2);
+    auto attr = make_attribute(attr_name, cfg, true);
+    auto& pattr = dynamic_cast<PredicateAttribute&>(*attr);
+    PredicateSlimeBuilder builder;
+    builder.neg().feature("foo").value("bar").value("baz");
+    PredicateFieldValue val(builder.build());
+    EXPECT_EQ("'foo' not in ['bar','baz']\n", fv_as_string(val));
+    attr->addDocs(10);
+    pattr.updateValue(1,  val);
+    attr->commit();
+    EXPECT_TRUE(attr->isLoaded());
+    return attr;
+}
+
+void
+corrupt_file_header(const vespalib::string &name)
+{
+    vespalib::FileHeader h(FileSettings::DIRECTIO_ALIGNMENT);
+    FastOS_File f;
+    auto file_bit_size = 0;
+    f.OpenReadWrite(name.c_str());
+    h.readFile(f);
+    if (h.hasTag(FILE_BIT_SIZE)) {
+        file_bit_size = h.getTag(FILE_BIT_SIZE).asInteger() - 8;
+    }
+    h.putTag(Tag(FILE_BIT_SIZE, file_bit_size));
+    h.rewriteFile(f);
+    bool sync_ok = f.Sync();
+    assert(sync_ok);
+}
+
 }
 
 class PredicateAttributeTest : public ::testing::Test
@@ -91,25 +134,32 @@ PredicateAttributeTest::TearDown()
 
 TEST_F(PredicateAttributeTest, save_and_load_predicate_attribute)
 {
-    auto cfg = get_predicate_with_arity(2);
-    auto attr = make_attribute(attr_name, cfg, true);
-    auto& pattr = dynamic_cast<PredicateAttribute&>(*attr);
-    PredicateSlimeBuilder builder;
-    builder.neg().feature("foo").value("bar").value("baz");
-    PredicateFieldValue val(builder.build());
-    EXPECT_EQ("'foo' not in ['bar','baz']\n", fv_as_string(val));
-    attr->addDocs(10);
-    pattr.updateValue(1,  val);
-    attr->commit();
-    EXPECT_TRUE(attr->isLoaded());
+    auto attr = make_sample_predicate_attribute();
     std::filesystem::path file_name(tmp_dir);
     file_name.append(attr_name);
     attr->save(file_name.native());
-    auto attr2 = make_attribute(file_name.native(), cfg, false);
+    auto attr2 = make_attribute(file_name.native(), attr->getConfig(), false);
     EXPECT_FALSE(attr2->isLoaded());
     EXPECT_TRUE(attr2->load());
     EXPECT_TRUE(attr2->isLoaded());
     EXPECT_EQ(11, attr2->getCommittedDocIdLimit());
 }
 
+TEST_F(PredicateAttributeTest, buffer_size_mismatch_is_fatal_during_load)
+{
+    auto attr = make_sample_predicate_attribute();
+    std::filesystem::path file_name(tmp_dir);
+    file_name.append(attr_name);
+    attr->save(file_name.native());
+    corrupt_file_header(file_name.native() + ".dat");
+    auto attr2 = make_attribute(file_name.native(), attr->getConfig(), false);
+    EXPECT_FALSE(attr2->isLoaded());
+    try {
+        attr2->load();
+        FAIL() << "Expected exception not thrown when loading corrupt attribute";
+    } catch (IllegalStateException& e) {
+        EXPECT_EQ("Deserialize error when loading predicate attribute 'test', -1 bytes remaining in buffer", e.getMessage());
+    }
+}
+
 GTEST_MAIN_RUN_ALL_TESTS()
diff --git a/searchlib/src/vespa/searchlib/attribute/predicate_attribute.cpp b/searchlib/src/vespa/searchlib/attribute/predicate_attribute.cpp
index 1c273f17176..60898450e50 100644
--- a/searchlib/src/vespa/searchlib/attribute/predicate_attribute.cpp
+++ b/searchlib/src/vespa/searchlib/attribute/predicate_attribute.cpp
@@ -12,6 +12,7 @@
 #include <vespa/searchlib/util/fileutil.h>
 #include <vespa/searchcommon/attribute/config.h>
 #include <vespa/vespalib/data/slime/slime.h>
+#include <vespa/vespalib/util/exceptions.h>
 #include <vespa/vespalib/util/size_literals.h>
 
 #include <vespa/log/log.h>
@@ -20,6 +21,8 @@ LOG_SETUP(".searchlib.attribute.predicate_attribute");
 using document::Predicate;
 using document::PredicateFieldValue;
 using vespalib::DataBuffer;
+using vespalib::IllegalStateException;
+using vespalib::make_string;
 using namespace search::predicate;
 
 namespace search {
@@ -236,6 +239,9 @@ PredicateAttribute::onLoad(vespalib::Executor *)
         _interval_range_vector[docId] = version < 2 ? MAX_INTERVAL_RANGE : buffer.readInt16();
     }
     _max_interval_range = version < 2 ? MAX_INTERVAL_RANGE : buffer.readInt16();
+    if (buffer.getDataLen() != 0) {
+        throw IllegalStateException(make_string("Deserialize error when loading predicate attribute '%s', %" PRId64 " bytes remaining in buffer", getName().c_str(), (int64_t) buffer.getDataLen()));
+    }
     _index->adjustDocIdLimit(highest_doc_id);
     setNumDocs(highest_doc_id + 1);
     setCommittedDocIdLimit(highest_doc_id + 1);