AI review: detect corruption in Free()/PopFront()

Author: Andrei Nasonov

cloud/storage/core/libs/file_backed_containers/file_ring_buffer.cpp

@@ -521,16 +521,16 @@ class TFileRingBuffer::TImpl
 
     TResultOrError<char*> Alloc(size_t size)
     {
+        if (!ValidateAccess("Alloc")) {
+            return MakeBufferIsCorruptError();
+        }
+
         if (CurrentAllocation.HasValue()) {
             return MakeError(
                 E_INVALID_STATE,
                 "Previous allocation is not committed");
         }
 
-        if (!ValidateAccess("Alloc")) {
-            return MakeBufferIsCorruptError();
-        }
-
         if (size == 0) {
             return MakeError(
                 E_ARGUMENT,
@@ -626,7 +626,12 @@ class TFileRingBuffer::TImpl
             CurrentAllocation.ActualPos,
             CurrentAllocation.Header);
 
-        Y_ABORT_UNLESS(written);
+        if (!written) {
+            SetCorrupted(
+                TStringBuilder() << "Cannot write entry header at "
+                                 << CurrentAllocation.ActualPos);
+            return MakeBufferIsCorruptError();
+        }
 
         // A compiler-only fence is sufficient here because there is no
         // concurrent access to the memory and we just need to ensure
@@ -657,12 +662,25 @@ class TFileRingBuffer::TImpl
         auto eh = Data()->ReadEntryHeader(it->second);
         eh.DataChecksum = 0;
         eh.FreeFlag = true;
-        Data()->WriteEntryHeader(it->second, eh);
+
+        bool written = Data()->WriteEntryHeader(it->second, eh);
+
+        if (!written) {
+            SetCorrupted(
+                TStringBuilder() << "Cannot write entry header at "
+                                 << CurrentAllocation.ActualPos);
+            return MakeBufferIsCorruptError();
+        }
 
         EntryMap.erase(it);
 
         EraseFreeEntriesFromFront();
 
+        if (IsCorrupted()) {
+            // EraseFreeEntriesFromFront() may set IsCorrupted flag
+            return MakeBufferIsCorruptError();
+        }
+
         return {};
     }
 

cloud/storage/core/libs/file_backed_containers/file_ring_buffer_ut.cpp

@@ -663,6 +663,37 @@ Y_UNIT_TEST_SUITE(TFileRingBufferTest)
         UNIT_ASSERT_STRINGS_EQUAL(dump, s.Dump());
     }
 
+    FILE_RING_BUFFER_TEST(ShouldDetectCorruptionOnPopFront)
+    {
+        const auto f = TTempFileHandle();
+        const ui32 len = 42;
+        TFileRingBuffer rb(f.GetName(), len, 0, ver);
+
+        UNIT_ASSERT_VALUES_EQUAL(true, rb.PushBack("abc"));
+        UNIT_ASSERT_VALUES_EQUAL(true, rb.PushBack("def"));
+
+        {
+            // Corrupt the buffer
+            TFileMapFileRingBufferAccessor accessor(
+                f.GetName(),
+                EFileRingBufferAccessorValidationMode::Normal,
+                TMemoryMapCommon::EOpenModeFlag::oRdWr);
+
+            UNIT_ASSERT(!HasError(accessor.Map()));
+
+            UNIT_ASSERT_VALUES_EQUAL(
+                EFileRingBufferAccessorValidationStatus::Success,
+                accessor.ValidateAndInitialize());
+
+            auto eh = accessor.GetDataProcessor()->ReadEntryHeader(0);
+            UNIT_ASSERT_VALUES_EQUAL(3, eh.DataSize);
+            eh.DataSize = 1000;
+            accessor.GetDataProcessor()->WriteEntryHeader(0, eh);
+        }
+
+        UNIT_ASSERT(HasError(rb.PopFront()));
+    }
+
     FILE_RING_BUFFER_TEST(ShouldGetRawCapacity)
     {
         const auto f = TTempFileHandle();