fix for "malfunction when reading CZIs with empty attachment-director…

…y via curl_http_inputstream" (#99) * clang-tidy fixes * cosmetic * fix * the pull request link for this version in the version history has been updated. * update codecov * When we run choco install we should pass --no-progress to keep output trimmed down. * linter * Update Src/libCZI/CziParse.cpp Co-authored-by: m-aXimilian <56168660+m-aXimilian@users.noreply.github.com> --------- Co-authored-by: m-aXimilian <56168660+m-aXimilian@users.noreply.github.com>
ZEISS · May 23, 2024 · 0c4f6c7 · 0c4f6c7
1 parent 2e37398
commit 0c4f6c7
Show file tree

Hide file tree

Showing 8 changed files with 95 additions and 42 deletions.
diff --git a/.github/workflows/cmake.yml b/.github/workflows/cmake.yml
@@ -73,7 +73,7 @@ jobs:
       - name: Prepare Coverage
         if: ${{ (matrix.OS == 'windows-latest') && ( matrix.build == 'Debug') }}
         run: |
-          choco install OpenCppCoverage -y
+          choco install OpenCppCoverage -y --no-progress
           echo "C:\Program Files\OpenCppCoverage" >> "$env:GITHUB_PATH"
 
       - name: Get Coverage
@@ -83,7 +83,7 @@ jobs:
         run: OpenCppCoverage.exe --export_type cobertura:${{github.workspace}}\coverage.xml --config_file "${{github.workspace}}\opencppcoverage.txt" -- libCZI_UnitTests.exe
 
       - name: Upload Coverage
-        uses: codecov/codecov-action@v3
+        uses: codecov/codecov-action@v4
         if: ${{ (matrix.OS == 'windows-latest') && ( matrix.build == 'Debug') }}
         with:
           files: ./coverage.xml
@@ -92,3 +92,4 @@ jobs:
           # Only one flag to be safe with
           # https://docs.codecov.com/docs/flags#one-to-one-relationship-of-flags-to-uploads
           flags: ${{matrix.OS}}
+          token: e9a842e5-9f84-48a6-9320-8f67a28f0260 #gitleaks:allow
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -3,7 +3,7 @@ cmake_minimum_required(VERSION 3.15)
 cmake_policy(SET CMP0091 NEW) # enable new "MSVC runtime library selection" (https://cmake.org/cmake/help/latest/variable/CMAKE_MSVC_RUNTIME_LIBRARY.html)
 
 project(libCZI 
-      VERSION 0.58.3
+      VERSION 0.58.4
       HOMEPAGE_URL "https://github.com/ZEISS/libczi"
       DESCRIPTION "libCZI is an Open Source Cross-Platform C++ library to read and write CZI")
 

