From aa90caa3894fcf1ff3122a071c69b60a988c9bc6 Mon Sep 17 00:00:00 2001
From: Andy Maloney <asmaloney@gmail.com>
Date: Sun, 22 Oct 2023 11:03:16 -0400
Subject: [PATCH] Give better error information when trying to read an invalid
 Data3D with zero points

Instead of throwing ErrorInternal which is a bit misleading, throw a new exception ErrorData3DReadInvalidZeroRecords.

It looks like this:

trying to read an invalid Data3D with zero records - check for zero records before trying to read this Data3D section (ErrorInvalidZeroRecordsData3D): fileOffset cannot be 0; cvPathName=/data3D/0/points imageFileName=ZeroPointsInvalid.e57

Part of #262
---
 include/E57Exception.h             |  4 ++++
 src/CompressedVectorReaderImpl.cpp | 20 ++++++++++++++-----
 src/E57Exception.cpp               |  7 +++++--
 test/src/test_SimpleReader.cpp     | 32 ++++++++++++++++++++++++++++++
 4 files changed, 56 insertions(+), 7 deletions(-)

diff --git a/include/E57Exception.h b/include/E57Exception.h
index 9ce6d6d..138c276 100644
--- a/include/E57Exception.h
+++ b/include/E57Exception.h
@@ -138,6 +138,10 @@ namespace e57
       /// passed an invalid value in Data3D pointFields
       ErrorInvalidData3DValue = 52,
 
+      /// Older versions of this library (and E57RefImpl) incorrectly set the "fileOffset" to 0
+      /// when "recordCount" is 0. "fileOffset" must be greater than 0 (Table 9 in the standard).
+      ErrorData3DReadInvalidZeroRecords = 53,
+
       /// @deprecated Will be removed in 4.0. Use e57::Success.
       E57_SUCCESS DEPRECATED_ENUM( "Will be removed in 4.0. Use Success." ) = Success,
       /// @deprecated Will be removed in 4.0. Use e57::ErrorBadCVHeader.
diff --git a/src/CompressedVectorReaderImpl.cpp b/src/CompressedVectorReaderImpl.cpp
index ff3ac52..7f79e6c 100644
--- a/src/CompressedVectorReaderImpl.cpp
+++ b/src/CompressedVectorReaderImpl.cpp
@@ -100,17 +100,27 @@ namespace e57
       //??? what if fault in this constructor?
       cache_ = new PacketReadCache( imf->file_, 32 );
 
-      // Read CompressedVector section header
-      CompressedVectorSectionHeader sectionHeader;
+      // Check the file offset of this vector - it must be positive
       uint64_t sectionLogicalStart = cVector_->getBinarySectionLogicalStart();
       if ( sectionLogicalStart == 0 )
       {
-         //??? should have caught this before got here, in XML read, get this if CV
-         // wasn't written to
-         // by writer.
+         // Older versions of this library (and E57RefImpl) incorrectly set the "fileOffset" to 0
+         // when "recordCount" is 0. "fileOffset" must be greater than 0 (Table 9 in the standard).
+         if ( maxRecordCount_ == 0 )
+         {
+            throw E57_EXCEPTION2( ErrorData3DReadInvalidZeroRecords,
+                                  "fileOffset cannot be 0; cvPathName=" + cVector_->pathName() +
+                                     " imageFileName=" + cVector_->imageFileName() );
+         }
+
+         //??? should have caught this before got here, in XML read, get this if CV wasn't written
+         // to by writer.
          throw E57_EXCEPTION2( ErrorInternal, "imageFileName=" + cVector_->imageFileName() +
                                                  " cvPathName=" + cVector_->pathName() );
       }
+
+      // Read CompressedVector section header
+      CompressedVectorSectionHeader sectionHeader;
       imf->file_->seek( sectionLogicalStart, CheckedFile::Logical );
       imf->file_->read( reinterpret_cast<char *>( &sectionHeader ), sizeof( sectionHeader ) );
 
diff --git a/src/E57Exception.cpp b/src/E57Exception.cpp
index 137dcbb..ef334cd 100644
--- a/src/E57Exception.cpp
+++ b/src/E57Exception.cpp
@@ -355,9 +355,12 @@ namespace e57
          case ErrorInvarianceViolation:
             return "class invariance constraint violation in debug mode (ErrorInvarianceViolation)";
          case ErrorInvalidNodeType:
-            return "an invalid node type was passed in Data3D pointFields";
+            return "an invalid node type was passed in Data3D pointFields (ErrorInvalidNodeType)";
          case ErrorInvalidData3DValue:
-            return "an invalid value was passed in Data3D pointFields";
+            return "an invalid value was passed in Data3D pointFields (ErrorInvalidData3DValue)";
+         case ErrorData3DReadInvalidZeroRecords:
+            return "trying to read an invalid Data3D with zero records - check for zero records "
+                   "before trying to read this Data3D section (ErrorInvalidZeroRecordsData3D)";
 
          default:
             return "unknown error (" + std::to_string( ecode ) + ")";
diff --git a/test/src/test_SimpleReader.cpp b/test/src/test_SimpleReader.cpp
index 5a37189..61eb210 100644
--- a/test/src/test_SimpleReader.cpp
+++ b/test/src/test_SimpleReader.cpp
@@ -45,6 +45,38 @@ TEST( SimpleReaderData, Empty )
    delete reader;
 }
 
+TEST( SimpleReaderData, ZeroPointsInvalid )
+{
+   e57::Reader *reader = nullptr;
+
+   E57_ASSERT_NO_THROW(
+      reader = new e57::Reader( TestData::Path() + "/self/ZeroPointsInvalid.e57", {} ) );
+
+   ASSERT_TRUE( reader->IsOpen() );
+   EXPECT_EQ( reader->GetImage2DCount(), 0 );
+   EXPECT_EQ( reader->GetData3DCount(), 1 );
+
+   e57::E57Root fileHeader;
+   ASSERT_TRUE( reader->GetE57Root( fileHeader ) );
+
+   CheckFileHeader( fileHeader );
+   EXPECT_EQ( fileHeader.guid, "{EC1A0DE4-F76F-44CE-E527-789EEB818347}" );
+
+   e57::Data3D data3DHeader;
+   ASSERT_TRUE( reader->ReadData3D( 0, data3DHeader ) );
+
+   ASSERT_EQ( data3DHeader.pointCount, 0 );
+
+   const uint64_t cNumPoints = data3DHeader.pointCount;
+
+   e57::Data3DPointsFloat pointsData( data3DHeader );
+
+   E57_ASSERT_THROW( auto vectorReader =
+                        reader->SetUpData3DPointsData( 0, cNumPoints, pointsData ); );
+
+   delete reader;
+}
+
 TEST( SimpleReaderData, BadCRC )
 {
    E57_ASSERT_THROW( e57::Reader( TestData::Path() + "/self/bad-crc.e57", {} ) );