Merge pull request #25720 from vespa-engine/vekterli/improve-doc-deserialization

Improve handling of corrupted serialized document data

1 files changed, 21 insertions, 16 deletions

diff --git a/document/src/vespa/document/serialization/vespadocumentdeserializer.cpp b/document/src/vespa/document/serialization/vespadocumentdeserializer.cpp
index c8074b27549..829cf9d306c 100644
--- a/document/src/vespa/document/serialization/vespadocumentdeserializer.cpp
+++ b/document/src/vespa/document/serialization/vespadocumentdeserializer.cpp
@@ -294,16 +294,20 @@ namespace {
 
 using FieldInfo = SerializableArray::EntryMap;
 
-void readFieldInfo(nbostream& input, SerializableArray::EntryMap & field_info) __attribute__((noinline));
+void readFieldInfo(nbostream& input, SerializableArray::EntryMap & field_info, size_t max_buffer_extent) __attribute__((noinline));
 
 void
-readFieldInfo(nbostream& input, SerializableArray::EntryMap & field_info) {
+readFieldInfo(nbostream& input, SerializableArray::EntryMap & field_info, size_t max_buffer_extent) {
     size_t field_count = getInt1_4Bytes(input);
     field_info.reserve(field_count);
     uint32_t offset = 0;
     for (size_t i = 0; i < field_count; ++i) {
         const uint32_t id = getInt1_4Bytes(input);
         const uint32_t size = getInt2_4_8Bytes(input);
+        if (size_t(offset) + size > max_buffer_extent) [[unlikely]] {
+            throw DeserializeException(fmt("Field (id=%u, offset=%u, size=%u) extends beyond max buffer extent (%zu)",
+                                           id, offset, size, max_buffer_extent), VESPA_STRLOC);
+        }
         field_info.emplace_back(id, size, offset);
         offset += size;
     }
@@ -344,25 +348,24 @@ void
 VespaDocumentDeserializer::readStructNoReset(StructFieldValue &value) {
     size_t data_size = readValue<uint32_t>(_stream);
 
-    CompressionConfig::Type compression_type = CompressionConfig::Type(readValue<uint8_t>(_stream));
+    const auto compression_type = CompressionConfig::Type(readValue<uint8_t>(_stream));
+    const bool is_compressed    = CompressionConfig::isCompressed(compression_type);
 
-    SerializableArray::EntryMap  field_info;
+    SerializableArray::EntryMap field_info;
     size_t uncompressed_size = 0;
-    if (CompressionConfig::isCompressed(compression_type)) {
+    if (is_compressed) {
         uncompressed_size = getInt2_4_8Bytes(_stream);
     }
-    readFieldInfo(_stream, field_info);
-
-    if (CompressionConfig::isCompressed(compression_type)) {
-        if ((compression_type != CompressionConfig::LZ4)) {
-            throw DeserializeException("Unsupported compression type.", VESPA_STRLOC);
-        } else if (data_size > _stream.size()) {
-            throw DeserializeException("Invalid compressed struct data.", VESPA_STRLOC);
-        }
+    if (is_compressed && (compression_type != CompressionConfig::LZ4)) [[unlikely]] {
+        throw DeserializeException(fmt("Unsupported compression type: %u", static_cast<uint8_t>(compression_type)), VESPA_STRLOC);
     }
-
+    if (data_size > _stream.size()) [[unlikely]] {
+        throw DeserializeException(fmt("Struct size (%zu) is greater than remaining buffer size (%zu)",
+                                       data_size, _stream.size()), VESPA_STRLOC);
+    }
+    readFieldInfo(_stream, field_info, is_compressed ? uncompressed_size : data_size);
     if (data_size > 0) {
-        ByteBuffer buffer = CompressionConfig::isCompressed(compression_type)
+        ByteBuffer buffer = is_compressed
                             ? deCompress(compression_type, uncompressed_size, ConstBufferRef(_stream.peek(), data_size))
                             : (_stream.isLongLivedBuffer()
                                 ? ByteBuffer(_stream.peek(), data_size)
@@ -377,7 +380,9 @@ VespaDocumentDeserializer::readStructNoReset(StructFieldValue &value) {
             for (const auto & entry : tmp) {
                 try {
                     FieldValue::UP decoded = tmp.getValue(entry);
-                    value.setValue(entry, std::move(decoded));
+                    if (decoded) {
+                        value.setValue(entry, std::move(decoded));
+                    }
                 } catch (const vespalib::Exception & e) {
                     LOG(warning, "Failed decoding field '%s' in legacy bodyfield -> Skipping it: %s",
                                  entry.getName().data(), e.what());