diff --git a/Src/libCZI/CziParse.cpp b/Src/libCZI/CziParse.cpp
@@ -171,7 +171,7 @@ using namespace libCZI;
 /*static*/void CCZIParse::ReadAttachmentsDirectory(libCZI::IStream* str, std::uint64_t offset, const std::function<void(const CCziAttachmentsDirectoryBase::AttachmentEntry&)>& addFunc, SegmentSizes* segmentSizes/*= nullptr*/)
 {
     AttachmentDirectorySegment attachmentDirSegment;
-    std::uint64_t bytesRead;
+    uint64_t bytesRead;
     try
     {
         str->Read(offset, &attachmentDirSegment, sizeof(attachmentDirSegment), &bytesRead);
@@ -201,45 +201,49 @@ using namespace libCZI;
 
     // TODO: we can add a couple of consistency checks here
 
-    // now read the AttachmentEntries
-    std::uint64_t attachmentEntriesSize = ((std::uint64_t)attachmentDirSegment.data.EntryCount) * sizeof(AttachmentEntryA1);
-
-    std::unique_ptr<AttachmentEntryA1, decltype(free)*> pBuffer((AttachmentEntryA1*)malloc((size_t)attachmentEntriesSize), free);
-
-    // now read the attachment-entries
-    try
-    {
-        str->Read(offset + sizeof(attachmentDirSegment), pBuffer.get(), attachmentEntriesSize, &bytesRead);
-    }
-    catch (const std::exception&)
-    {
-        std::throw_with_nested(LibCZIIOException("Error reading FileHeaderSegment", offset + sizeof(attachmentDirSegment), attachmentEntriesSize));
-    }
-
-    if (bytesRead != attachmentEntriesSize)
+    // an empty attachment-directory (with zero entries) is valid and in this
+    //  case we can skip the rest of the processing
+    if (attachmentDirSegment.data.EntryCount > 0)
     {
-        CCZIParse::ThrowNotEnoughDataRead(offset + sizeof(attachmentDirSegment), attachmentEntriesSize, bytesRead);
-    }
+        // now read the AttachmentEntries
+        const uint64_t attachmentEntriesSize = static_cast<uint64_t>(attachmentDirSegment.data.EntryCount) * sizeof(AttachmentEntryA1);
+        unique_ptr<AttachmentEntryA1, decltype(free)*> pBuffer(static_cast<AttachmentEntryA1*>(malloc((size_t)attachmentEntriesSize)), free);
 
-    for (int i = 0; i < attachmentDirSegment.data.EntryCount; ++i)
-    {
-        ConvertToHostByteOrder::Convert(pBuffer.get() + i);
+        // now read the attachment-entries
+        try
+        {
+            str->Read(offset + sizeof(attachmentDirSegment), pBuffer.get(), attachmentEntriesSize, &bytesRead);
+        }
+        catch (const std::exception&)
+        {
+            std::throw_with_nested(LibCZIIOException("Error reading FileHeaderSegment", offset + sizeof(attachmentDirSegment), attachmentEntriesSize));
+        }
 
-        const AttachmentEntryA1* pSrc = pBuffer.get() + i;
-        CCziAttachmentsDirectoryBase::AttachmentEntry ae;
-        bool b = CheckAttachmentSchemaType((const char*)&pSrc->SchemaType[0], 2);
-        if (b == false)
+        if (bytesRead != attachmentEntriesSize)
         {
-            // TODO: what do we do if we encounter this...?
-            continue;
+            CCZIParse::ThrowNotEnoughDataRead(offset + sizeof(attachmentDirSegment), attachmentEntriesSize, bytesRead);
         }
 
-        ae.FilePosition = pSrc->FilePosition;
-        ae.ContentGuid = pSrc->ContentGuid;
-        memcpy(&ae.ContentFileType[0], &pSrc->ContentFileType[0], sizeof(AttachmentEntryA1::ContentFileType));
-        memcpy(&ae.Name[0], &pSrc->Name[0], sizeof(AttachmentEntryA1::Name));
-        ae.Name[sizeof(AttachmentEntryA1::Name) - 1] = '\0';
-        addFunc(ae);
+        for (int i = 0; i < attachmentDirSegment.data.EntryCount; ++i)
+        {
+            ConvertToHostByteOrder::Convert(pBuffer.get() + i);
+
+            const AttachmentEntryA1* pSrc = pBuffer.get() + i;
+            CCziAttachmentsDirectoryBase::AttachmentEntry ae;
+            bool b = CheckAttachmentSchemaType(reinterpret_cast<const char*>(&pSrc->SchemaType[0]), 2);
+            if (b == false)
+            {
+                // TODO: what do we do if we encounter this...?
+                continue;
+            }
+
+            ae.FilePosition = pSrc->FilePosition;
+            ae.ContentGuid = pSrc->ContentGuid;
+            memcpy(&ae.ContentFileType[0], &pSrc->ContentFileType[0], sizeof(AttachmentEntryA1::ContentFileType));
+            memcpy(&ae.Name[0], &pSrc->Name[0], sizeof(AttachmentEntryA1::Name));
+            ae.Name[sizeof(AttachmentEntryA1::Name) - 1] = '\0';
+            addFunc(ae);
+        }
     }
 }
 

diff --git a/Src/libCZI/Doc/version-history.markdown b/Src/libCZI/Doc/version-history.markdown
@@ -18,5 +18,6 @@ version history                 {#version_history}
  0.57.3             | [91](https://github.com/ZEISS/libczi/pull/91)        | improve error-message
  0.58.0             | [92](https://github.com/ZEISS/libczi/pull/92)        | export a list with properties for streams-property-bag
  0.58.1             | [95](https://github.com/ZEISS/libczi/pull/95)        | some fixes for CziReaderWriter
- 0.58.2             | [95](https://github.com/ZEISS/libczi/pull/96)        | small fixes for deficiencies reported by CodeQL
- 0.58.3             | [96](https://github.com/ZEISS/libczi/pull/97)        | update zstd to [version 1.5.6](https://github.com/facebook/zstd/releases/tag/v1.5.6)
+ 0.58.2             | [96](https://github.com/ZEISS/libczi/pull/96)        | small fixes for deficiencies reported by CodeQL
+ 0.58.3             | [97](https://github.com/ZEISS/libczi/pull/97)        | update zstd to [version 1.5.6](https://github.com/facebook/zstd/releases/tag/v1.5.6)
+ 0.58.4             | [99](https://github.com/ZEISS/libczi/pull/99)        | fix a rare issue with curl_http_inputstream which would fail to read CZIs with an attachment-directory containing zero entries
diff --git a/Src/libCZI/StreamsLib/curlhttpinputstream.cpp b/Src/libCZI/StreamsLib/curlhttpinputstream.cpp
@@ -121,6 +121,8 @@ CurlHttpInputStream::CurlHttpInputStream(const std::string& url, const std::map<
     CURLcode return_code = curl_easy_setopt(up_curl_handle.get(), CURLOPT_CURLU, up_curl_url_handle.get());
     ThrowIfCurlSetOptError(return_code, "CURLOPT_CURLU");
 
+    // for debugging purposes, set verbose mode to 1, and libcurl will output information about the operation
+    //  to stdout
     return_code = curl_easy_setopt(up_curl_handle.get(), CURLOPT_VERBOSE, 0L/*1L*/);
     ThrowIfCurlSetOptError(return_code, "CURLOPT_VERBOSE");
 
@@ -233,6 +235,17 @@ CurlHttpInputStream::CurlHttpInputStream(const std::string& url, const std::map<
 
 /*virtual*/void CurlHttpInputStream::Read(std::uint64_t offset, void* pv, std::uint64_t size, std::uint64_t* ptrBytesRead)
 {
+    // we handle the case of size == 0 explicitly, because the "curl_easy_perform" would not work with a size of 0
+    if (size == 0)
+    {
+        if (ptrBytesRead != nullptr)
+        {
+            *ptrBytesRead = 0;
+        }
+
+        return;
+    }
+
     stringstream ss;
     ss << offset << "-" << offset + size - 1;
 

diff --git a/Src/libCZI/libCZI.h b/Src/libCZI/libCZI.h
@@ -196,6 +196,9 @@ namespace libCZI
         /// is expected to throw an exception for any kind of I/O-related error. It must not throw
         /// an exception if reading past the end of a file - instead, it must return the number of
         /// bytes actually read accordingly.
+        /// For the special case of size==0, the behavior should be as follows: the method should
+        /// operate as for a size>0, but it should not read any data. The method should return 0 in
+        /// ptrBytesRead.
         ///
         /// \param offset                The offset to start reading from.
         /// \param [out] pv              The caller-provided buffer for the data. Must be non-null.

diff --git a/Src/libCZI/libCZI_StreamsLib.h b/Src/libCZI/libCZI_StreamsLib.h
@@ -272,7 +272,7 @@ namespace libCZI
         /// initializing the stream, an exception will be thrown.
         ///
         /// \param  stream_info     Information describing the stream.
-        /// \param  file_identifier The filename (or, more generally, an URI of some sort) identifying the file to be opened in UTF8-encoding.
+        /// \param  file_identifier The filename (or, more generally, a URI of some sort) identifying the file to be opened in UTF8-encoding.
         ///
         /// \returns    The newly created and initialized stream.
         static std::shared_ptr<libCZI::IStream> CreateStream(const CreateStreamInfo& stream_info, const std::string& file_identifier);
@@ -282,7 +282,7 @@ namespace libCZI
         /// initializing the stream, an exception will be thrown.
         ///
         /// \param  stream_info     Information describing the stream.
-        /// \param  file_identifier The filename (or, more generally, an URI of some sort) identifying the file to be opened.
+        /// \param  file_identifier The filename (or, more generally, a URI of some sort) identifying the file to be opened.
         ///
         /// \returns    The newly created and initialized stream.
         static std::shared_ptr<libCZI::IStream> CreateStream(const CreateStreamInfo& stream_info, const std::wstring& file_identifier);

diff --git a/Src/libCZI_UnitTests/test_curlhttpstream.cpp b/Src/libCZI_UnitTests/test_curlhttpstream.cpp
@@ -44,7 +44,7 @@ TEST(CurlHttpInputStream, SimpleReadFromHttps)
     EXPECT_TRUE(memcmp(hash, expectedResult, 16) == 0) << "Incorrect result";
 }
 
-TEST(CurlHttpInputStream, OpenAndReadCziFromUrl)
+TEST(CurlHttpInputStream, TryToReadZeroBytesFromHttpsAndExpectSuccess)
 {
     static constexpr char kUrl[] = "https://media.githubusercontent.com/media/ptahmose/libCZI_testdata/main/MD5/ff20e3a15d797509f7bf494ea21109d3";  // sparse_planes.czi.
 
@@ -61,6 +61,37 @@ TEST(CurlHttpInputStream, OpenAndReadCziFromUrl)
         GTEST_SKIP() << "The stream-class 'curl_http_inputstream' is not available/configured, skipping this test therefore.";
     }
 
+    uint8_t buffer[1];
+    uint64_t bytes_read = (std::numeric_limits<uint64_t>::max)();
+    try
+    {
+        stream->Read(0, buffer, 0, &bytes_read);
+    }
+    catch (const std::exception& e)
+    {
+        GTEST_SKIP() << "Exception: " << e.what() << "--> skipping this test as inconclusive, assuming network issues";
+    }
+
+    EXPECT_EQ(bytes_read, 0);
+}
+
+TEST(CurlHttpInputStream, OpenAndReadCziFromUrl)
+{
+    static constexpr char kUrl[] = "https://media.githubusercontent.com/media/ptahmose/libCZI_testdata/main/MD5/ff20e3a15d797509f7bf494ea21109d3";  // sparse_planes.czi
+
+    StreamsFactory::CreateStreamInfo create_info;
+    create_info.class_name = "curl_http_inputstream";
+
+    // set a five-seconds time-out (for the whole operation)
+    create_info.property_bag = { {StreamsFactory::StreamProperties::kCurlHttp_Timeout,StreamsFactory::Property(5)} };
+
+    const auto stream = StreamsFactory::CreateStream(create_info, kUrl);
+
+    if (!stream)
+    {
+        GTEST_SKIP() << "The stream-class 'curl_http_inputstream' is not available/configured, skipping this test therefore.";
+    }
+
     const auto czi_reader = CreateCZIReader();
     try
     {