Add a test for packaging a file larger than 4GB (#203)

The primary change is the addition of a test that packs a file larger than 4GB. This is done by creating a stream that returns easily compressed data, resulting in an ~6MB package file. The test is tagged with a hidden [.slow] tag, as it takes ~5 minutes to churn through the data and then open the resulting package for verification.

The change also reorganizes the msix in directory into categories for current and future use. The categories (and directories) are:

public: headers that we "publish" for consumption by external callers
shared: headers used by the functional tests; no linking allowed
internal: headers used by future unit tests, were linking will happen
This allows the tests to reuse product functionality, rather than re-implementing things like ComPtr.

.gitignore

@@ -397,7 +397,7 @@ __pycache__/
 # run git update-index --assume-unchanged lib/zlib/zconf.h
 lib/zlib
 # Files generated by CMake
-src/inc/MSIXResource.hpp
+src/inc/internal/MSIXResource.hpp
 src/msix/MSIXResource.cpp
 src/msix/common/MSIXResource.cpp
 

cmake/msix_resources.cmake

@@ -348,5 +348,5 @@ foreach(FILE ${RESOURCES_CERTS})
     string(APPEND CERTS_HPP result.push_back(std::make_pair(\"${FILE}\", std::move(factory->GetResource(\"${FILE}\")))) ";\n\t\t\t\t")
 endforeach()
 
-configure_file(${MSIX_PROJECT_ROOT}/src/inc/MSIXResource.hpp.cmakein ${MSIX_PROJECT_ROOT}/src/inc/MSIXResource.hpp CRLF)
+configure_file(${MSIX_PROJECT_ROOT}/src/inc/internal/MSIXResource.hpp.cmakein ${MSIX_PROJECT_ROOT}/src/inc/internal/MSIXResource.hpp CRLF)
 configure_file(${MSIX_PROJECT_ROOT}/src/msix/common/MSIXResource.cpp.cmakein ${MSIX_PROJECT_ROOT}/src/msix/common/MSIXResource.cpp CRLF)

pipelines/azure-pipelines-windows.yml

@@ -73,8 +73,8 @@ jobs:
         _arguments: x86 -d --pack
         _artifact: WIN32chk-pack
       debug_64_pack:
-        _arguments: x86 -d --pack
-        _artifact: WIN32chk-pack
+        _arguments: x64 -d --pack
+        _artifact: WIN32-x64chk-pack
 
   steps:
   - task: BatchScript@1

src/inc/internal/IXml.hpp

@@ -194,13 +194,58 @@ MSIX_INTERFACE(IXmlFactory, 0xf82a60ec,0xfbfc,0x4cb9,0xbc,0x04,0x1a,0x0f,0xe2,0x
 namespace MSIX {
     MSIX::ComPtr<IXmlFactory> CreateXmlFactory(IMsixFactory* factory);
 
+    template <typename T>
+    struct StringToNumber
+    {
+        static uint32_t Get(const std::string&)
+        {
+            static_assert(False<T>::value, "An appropriate specialization must be specified");
+        }
+    };
+
+    template <>
+    struct StringToNumber<std::uint32_t>
+    {
+        static uint32_t Get(const std::string& str)
+        {
+            return static_cast<uint32_t>(std::stoul(str));
+        }
+    };
+
+    template <>
+    struct StringToNumber<std::uint64_t>
+    {
+        static uint64_t Get(const std::string& str)
+        {
+            return static_cast<uint64_t>(std::stoull(str));
+        }
+    };
+
     template <class T>
     static T GetNumber(const ComPtr<IXmlElement>& element, XmlAttributeName attribute, T defaultValue)
     {
         const auto& attributeValue = element->GetAttributeValue(attribute);
         bool hasValue = !attributeValue.empty();
         T value = defaultValue;
-        if (hasValue) { value = static_cast<T>(std::stoul(attributeValue)); }
+        if (hasValue)
+        {
+            try
+            {
+                value = StringToNumber<T>::Get(attributeValue);
+            }
+            catch (std::invalid_argument& ia)
+            {
+                ThrowErrorAndLog(Error::XmlInvalidData, ia.what());
+            }
+            catch (std::out_of_range& oor)
+            {
+                ThrowErrorAndLog(Error::XmlInvalidData, oor.what());
+            }
+            catch (...)
+            {
+                ThrowErrorAndLog(Error::XmlInvalidData, "Unexpected exception converting string to number");
+            }
+        }
         return value;
     }
 

src/inc/internal/README.txt

@@ -0,0 +1 @@
+Contains headers for consumption by both the product and *unit* tests.

src/inc/public/MsixErrors.hpp

@@ -3,6 +3,8 @@
 //  See LICENSE file in the project root for full license information.
 // 
 #pragma once
+#ifndef MSIX_MSIX_ERRORS__H
+#define MSIX_MSIX_ERRORS__H
 
 namespace MSIX {
 
@@ -84,5 +86,8 @@ namespace MSIX {
         XmlWarning                  = XML_FACILITY + 0x0001,
         XmlError                    = XML_FACILITY + 0x0002,
         XmlFatal                    = XML_FACILITY + 0x0003,
+        XmlInvalidData              = XML_FACILITY + 0x0004,
     };
 }
+
+#endif

src/inc/public/README.txt

@@ -0,0 +1 @@
+Contains headers for public consumption; can only reference each other and external dependencies.

src/inc/shared/Exceptions.hpp

@@ -11,10 +11,21 @@
 #include <functional>
 #include <sstream>
 
-#include "Log.hpp"
 #include "MSIXWindows.hpp"
 #include "MsixErrors.hpp"
 
+#ifndef MSIX_TEST
+#include "Log.hpp"
+#else
+namespace MSIX {
+    namespace Global {
+        namespace Log {
+            inline void Append(const std::string&) {}
+        }
+    }
+}
+#endif
+
 namespace MSIX {
 
     // Defines a common exception type to throw in exceptional cases.  DO NOT USE FOR FLOW CONTROL!
@@ -122,7 +133,11 @@ namespace MSIX {
 #define ThrowErrorIf(c, a, m) ThrowErrorIfNot(c,!(a), m)
 #define ThrowErrorAndLog(c, m) { MSIX::RaiseException<MSIX::Exception>(__LINE__, __FILE__, m, c); }
 
+#ifndef MSIX_TEST
 #define ThrowHrIfFailed(a) MSIX::RaiseExceptionIfFailed(a, __LINE__, __FILE__);
+#else
+#define ThrowHrIfFailed(a) { HRESULT __excMacroHR = (a); ThrowErrorIf(__excMacroHR, FAILED(__excMacroHR), nullptr) }
+#endif
 
 #ifdef WIN32
     #define ThrowHrIfFalse(a, m)                                                                               \

src/inc/shared/README.txt

@@ -0,0 +1,3 @@
+Contains headers for consumption by both the product and *functional* tests; can only reference each other, public headers, and external dependencies.
+
+Any functions used by tests must be implemented in the headers, as the tests will not be linking in anything.

src/msix/CMakeLists.txt

@@ -87,9 +87,9 @@ endif()
 include(msix_resources) # Handles all the certificates and schemas we are going to use.
 
 set(LIB_PUBLIC_HEADERS
-    ../inc/AppxPackaging.hpp
-    ../inc/MSIXWindows.hpp
-    ../inc/MsixErrors.hpp
+    ../inc/public/AppxPackaging.hpp
+    ../inc/public/MSIXWindows.hpp
+    ../inc/public/MsixErrors.hpp
 )
 
 set(MsixSrc) # list with all the files we are going to use
@@ -230,13 +230,13 @@ add_library(${PROJECT_NAME} SHARED
 add_dependencies(${PROJECT_NAME} LIBS)
 
 # Copy out public headers to <binary dir>/src/unpack
-configure_file(../inc/MSIXWindows.hpp   ${CMAKE_CURRENT_BINARY_DIR}/MSIXWindows.hpp  )
-configure_file(../inc/AppxPackaging.hpp ${CMAKE_CURRENT_BINARY_DIR}/AppxPackaging.hpp)
-configure_file(../inc/MsixErrors.hpp ${CMAKE_CURRENT_BINARY_DIR}/MsixErrors.hpp)
+configure_file(../inc/public/MSIXWindows.hpp   ${CMAKE_CURRENT_BINARY_DIR}/MSIXWindows.hpp  )
+configure_file(../inc/public/AppxPackaging.hpp ${CMAKE_CURRENT_BINARY_DIR}/AppxPackaging.hpp)
+configure_file(../inc/public/MsixErrors.hpp    ${CMAKE_CURRENT_BINARY_DIR}/MsixErrors.hpp)
 
 # Linker and includes
 # Include MSIX headers
-target_include_directories(${PROJECT_NAME} PRIVATE ${MSIX_PROJECT_ROOT}/src/inc)
+target_include_directories(${PROJECT_NAME} PRIVATE ${MSIX_PROJECT_ROOT}/src/inc/public ${MSIX_PROJECT_ROOT}/src/inc/shared ${MSIX_PROJECT_ROOT}/src/inc/internal)
 
 if(WIN32)
     string(REPLACE "/GR" " " CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}")

src/msix/pack/AppxPackageWriter.cpp

@@ -175,7 +175,7 @@ namespace MSIX {
         ULARGE_INTEGER end = { 0 };
         ThrowHrIfFailed(stream->Seek(start, StreamBase::Reference::END, &end));
         ThrowHrIfFailed(stream->Seek(start, StreamBase::Reference::START, nullptr));
-        std::uint64_t uncompressedSize = static_cast<std::uint64_t>(end.u.LowPart);
+        std::uint64_t uncompressedSize = static_cast<std::uint64_t>(end.QuadPart);
 
         std::string opcFileName;
         // Don't encode [Content Type].xml

src/test/msixtest/CMakeLists.txt

@@ -14,6 +14,10 @@ if(WIN32)
     endif()
 endif()
 
+# Enable differentiation of based on being used in the tests. 
+# Keep usage of this to a minimum; it is primarily intended for Exceptions.hpp
+add_definitions(-DMSIX_TEST=1)
+
 set(MsixTestFiles)
 
 list(APPEND MsixTestFiles
@@ -81,7 +85,7 @@ else()
         )
 endif()
 
-target_include_directories(${PROJECT_NAME} PRIVATE ${MSIX_BINARY_ROOT}/src/msix ${MSIX_PROJECT_ROOT}/lib/catch2 ${CMAKE_CURRENT_SOURCE_DIR}/inc)
+target_include_directories(${PROJECT_NAME} PRIVATE ${MSIX_PROJECT_ROOT}/src/inc/public ${MSIX_PROJECT_ROOT}/lib/catch2 ${CMAKE_CURRENT_SOURCE_DIR}/inc ${MSIX_PROJECT_ROOT}/src/inc/shared)
 
 # Output test binaries into a test directory
 set_target_properties(${PROJECT_NAME} PROPERTIES

src/test/msixtest/api_packagewriter.cpp

@@ -8,6 +8,7 @@
 #include "FileHelpers.hpp"
 #include "PackTestData.hpp"
 #include "macros.hpp"
+#include "StreamBase.hpp"
 
 #include <iostream>
 
@@ -350,3 +351,97 @@ TEST_CASE("Api_AppxPackageWriter_closed", "[api]")
             APPX_COMPRESSION_OPTION_NORMAL,
             fileStream.Get()));
 }
+
+class GeneratedEasilyCompressedFileStream final : public MSIX::StreamBase
+{
+public:
+    GeneratedEasilyCompressedFileStream(uint64_t size) : m_size(size) {}
+
+    // IStream
+    HRESULT STDMETHODCALLTYPE Seek(LARGE_INTEGER move, DWORD origin, ULARGE_INTEGER* newPosition) noexcept override try
+    {
+        // Determine new range relative position
+        LARGE_INTEGER newPos = { 0 };
+        switch (origin)
+        {
+        case Reference::CURRENT:
+            newPos.QuadPart = m_offset + move.QuadPart;
+            break;
+        case Reference::START:
+            newPos.QuadPart = move.QuadPart;
+            break;
+        case Reference::END:
+            newPos.QuadPart = m_size + move.QuadPart;
+            break;
+        }
+
+        // Constrain newPos to range relative values
+        if (newPos.QuadPart < 0)
+        {
+            m_offset = 0;
+        }
+        else
+        {
+            m_offset = std::min(static_cast<uint64_t>(newPos.QuadPart), m_size);
+        }
+
+        if (newPosition) { newPosition->QuadPart = m_offset; }
+        return static_cast<HRESULT>(MSIX::Error::OK);
+    } CATCH_RETURN();
+
+    HRESULT STDMETHODCALLTYPE Read(void* buffer, ULONG countBytes, ULONG* bytesRead) noexcept override try
+    {
+        uint64_t bytesToRead = std::min(m_size - m_offset, static_cast<uint64_t>(countBytes));
+        // We can't really fail so just put the value in directly.
+        if (bytesRead) { *bytesRead = static_cast<ULONG>(bytesToRead); }
+
+        while (bytesToRead)
+        {
+            uint64_t block = m_offset / DefaultBlockSize;
+            uint64_t endOfBlock = (block + 1) * DefaultBlockSize;
+            uint64_t bytesToWrite = std::min(endOfBlock, m_size) - m_offset;
+            bytesToWrite = std::min(bytesToWrite, bytesToRead);
+            memset(buffer, static_cast<int>(block % 256), static_cast<size_t>(bytesToWrite));
+
+            buffer = static_cast<void*>(static_cast<int8_t*>(buffer) + bytesToWrite);
+            m_offset += bytesToWrite;
+            bytesToRead -= bytesToWrite;
+        }
+
+        return static_cast<HRESULT>(MSIX::Error::OK);
+    } CATCH_RETURN();
+
+protected:
+    uint64_t m_size = 0;
+    uint64_t m_offset = 0;
+};
+
+// Test creating a valid msix package with a contained file that is larger than 4GB
+// The package itself will be much smaller; do not unpack the package from this test
+TEST_CASE("Api_AppxPackageWriter_file_over_4GB", "[api][.slow]")
+{
+    auto outputStream = MsixTest::StreamFile("test_package.msix", false, true);
+
+    MsixTest::ComPtr<IAppxPackageWriter> packageWriter;
+    InitializePackageWriter(outputStream.Get(), &packageWriter);
+
+    // Create stream to generate our very compressable data with more than 4GB of data
+    auto fileStream = MsixTest::ComPtr<IStream>::Make<GeneratedEasilyCompressedFileStream>(0x100000100);
+    REQUIRE_SUCCEEDED(packageWriter->AddPayloadFile(
+        L"largefile.bin",
+        TestConstants::ContentType.c_str(),
+        APPX_COMPRESSION_OPTION_NORMAL,
+        fileStream.Get()));
+
+    // Finalize package, create manifest stream
+    MsixTest::ComPtr<IStream> manifestStream;
+    MakeManifestStream(&manifestStream);
+    REQUIRE_SUCCEEDED(packageWriter->Close(manifestStream.Get()));
+
+    // Reopen the package, validates that the written package is readable
+    // return to the beginning
+    LARGE_INTEGER zero = { 0 };
+    REQUIRE_SUCCEEDED(outputStream.Get()->Seek(zero, STREAM_SEEK_SET, nullptr));
+    MsixTest::ComPtr<IAppxPackageReader> packageReader;
+    MsixTest::InitializePackageReader(outputStream.Get(), &packageReader);
+}

src/test/msixtest/inc/msixtest_int.hpp

@@ -6,6 +6,7 @@
 #include "AppxPackaging.hpp"
 #include "MSIXWindows.hpp"
 #include "MsixErrors.hpp"
+#include "ComHelper.hpp"
 
 #include "msixtest.hpp"
 #include "macros.hpp"
@@ -116,72 +117,8 @@ namespace MsixTest {
     void InitializeBundleReader(const std::string& package, IAppxBundleReader** bundleReader);
     void InitializeManifestReader(const std::string& manifest, IAppxManifestReader** manifestReader);
 
-    template <class T>
-    class ComPtr
-    {
-    public:
-        // default ctor
-        ComPtr() = default;
-        explicit ComPtr(T* ptr) : m_ptr(ptr) { InternalAddRef(); }
-
-        ComPtr(const ComPtr& other) : m_ptr(other.m_ptr) { InternalAddRef(); }
-        ComPtr& operator=(const ComPtr& other) { InternalRelease(); m_ptr = other.m_ptr; InternalAddRef(); return *this; }
-
-        ComPtr(ComPtr&& other) : m_ptr(other.m_ptr) { other.m_ptr = nullptr; }
-        ComPtr& operator=(ComPtr&& other) { InternalRelease(); m_ptr = other.m_ptr; other.m_ptr = nullptr; return *this; }
-
-        ~ComPtr() { InternalRelease(); }
-
-        // For use instead of ComPtr<T> t(new Foo(...));
-        template<class U, class... Args>
-        static ComPtr<T> Make(Args&&... args)
-        {
-            ComPtr<T> result;
-            result.m_ptr = new U(std::forward<Args>(args)...);
-            return result;
-        }
-        
-        inline T* operator->() const { return m_ptr; }
-        inline T* Get() const { return m_ptr; }
-        T* Detach() 
-        {
-            T* temp = m_ptr;
-            m_ptr = nullptr;
-            return temp;
-        }
-
-        template <class U>
-        ComPtr<U> As() const
-        {
-            ComPtr<U> out;
-            REQUIRE_SUCCEEDED(m_ptr->QueryInterface(UuidOfImpl<U>::iid, reinterpret_cast<void**>(&out)));
-            return out;
-        }
-
-        inline T** operator&()
-        {   InternalRelease();
-            return &m_ptr;
-        }
-
-        bool Release()
-        {
-            return InternalRelease();
-        }
-
-    protected:
-        T* m_ptr = nullptr;
-
-        inline void InternalAddRef() { if (m_ptr) { m_ptr->AddRef(); } }
-        inline bool InternalRelease()
-        {
-            T* temp = m_ptr;
-            if (temp)
-            {   m_ptr = nullptr;
-                return (temp->Release() == 0);
-            }
-            return false;
-        }
-    };
+    // Use the product ComPtr; enables sharing without updating every qualified use.
+    using MSIX::ComPtr;
 
     // Helper class that creates a stream from a given file name.
     // toRead - true if the file already exists, false to create it

src/test/msixtest/msixtest.cpp

@@ -215,12 +215,10 @@ namespace MsixTest {
     {
         if (m_toDelete && (m_stream.Get() != nullptr))
         {
+            m_stream = nullptr;
             // best effort to delete the file. If someone else has a reference to this stream
             // and this object is deleted, the file WILL NOT be deleted.
-            if (m_stream.Release())
-            {
-                remove(m_fileName.c_str());
-            }
+            remove(m_fileName.c_str());
         }
     }
 }