[#4664] Fix VersionedCSVFile ignoring CSVFile::next failures

Propagate false from CSVFile::next so I/O/stream errors are not
misreported as success against stale row data.

Closes #4664

Co-authored-by: Cursor <cursoragent@cursor.com>

diff --git a/changelog_unreleased/4664-versionedcsvfile-next-ignores-csvfile-next-i-o-failure b/changelog_unreleased/4664-versionedcsvfile-next-ignores-csvfile-next-i-o-failure
new file mode 100644 (file)
index 0000000..3fab048
--- /dev/null
+++ b/changelog_unreleased/4664-versionedcsvfile-next-ignores-csvfile-next-i-o-failure
@@ -0,0 +1,4 @@
+[bug]          wlodek
+       VersionedCSVFile::next now returns false when the
+       underlying CSVFile::next read fails.
+       (Gitlab #4664)

diff --git a/src/lib/util/tests/versioned_csv_file_unittest.cc b/src/lib/util/tests/versioned_csv_file_unittest.cc
index 31916c00f556027219777c7c8dae5b2b7d5e6f30..13ce44475d33931fef4c70aa6ccfca9ce5408bd1 100644 (file)
--- a/src/lib/util/tests/versioned_csv_file_unittest.cc
+++ b/src/lib/util/tests/versioned_csv_file_unittest.cc
@@ -498,4 +498,39 @@ TEST_F(VersionedCSVFileTest, rowChecking) {
     EXPECT_FALSE(csv->next(row));
 }
 
+// Verifies that VersionedCSVFile::next() propagates CSVFile::next()
+// failures. Closing the stream after a successful read leaves the
+// previous row intact while the base class returns false; the
+// versioned wrapper must not treat that stale row as a new success.
+// See Gitlab #4664.
+TEST_F(VersionedCSVFileTest, nextPropagatesReadFailure) {
+    writeFile("animal,color,age\n"
+              "cat,black,2\n"
+              "lion,yellow,17\n");
+
+    boost::scoped_ptr<VersionedCSVFile> csv(new VersionedCSVFile(testfile_));
+    ASSERT_NO_THROW(csv->addColumn("animal", "2.0", ""));
+    ASSERT_NO_THROW(csv->addColumn("color", "2.0", "grey"));
+    ASSERT_NO_THROW(csv->addColumn("age", "2.0", "0"));
+    ASSERT_NO_THROW(csv->open());
+
+    CSVRow row;
+    ASSERT_TRUE(csv->next(row));
+    EXPECT_EQ("cat", row.readAt(0));
+    EXPECT_EQ("black", row.readAt(1));
+    EXPECT_EQ("2", row.readAt(2));
+
+    // Simulate a subsequent read failure: stream is unusable, but
+    // 'row' still holds the previously read valid contents.
+    ASSERT_NO_THROW(csv->close());
+    EXPECT_FALSE(csv->next(row));
+    EXPECT_NE(std::string::npos, csv->getReadMsg().find("NULL stream"));
+
+    // Stale row contents must remain unchanged (base class does not
+    // clear the row on failure).
+    EXPECT_EQ("cat", row.readAt(0));
+    EXPECT_EQ("black", row.readAt(1));
+    EXPECT_EQ("2", row.readAt(2));
+}
+
 } // end of anonymous namespace

diff --git a/src/lib/util/versioned_csv_file.cc b/src/lib/util/versioned_csv_file.cc
index 8c48f66e10d6aa633badaac8b9b8e6ea20e2ed42..099ffe0af93983fc3903ae7604283db2c01acb5d 100644 (file)
--- a/src/lib/util/versioned_csv_file.cc
+++ b/src/lib/util/versioned_csv_file.cc
@@ -123,9 +123,13 @@ VersionedCSVFile::getVersionedColumn(const size_t index) const {
 bool
 VersionedCSVFile::next(CSVRow& row) {
     setReadMsg("success");
-    // Use base class to physical read the row, but skip its row
-    // validation
-    CSVFile::next(row, true);
+    // Use base class to physically read the row, but skip its row
+    // validation. Propagate I/O and stream failures: on error the
+    // base class leaves 'row' unchanged, so continuing would validate
+    // stale contents and incorrectly report success.
+    if (!CSVFile::next(row, true)) {
+        return (false);
+    }
     if (row == CSVFile::EMPTY_ROW()) {
         return(true);
     }

diff --git a/src/lib/util/versioned_csv_file.h b/src/lib/util/versioned_csv_file.h
index b5bb821d3140ac66fbd4492a8e0d8bde3ca5dc26..c64c4feeb6c3cd511075b5c699b7a6a7f03f1a4a 100644 (file)
--- a/src/lib/util/versioned_csv_file.h
+++ b/src/lib/util/versioned_csv_file.h
@@ -1,4 +1,4 @@
-// Copyright (C) 2015-2024 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2015-2026 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -225,8 +225,9 @@ public:
     ///
     /// @param [out] row Object receiving the parsed CSV file.
     ///
-    /// @return true if row has been read and validated; false if validation
-    /// failed.
+    /// @return true if row has been read and validated; false if the
+    /// underlying read failed or validation failed. On read failure the
+    /// contents of @c row are left unchanged.
     bool next(CSVRow& row);
 
     /// @brief Returns the schema version of the physical file