package/qa/cppunit/data/tdf163341.ods |binary package/qa/cppunit/data/tdf163364.ods |binary package/qa/cppunit/test_zippackage.cxx | 48 +++++++++++++++++++++++++++++++ package/source/zipapi/ZipFile.cxx | 10 +++--- package/source/zippackage/ZipPackage.cxx | 8 +++++ 5 files changed, 61 insertions(+), 5 deletions(-)
New commits: commit cb5e6a5f1139064c50f00566c30576f3a8f1ab48 Author: Michael Stahl <[email protected]> AuthorDate: Mon Oct 14 13:52:12 2024 +0200 Commit: Michael Stahl <[email protected]> CommitDate: Mon Oct 14 16:19:53 2024 +0200 tdf#163341 package: fix reading Zip64 produced by stream-write-ods 1. Accept 0xFFFF as nEndDisk/nEndDirDisk - the Zip APPNOTE says that values that don't fit into 16 bits SHOULD be 0xFFFF but it doesn't prohibit values that do fit (like, uhm, 0) to be written as 0xFFFF (regression from commit ca21cc985d57fffe7c834159b17c095206304994) 2. Fix misuse of o3tl::make_unsigned - it requires non-negative value, just do signed compare instead 3. Fix bad conversion from pointer to optional in ZipFile::readExtraFields() which effectively prevented the offset from being read (regression from commit efae4fc42d5fe3c0a69757226f38efc10d101194) Change-Id: Ib5e7776a30834e507b297fb28266b5489d1ab68d Reviewed-on: https://gerrit.libreoffice.org/c/core/+/174898 Tested-by: Jenkins Reviewed-by: Michael Stahl <[email protected]> (cherry picked from commit 279f42fa8b19d4fe81c3bba4c7af21aa8ab135b9) diff --git a/package/qa/cppunit/data/tdf163341.ods b/package/qa/cppunit/data/tdf163341.ods new file mode 100644 index 000000000000..5971e0123883 Binary files /dev/null and b/package/qa/cppunit/data/tdf163341.ods differ diff --git a/package/qa/cppunit/test_zippackage.cxx b/package/qa/cppunit/test_zippackage.cxx index 2e12ac379e73..4eabcd9d424d 100644 --- a/package/qa/cppunit/test_zippackage.cxx +++ b/package/qa/cppunit/test_zippackage.cxx @@ -412,6 +412,26 @@ CPPUNIT_TEST_FIXTURE(ZipPackageTest, testTdf163364) } } +CPPUNIT_TEST_FIXTURE(ZipPackageTest, testTdf163341) +{ + auto const url(m_directories.getURLFromSrc(u"/package/qa/cppunit/data/tdf163341.ods")); + uno::Sequence<uno::Any> const args{ + uno::Any(url), + uno::Any(beans::NamedValue("StorageFormat", uno::Any(embed::StorageFormats::PACKAGE))) + }; + + // this Zip64 should load successfully + // on branches with Zip64 support, but not this old branch + try + { + m_xContext->getServiceManager()->createInstanceWithArgumentsAndContext(ZipPackage, args, + m_xContext); + } + catch (css::packages::zip::ZipIOException const&) + { + } +} + //CPPUNIT_TEST_SUITE_REGISTRATION(...); //CPPUNIT_PLUGIN_IMPLEMENT(); diff --git a/package/source/zipapi/ZipFile.cxx b/package/source/zipapi/ZipFile.cxx index 54e63d670d8e..6b253c665399 100644 --- a/package/source/zipapi/ZipFile.cxx +++ b/package/source/zipapi/ZipFile.cxx @@ -999,12 +999,12 @@ std::tuple<sal_Int64, sal_Int64, sal_Int64> ZipFile::findCentralDirectory() aGrabber.seek(nEndPos + 4); sal_uInt16 const nEndDisk = aGrabber.ReadUInt16(); - if (nEndDisk != 0) + if (nEndDisk != 0 && nEndDisk != 0xFFFF) { // only single disk is supported! throw ZipException("invalid end (disk)" ); } sal_uInt16 const nEndDirDisk = aGrabber.ReadUInt16(); - if (nEndDirDisk != 0) + if (nEndDirDisk != 0 && nEndDisk != 0xFFFF) { throw ZipException("invalid end (directory disk)" ); } @@ -1086,12 +1086,12 @@ std::tuple<sal_Int64, sal_Int64, sal_Int64> ZipFile::findCentralDirectory() { throw ZipException("inconsistent Zip/Zip64 end (entries)"); } - if (o3tl::make_unsigned(nEndDirSize) != sal_uInt32(-1) + if (nEndDirSize != -1 && nEnd64DirSize != nEndDirSize) { throw ZipException("inconsistent Zip/Zip64 end (size)"); } - if (o3tl::make_unsigned(nEndDirOffset) != sal_uInt32(-1) + if (nEndDirOffset != -1 && nEnd64DirOffset != nEndDirOffset) { throw ZipException("inconsistent Zip/Zip64 end (offset)"); @@ -1378,7 +1378,7 @@ bool ZipFile::readExtraFields(MemoryByteGrabber& aMemGrabber, sal_Int16 nExtraLe isZip64 = true; } nReadSize = 16; - if (dataSize >= 24 /*&& roOffset*/) + if (dataSize >= 24) { isZip64 = true; #if 0 commit 12adc813d1d36d8ef66cd639bed0d1bced8e21b2 Author: Michael Stahl <[email protected]> AuthorDate: Mon Oct 14 12:04:05 2024 +0200 Commit: Michael Stahl <[email protected]> CommitDate: Mon Oct 14 15:13:15 2024 +0200 tdf#163364 package: ask to recover for this invalid ODF package Bugdoc has a data descriptor on a folder entry, which is very odd and entirely pointless. Which is also the first entry, so it's an invalid ODF package anyway. ZipPackageFolder throws UnknownPropertyException for "WasEncrypted", which results in General I/O error, but we want to ask the user if the file should be opened in recovery mode. (regression from commit 32cad89592ec04ab552399095c91dd76afb3002c) Change-Id: Iafe610d507cf92d2fd2e9c3040592c3e638a30dd Reviewed-on: https://gerrit.libreoffice.org/c/core/+/174889 Tested-by: Jenkins Reviewed-by: Michael Stahl <[email protected]> (cherry picked from commit 3efad499bf4f7623610a54f9f14622de4954352f) diff --git a/package/qa/cppunit/data/tdf163364.ods b/package/qa/cppunit/data/tdf163364.ods new file mode 100644 index 000000000000..a772aebdbc7e Binary files /dev/null and b/package/qa/cppunit/data/tdf163364.ods differ diff --git a/package/qa/cppunit/test_zippackage.cxx b/package/qa/cppunit/test_zippackage.cxx index 57a2eb1fed74..2e12ac379e73 100644 --- a/package/qa/cppunit/test_zippackage.cxx +++ b/package/qa/cppunit/test_zippackage.cxx @@ -384,6 +384,34 @@ CPPUNIT_TEST_FIXTURE(ZipPackageTest, testZip64End) } } +CPPUNIT_TEST_FIXTURE(ZipPackageTest, testTdf163364) +{ + auto const url(m_directories.getURLFromSrc(u"/package/qa/cppunit/data/tdf163364.ods")); + uno::Sequence<uno::Any> const args{ + uno::Any(url), + uno::Any(beans::NamedValue("StorageFormat", uno::Any(embed::StorageFormats::PACKAGE))) + }; + + // don't load corrupted zip file + CPPUNIT_ASSERT_THROW(m_xContext->getServiceManager()->createInstanceWithArgumentsAndContext( + ZipPackage, args, m_xContext), + css::packages::zip::ZipIOException); + + try + { + uno::Sequence<uno::Any> const args2{ + uno::Any(url), uno::Any(beans::NamedValue("RepairPackage", uno::Any(true))), + uno::Any(beans::NamedValue("StorageFormat", uno::Any(embed::StorageFormats::ZIP))) + }; + m_xContext->getServiceManager()->createInstanceWithArgumentsAndContext(ZipPackage, args2, + m_xContext); + } + catch (css::packages::zip::ZipIOException const&) + { + // check that this doesn't crash, it doesn't matter if it succeeds or not + } +} + //CPPUNIT_TEST_SUITE_REGISTRATION(...); //CPPUNIT_PLUGIN_IMPLEMENT(); diff --git a/package/source/zippackage/ZipPackage.cxx b/package/source/zippackage/ZipPackage.cxx index 37370c0f7e3f..7988db4fbb4c 100644 --- a/package/source/zippackage/ZipPackage.cxx +++ b/package/source/zippackage/ZipPackage.cxx @@ -176,6 +176,14 @@ void ZipPackage::checkZipEntriesWithDD() { uno::Reference<XPropertySet> xStream; getByHierarchicalName(rEntry.sPath) >>= xStream; + uno::Reference<XServiceInfo> const xStreamSI{xStream, uno::UNO_QUERY_THROW}; + if (!xStreamSI->supportsService("com.sun.star.packages.PackageStream")) + { + SAL_INFO("package", "entry STORED with data descriptor is folder: \"" << rEntry.sPath << "\""); + throw ZipIOException( + THROW_WHERE + "entry STORED with data descriptor is folder"); + } if (!xStream->getPropertyValue("WasEncrypted").get<bool>()) { SAL_INFO("package", "entry STORED with data descriptor but not encrypted: \"" << rEntry.sPath << "\"");
