diff --git a/CHANGELOG.md b/CHANGELOG.md index f6c6706ad..4eb7a81c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,10 @@ The release run heads these entries with the version and opens a fresh ## Unreleased +- Document path saves preserve the destination on write failure and retain + lazy resources when saving repeatedly over the input. Existing permissions + are preserved; temporary copies are private to their owner. + - PDF calculator functions distinguish booleans, integers and reals; correct bit shifts and rounding; and reject invalid arithmetic and operand types. diff --git a/docs/design/editing.md b/docs/design/editing.md index 8d4f3aced..cf33f2560 100644 --- a/docs/design/editing.md +++ b/docs/design/editing.md @@ -110,8 +110,16 @@ different feature and belongs to ``, `w:ins` and `w:del`. The browser refuses edits for UX only. Replay validates again and throws on an op it cannot apply. Earlier operations remain applied when a later one fails; -replay does not clone or roll back the document. Saving is a separate call and -is not atomic. The envelope states a `version`, and replay refuses any other. +replay does not clone or roll back the document. The envelope states a +`version`, and replay refuses any other. + +Saving is a separate call. Path saves write a temporary file beside the +destination and replace it after a successful close. Before that, the +document's `release_source` moves a ZIP package that reads from the +destination onto a private copy, so lazy resource reads and later saves keep +working. Only the saving document is moved: a second document open on the +same file is not, and a hard link to the destination keeps the old bytes. +Stream saves leave partial output on failure. Open: a conformance corpus of `(base document, op log) → expected saved result` cases, replayed in C++ as a GoogleTest, is not built. What stands in diff --git a/src/odr/document.cpp b/src/odr/document.cpp index 37fefcc95..1813ff2c1 100644 --- a/src/odr/document.cpp +++ b/src/odr/document.cpp @@ -49,22 +49,27 @@ bool Document::is_savable(const bool encrypted) const noexcept { // Every overload checks before it writes, so an unsavable format leaves no // empty file behind and no half-written stream. void Document::save(const std::string &path) const { - if (!m_impl->is_savable(false)) { - throw UnsupportedOperation(); - } - recalculate_edits_(); - std::ofstream out = internal::util::file::create(path); - m_impl->save(out); + save_file_(path, nullptr); } void Document::save(const std::string &path, const std::string &password) const { - if (!m_impl->is_savable(true)) { + save_file_(path, password.c_str()); +} + +void Document::save_file_(const std::string &path, const char *password) const { + if (!m_impl->is_savable(password != nullptr)) { throw UnsupportedOperation(); } recalculate_edits_(); - std::ofstream out = internal::util::file::create(path); - m_impl->save(out, password.c_str()); + m_impl->release_source(internal::AbsPath(std::filesystem::absolute(path))); + internal::util::file::write_atomic(path, [&](std::ostream &out) { + if (password == nullptr) { + m_impl->save(out); + } else { + m_impl->save(out, password); + } + }); } void Document::save(std::ostream &out) const { diff --git a/src/odr/document.hpp b/src/odr/document.hpp index bef61eb94..166dc679a 100644 --- a/src/odr/document.hpp +++ b/src/odr/document.hpp @@ -61,6 +61,7 @@ class Document final { /// out in the clear. [[nodiscard]] bool is_savable(bool encrypted = false) const noexcept; + /// Replaces the destination only after writing and closing successfully. void save(const std::string &path) const; void save(const std::string &path, const std::string &password) const; @@ -161,6 +162,7 @@ class Document final { /// Recalculates formulas made stale by edits before saving. void recalculate_edits_() const; + void save_file_(const std::string &path, const char *password) const; /// @p element 's identifier, checked to be one this document holds. [[nodiscard]] ElementIdentifier check_(const Element &element) const; diff --git a/src/odr/internal/abstract/document.hpp b/src/odr/internal/abstract/document.hpp index acc700884..a87453816 100644 --- a/src/odr/internal/abstract/document.hpp +++ b/src/odr/internal/abstract/document.hpp @@ -32,6 +32,7 @@ struct GraphicStyle; } // namespace odr namespace odr::internal { +class AbsPath; class SheetDependencies; } // namespace odr::internal @@ -77,6 +78,9 @@ class Document { virtual void save(std::ostream &out) const = 0; virtual void save(std::ostream &out, const char *password) const = 0; + /// Called before a save replaces the file at @p path: whatever still reads + /// from that file reads from a private copy from then on. + virtual void release_source(const AbsPath & /*path*/) const {} [[nodiscard]] virtual FileType file_type() const noexcept = 0; [[nodiscard]] virtual DocumentType document_type() const noexcept = 0; diff --git a/src/odr/internal/common/document.cpp b/src/odr/internal/common/document.cpp index 32ccb562e..99d89d58b 100644 --- a/src/odr/internal/common/document.cpp +++ b/src/odr/internal/common/document.cpp @@ -5,14 +5,17 @@ #include #include #include +#include namespace odr::internal { Document::Document(const FileType file_type, const DocumentType document_type, std::shared_ptr files, - const EncryptionState encryption_state) + const EncryptionState encryption_state, + std::shared_ptr archive) : m_file_type{file_type}, m_document_type{document_type}, - m_encryption_state{encryption_state}, m_files{std::move(files)} {} + m_encryption_state{encryption_state}, m_files{std::move(files)}, + m_archive{std::move(archive)} {} Document::~Document() = default; @@ -24,6 +27,12 @@ bool Document::is_savable(const bool /*encrypted*/) const noexcept { return false; } +void Document::release_source(const AbsPath &path) const { + if (m_archive != nullptr) { + m_archive->release_source(path); + } +} + void Document::save(std::ostream & /*out*/) const { throw UnsupportedOperation(); } diff --git a/src/odr/internal/common/document.hpp b/src/odr/internal/common/document.hpp index 4cee09e88..2d5977afd 100644 --- a/src/odr/internal/common/document.hpp +++ b/src/odr/internal/common/document.hpp @@ -11,6 +11,10 @@ #include #include +namespace odr::internal::zip::util { +class Archive; +} // namespace odr::internal::zip::util + namespace odr::internal::abstract { class ReadableFilesystem; } // namespace odr::internal::abstract @@ -21,9 +25,11 @@ class SheetDependencies; class Document : public abstract::Document { public: + /// @p archive is the package @p files reads from lazily, if any. Document(FileType file_type, DocumentType document_type, std::shared_ptr files, - EncryptionState encryption_state = EncryptionState::not_encrypted); + EncryptionState encryption_state = EncryptionState::not_encrypted, + std::shared_ptr archive = nullptr); ~Document() override; /// Read-only, which every engine but odf and ooxml text is. @@ -31,6 +37,7 @@ class Document : public abstract::Document { [[nodiscard]] bool is_savable(bool encrypted) const noexcept override; void save(std::ostream &out) const override; void save(std::ostream &out, const char *password) const override; + void release_source(const AbsPath &path) const final; [[nodiscard]] FileType file_type() const noexcept final; [[nodiscard]] DocumentType document_type() const noexcept final; @@ -73,6 +80,7 @@ class Document : public abstract::Document { EncryptionState m_encryption_state{EncryptionState::not_encrypted}; std::shared_ptr m_files; + std::shared_ptr m_archive; ElementIdentifier m_root_element{null_element_id}; std::unique_ptr m_element_adapter; diff --git a/src/odr/internal/common/temporary_file.cpp b/src/odr/internal/common/temporary_file.cpp index ece0d8e4b..00198e30a 100644 --- a/src/odr/internal/common/temporary_file.cpp +++ b/src/odr/internal/common/temporary_file.cpp @@ -6,7 +6,9 @@ #include #include +#include #include +#include #include namespace odr::internal { @@ -56,6 +58,24 @@ TemporaryDiskFile::operator=(TemporaryDiskFile &&other) noexcept { return *this; } +void TemporaryDiskFile::persist(const AbsPath &target) { + // a missing target is no error: it has no permissions to keep + std::error_code missing; + const std::filesystem::file_status status = + std::filesystem::status(target.path(), missing); + std::error_code error; + if (std::filesystem::exists(status)) { + std::filesystem::permissions(path().path(), status.permissions(), error); + } + if (!error) { + std::filesystem::rename(path().path(), target.path(), error); + } + if (error) { + throw FileWriteError(target.string()); + } + m_owns_path = false; +} + const TemporaryDiskFileFactory &TemporaryDiskFileFactory::system_default() { static TemporaryDiskFileFactory instance( AbsPath(std::filesystem::temp_directory_path()), @@ -79,6 +99,17 @@ TemporaryDiskFileFactory::copy(const abstract::File &file) const { } TemporaryDiskFile TemporaryDiskFileFactory::copy(std::istream &in) const { + return create_([&](std::ostream &out) { util::stream::pipe(in, out); }, true); +} + +TemporaryDiskFile TemporaryDiskFileFactory::create( + const std::function &write) const { + return create_(write, false); +} + +TemporaryDiskFile TemporaryDiskFileFactory::create_( + const std::function &write, + const bool private_to_owner) const { std::ofstream file; AbsPath file_path; for (std::uint32_t attempt = 0; attempt < 128; ++attempt) { @@ -98,12 +129,18 @@ TemporaryDiskFile TemporaryDiskFileFactory::copy(std::istream &in) const { } try { - util::stream::pipe(in, file); + if (private_to_owner) { + std::filesystem::permissions(file_path.path(), + std::filesystem::perms::owner_read | + std::filesystem::perms::owner_write); + } + write(file); file.close(); if (!file) { throw FileWriteError(file_path.string()); } } catch (...) { + file.exceptions(std::ios::goodbit); file.close(); remove_quietly(file_path); throw; diff --git a/src/odr/internal/common/temporary_file.hpp b/src/odr/internal/common/temporary_file.hpp index f93dff545..ce8186d2a 100644 --- a/src/odr/internal/common/temporary_file.hpp +++ b/src/odr/internal/common/temporary_file.hpp @@ -19,6 +19,10 @@ class TemporaryDiskFile final : public DiskFile { TemporaryDiskFile &operator=(const TemporaryDiskFile &) = delete; TemporaryDiskFile &operator=(TemporaryDiskFile &&) noexcept; + /// Replaces @p target with this file, keeping the permissions @p target + /// has, and leaves it in place from then on. + void persist(const AbsPath &target); + private: bool m_owns_path{true}; ///< cleared by a move, so only one owner removes }; @@ -34,10 +38,18 @@ class TemporaryDiskFileFactory final { AbsPath directory, RandomFileNameGenerator random_file_name_generator = default_random_file_name_generator()); + /// Private to the owner, because a copy can hold decrypted content. [[nodiscard]] TemporaryDiskFile copy(const abstract::File &file) const; [[nodiscard]] TemporaryDiskFile copy(std::istream &in) const; + /// With the permissions any new file in the directory gets. + [[nodiscard]] TemporaryDiskFile + create(const std::function &write) const; private: + [[nodiscard]] TemporaryDiskFile + create_(const std::function &write, + bool private_to_owner) const; + AbsPath m_directory; RandomFileNameGenerator m_random_file_name_generator; }; diff --git a/src/odr/internal/odf/odf_document.cpp b/src/odr/internal/odf/odf_document.cpp index 211cd42bb..9cdf709b8 100644 --- a/src/odr/internal/odf/odf_document.cpp +++ b/src/odr/internal/odf/odf_document.cpp @@ -53,9 +53,10 @@ create_element_adapter(Document &document, ElementRegistry ®istry); Document::Document(const FileType file_type, const DocumentType document_type, std::shared_ptr files, - const EncryptionState encryption_state) + const EncryptionState encryption_state, + std::shared_ptr archive) : internal::Document(file_type, document_type, std::move(files), - encryption_state) { + encryption_state, std::move(archive)) { m_content_xml = xml::parse(*m_files, AbsPath("/content.xml")); if (m_files->exists(AbsPath("/styles.xml"))) { diff --git a/src/odr/internal/odf/odf_document.hpp b/src/odr/internal/odf/odf_document.hpp index 7aa9a45f4..5e1d10f28 100644 --- a/src/odr/internal/odf/odf_document.hpp +++ b/src/odr/internal/odf/odf_document.hpp @@ -16,7 +16,8 @@ class Document final : public internal::Document { public: Document(FileType file_type, DocumentType document_type, std::shared_ptr files, - EncryptionState encryption_state); + EncryptionState encryption_state, + std::shared_ptr archive = nullptr); /// A flat document: one tree holding both content and styles, no filesystem. Document(FileType file_type, DocumentType document_type, pugi::xml_document flat_xml); diff --git a/src/odr/internal/odf/odf_file.cpp b/src/odr/internal/odf/odf_file.cpp index 0a076bec7..ecf97c31d 100644 --- a/src/odr/internal/odf/odf_file.cpp +++ b/src/odr/internal/odf/odf_file.cpp @@ -15,8 +15,9 @@ class File; namespace odr::internal::odf { OpenDocumentFile::OpenDocumentFile( - std::shared_ptr filesystem) - : m_filesystem{std::move(filesystem)} { + std::shared_ptr filesystem, + std::shared_ptr archive) + : m_filesystem{std::move(filesystem)}, m_archive{std::move(archive)} { if (m_filesystem->exists(AbsPath("/META-INF/manifest.xml"))) { const pugi::xml_document manifest = xml::parse(*m_filesystem, AbsPath("/META-INF/manifest.xml")); @@ -77,6 +78,7 @@ OpenDocumentFile::decrypt(const std::string &password) const { auto decrypted_filesystem = odf::decrypt(m_filesystem, m_manifest, password); auto decrypted_file = std::make_shared(*this); decrypted_file->m_filesystem = std::move(decrypted_filesystem); + decrypted_file->m_archive = nullptr; decrypted_file->m_file_meta = parse_file_meta(*decrypted_file->m_filesystem, nullptr, true); decrypted_file->m_encryption_state = EncryptionState::decrypted; @@ -95,18 +97,20 @@ std::shared_ptr OpenDocumentFile::document() const { switch (file_type()) { case FileType::opendocument_text: return std::make_shared(m_file_meta.type, DocumentType::text, - m_filesystem, m_encryption_state); + m_filesystem, m_encryption_state, + m_archive); case FileType::opendocument_presentation: return std::make_shared(m_file_meta.type, DocumentType::presentation, m_filesystem, - m_encryption_state); + m_encryption_state, m_archive); case FileType::opendocument_spreadsheet: return std::make_shared(m_file_meta.type, DocumentType::spreadsheet, m_filesystem, - m_encryption_state); + m_encryption_state, m_archive); case FileType::opendocument_graphics: return std::make_shared(m_file_meta.type, DocumentType::drawing, - m_filesystem, m_encryption_state); + m_filesystem, m_encryption_state, + m_archive); default: throw UnsupportedFileType(file_type()); } diff --git a/src/odr/internal/odf/odf_file.hpp b/src/odr/internal/odf/odf_file.hpp index 79209fcfe..acefa4d19 100644 --- a/src/odr/internal/odf/odf_file.hpp +++ b/src/odr/internal/odf/odf_file.hpp @@ -16,12 +16,18 @@ namespace odr::internal::abstract { class ReadableFilesystem; } +namespace odr::internal::zip::util { +class Archive; +} // namespace odr::internal::zip::util + namespace odr::internal::odf { class OpenDocumentFile final : public virtual abstract::DocumentFile { public: + /// @p archive is the package @p filesystem reads from lazily, if any. explicit OpenDocumentFile( - std::shared_ptr filesystem); + std::shared_ptr filesystem, + std::shared_ptr archive = nullptr); [[nodiscard]] std::shared_ptr file() const noexcept override; @@ -44,6 +50,7 @@ class OpenDocumentFile final : public virtual abstract::DocumentFile { private: std::shared_ptr m_filesystem; + std::shared_ptr m_archive; EncryptionState m_encryption_state{EncryptionState::not_encrypted}; FileMeta m_file_meta; Manifest m_manifest; diff --git a/src/odr/internal/ooxml/ooxml_file.cpp b/src/odr/internal/ooxml/ooxml_file.cpp index 239404507..118d2b1d7 100644 --- a/src/odr/internal/ooxml/ooxml_file.cpp +++ b/src/odr/internal/ooxml/ooxml_file.cpp @@ -17,8 +17,9 @@ namespace odr::internal::ooxml { OfficeOpenXmlFile::OfficeOpenXmlFile( - std::shared_ptr files) - : m_files(std::move(files)) { + std::shared_ptr files, + std::shared_ptr archive) + : m_files(std::move(files)), m_archive(std::move(archive)) { m_file_meta = parse_file_meta(*m_files); if (m_file_meta.password_encrypted) { @@ -89,6 +90,7 @@ OfficeOpenXmlFile::decrypt(const std::string &password) const { std::make_shared(std::move(decrypted_package)); auto decrypted = std::make_shared(*this); decrypted->m_files = zip::ZipFile(memory_file).archive()->as_filesystem(); + decrypted->m_archive = nullptr; decrypted->m_file_meta = parse_file_meta(*decrypted->m_files); decrypted->m_encryption_state = EncryptionState::decrypted; return decrypted; @@ -105,11 +107,12 @@ std::shared_ptr OfficeOpenXmlFile::document() const { switch (file_type()) { case FileType::office_open_xml_document: - return std::make_shared(m_files, m_encryption_state); + return std::make_shared(m_files, m_encryption_state, + m_archive); case FileType::office_open_xml_presentation: - return std::make_shared(m_files); + return std::make_shared(m_files, m_archive); case FileType::office_open_xml_workbook: - return std::make_shared(m_files); + return std::make_shared(m_files, m_archive); default: throw UnsupportedFileType(file_type()); } diff --git a/src/odr/internal/ooxml/ooxml_file.hpp b/src/odr/internal/ooxml/ooxml_file.hpp index 8b5ed0f4e..b674061b0 100644 --- a/src/odr/internal/ooxml/ooxml_file.hpp +++ b/src/odr/internal/ooxml/ooxml_file.hpp @@ -15,12 +15,18 @@ namespace odr::internal::abstract { class ReadableFilesystem; } +namespace odr::internal::zip::util { +class Archive; +} // namespace odr::internal::zip::util + namespace odr::internal::ooxml { class OfficeOpenXmlFile final : public abstract::DocumentFile { public: + /// @p archive is the package @p files reads from lazily, if any. explicit OfficeOpenXmlFile( - std::shared_ptr files); + std::shared_ptr files, + std::shared_ptr archive = nullptr); [[nodiscard]] std::shared_ptr file() const noexcept override; @@ -43,6 +49,7 @@ class OfficeOpenXmlFile final : public abstract::DocumentFile { private: std::shared_ptr m_files; + std::shared_ptr m_archive; FileMeta m_file_meta; EncryptionState m_encryption_state{EncryptionState::not_encrypted}; }; diff --git a/src/odr/internal/ooxml/presentation/ooxml_presentation_document.cpp b/src/odr/internal/ooxml/presentation/ooxml_presentation_document.cpp index 229e97b94..4bf17a575 100644 --- a/src/odr/internal/ooxml/presentation/ooxml_presentation_document.cpp +++ b/src/odr/internal/ooxml/presentation/ooxml_presentation_document.cpp @@ -31,9 +31,11 @@ create_element_adapter(const Document &document, ElementRegistry ®istry); } // namespace -Document::Document(std::shared_ptr files) +Document::Document(std::shared_ptr files, + std::shared_ptr archive) : internal::Document(FileType::office_open_xml_presentation, - DocumentType::presentation, std::move(files)) { + DocumentType::presentation, std::move(files), + EncryptionState::not_encrypted, std::move(archive)) { m_document_xml = xml::parse(*m_files, AbsPath("/ppt/presentation.xml")); // Only the parts the slide-id list names: a package may relate anything at diff --git a/src/odr/internal/ooxml/presentation/ooxml_presentation_document.hpp b/src/odr/internal/ooxml/presentation/ooxml_presentation_document.hpp index 5d31c5c43..e1bb502d1 100644 --- a/src/odr/internal/ooxml/presentation/ooxml_presentation_document.hpp +++ b/src/odr/internal/ooxml/presentation/ooxml_presentation_document.hpp @@ -19,7 +19,8 @@ namespace odr::internal::ooxml::presentation { class Document final : public internal::Document { public: - explicit Document(std::shared_ptr files); + explicit Document(std::shared_ptr files, + std::shared_ptr archive = nullptr); [[nodiscard]] bool is_editable() const noexcept override; [[nodiscard]] bool is_savable(bool encrypted) const noexcept override; diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp index 04ff2c23b..f4ac4ed9d 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp @@ -59,9 +59,11 @@ pugi::xml_node calc_pr(pugi::xml_node workbook) { } } // namespace -Document::Document(std::shared_ptr files) +Document::Document(std::shared_ptr files, + std::shared_ptr archive) : internal::Document(FileType::office_open_xml_workbook, - DocumentType::spreadsheet, std::move(files)) { + DocumentType::spreadsheet, std::move(files), + EncryptionState::not_encrypted, std::move(archive)) { const AbsPath workbook_path("/xl/workbook.xml"); const auto [workbook_xml, workbook_relations] = parse_xml_(workbook_path); m_written_parts.push_back(workbook_path); diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.hpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.hpp index 7d5577721..a87e8ce44 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.hpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.hpp @@ -18,7 +18,8 @@ namespace odr::internal::ooxml::spreadsheet { class Document final : public internal::Document { public: - explicit Document(std::shared_ptr files); + explicit Document(std::shared_ptr files, + std::shared_ptr archive = nullptr); [[nodiscard]] const ElementRegistry &element_registry() const; [[nodiscard]] const StyleRegistry &style_registry() const; diff --git a/src/odr/internal/ooxml/text/ooxml_text_document.cpp b/src/odr/internal/ooxml/text/ooxml_text_document.cpp index f2fcb9c2b..67e86b85f 100644 --- a/src/odr/internal/ooxml/text/ooxml_text_document.cpp +++ b/src/odr/internal/ooxml/text/ooxml_text_document.cpp @@ -70,9 +70,11 @@ PageLayout read_page_layout(const pugi::xml_node body) { } // namespace Document::Document(std::shared_ptr files, - const EncryptionState encryption_state) + const EncryptionState encryption_state, + std::shared_ptr archive) : internal::Document(FileType::office_open_xml_document, DocumentType::text, - std::move(files), encryption_state) { + std::move(files), encryption_state, + std::move(archive)) { m_document_xml = xml::parse(*m_files, AbsPath("/word/document.xml")); if (m_files->is_file(AbsPath("/word/styles.xml"))) { m_styles_xml = xml::parse(*m_files, AbsPath("/word/styles.xml")); diff --git a/src/odr/internal/ooxml/text/ooxml_text_document.hpp b/src/odr/internal/ooxml/text/ooxml_text_document.hpp index 2e002d0cb..64fdcc4d6 100644 --- a/src/odr/internal/ooxml/text/ooxml_text_document.hpp +++ b/src/odr/internal/ooxml/text/ooxml_text_document.hpp @@ -18,7 +18,8 @@ namespace odr::internal::ooxml::text { class Document final : public internal::Document { public: Document(std::shared_ptr files, - EncryptionState encryption_state); + EncryptionState encryption_state, + std::shared_ptr archive = nullptr); ElementRegistry &element_registry(); StyleRegistry &style_registry(); diff --git a/src/odr/internal/open_strategy.cpp b/src/odr/internal/open_strategy.cpp index de1c4dd7f..84468034f 100644 --- a/src/odr/internal/open_strategy.cpp +++ b/src/odr/internal/open_strategy.cpp @@ -86,7 +86,8 @@ open_file_as(const std::shared_ptr &file, const FileType as, try { auto zip_file = std::make_unique(file); auto filesystem = zip_file->archive()->as_filesystem(); - auto odf_file = std::make_unique(filesystem); + auto odf_file = + std::make_unique(filesystem, zip_file->zip()); if (odf_file->file_type() == as) { return odf_file; } @@ -134,7 +135,8 @@ open_file_as(const std::shared_ptr &file, const FileType as, try { auto zip_file = std::make_unique(file); auto filesystem = zip_file->archive()->as_filesystem(); - auto ooxml_file = std::make_unique(filesystem); + auto ooxml_file = std::make_unique( + filesystem, zip_file->zip()); if (is_the_requested_ooxml(*ooxml_file, as)) { return ooxml_file; } @@ -481,14 +483,16 @@ open_by_cascade(const std::shared_ptr &file, try { ODR_VERBOSE(logger, "try open as odf"); - return std::make_unique(filesystem); + return std::make_unique(filesystem, + zip_file->zip()); } catch (...) { ODR_VERBOSE(logger, "failed to open as odf"); } try { ODR_VERBOSE(logger, "try open as ooxml"); - return std::make_unique(filesystem); + return std::make_unique(filesystem, + zip_file->zip()); } catch (...) { ODR_VERBOSE(logger, "failed to open as ooxml"); } diff --git a/src/odr/internal/util/file_util.cpp b/src/odr/internal/util/file_util.cpp index 8dec5e6f5..182de3878 100644 --- a/src/odr/internal/util/file_util.cpp +++ b/src/odr/internal/util/file_util.cpp @@ -3,6 +3,7 @@ #include #include +#include #include #include @@ -47,6 +48,25 @@ std::ofstream file::create(const std::string &path) { return out; } +void file::write_atomic(const std::string &path, + const std::function &write) { + const std::filesystem::path target = + std::filesystem::weakly_canonical(std::filesystem::absolute(path)); + const auto status = std::filesystem::status(target); + // a rename needs only the directory to be writable, so the file's own + // permissions refuse as truncating it would + if (std::filesystem::is_symlink(std::filesystem::symlink_status(target)) || + (std::filesystem::exists(status) && + (!std::filesystem::is_regular_file(status) || + (status.permissions() & std::filesystem::perms::owner_write) == + std::filesystem::perms::none))) { + throw FileWriteError(path); + } + TemporaryDiskFileFactory(AbsPath(target.parent_path())) + .create(write) + .persist(AbsPath(target)); +} + void file::write(const std::string &data, const std::string &path) { std::ofstream out = create(path); out << data; diff --git a/src/odr/internal/util/file_util.hpp b/src/odr/internal/util/file_util.hpp index ef3f297e8..be1e55ca8 100644 --- a/src/odr/internal/util/file_util.hpp +++ b/src/odr/internal/util/file_util.hpp @@ -1,5 +1,6 @@ #pragma once +#include #include #include @@ -11,6 +12,9 @@ std::string read(const std::string &path); void pipe(const std::string &path, std::ostream &out); std::ofstream create(const std::string &path); +/// Write beside the destination, then replace it after a successful close. +void write_atomic(const std::string &path, + const std::function &write); void write(const std::string &data, const std::string &path); void write(std::istream &in, const std::string &path); diff --git a/src/odr/internal/zip/zip_file.cpp b/src/odr/internal/zip/zip_file.cpp index 2927a56cb..8c3024aec 100644 --- a/src/odr/internal/zip/zip_file.cpp +++ b/src/odr/internal/zip/zip_file.cpp @@ -27,6 +27,10 @@ FileMeta ZipFile::file_meta() const { bool ZipFile::is_decodable() const noexcept { return true; } +const std::shared_ptr &ZipFile::zip() const noexcept { + return m_zip; +} + std::shared_ptr ZipFile::archive() const { return std::make_shared(m_zip); } diff --git a/src/odr/internal/zip/zip_file.hpp b/src/odr/internal/zip/zip_file.hpp index d2be6415f..ef86edf98 100644 --- a/src/odr/internal/zip/zip_file.hpp +++ b/src/odr/internal/zip/zip_file.hpp @@ -25,6 +25,8 @@ class ZipFile final : public abstract::ArchiveFile { [[nodiscard]] bool is_decodable() const noexcept override; [[nodiscard]] std::shared_ptr archive() const override; + /// The package, which reads its entries from @ref file lazily. + [[nodiscard]] const std::shared_ptr &zip() const noexcept; private: std::shared_ptr m_zip; diff --git a/src/odr/internal/zip/zip_util.cpp b/src/odr/internal/zip/zip_util.cpp index 70ddee765..6d1af7fec 100644 --- a/src/odr/internal/zip/zip_util.cpp +++ b/src/odr/internal/zip/zip_util.cpp @@ -3,11 +3,15 @@ #include #include +#include #include #include #include +#include #include +#include +#include #include namespace odr::internal::zip::util { @@ -196,6 +200,20 @@ ReadSource::ReadSource(std::shared_ptr file) m_memory = m_file->memory_data(); } +void ReadSource::release_source(const AbsPath &path) { + const std::lock_guard lock(m_mutex); + const std::optional source = m_file->disk_path(); + // false where either file is missing, such as a source deleted after open + std::error_code error; + if (!source.has_value() || + !std::filesystem::equivalent(source->path(), path.path(), error)) { + return; + } + m_file = std::make_shared( + TemporaryDiskFileFactory::system_default().copy(*m_file)); + m_streams.clear(); +} + std::size_t ReadSource::read(const std::uint64_t offset, void *buffer, const std::size_t size) const { if (m_memory.has_value()) { @@ -213,16 +231,17 @@ std::size_t ReadSource::read(const std::uint64_t offset, void *buffer, return 0; } std::unique_ptr stream; + std::shared_ptr source; { std::lock_guard lock(m_mutex); + source = m_file; if (!m_streams.empty()) { stream = std::move(m_streams.back()); m_streams.pop_back(); + } else { + stream = source->stream(); } } - if (stream == nullptr) { - stream = m_file->stream(); - } // A short read has to surface as one. Clear first so an earlier read past the // end does not poison every later seek. @@ -233,7 +252,9 @@ std::size_t ReadSource::read(const std::uint64_t offset, void *buffer, { std::lock_guard lock(m_mutex); - m_streams.push_back(std::move(stream)); + if (source == m_file) { + m_streams.push_back(std::move(stream)); + } } return result; @@ -255,6 +276,10 @@ Archive::Archive(std::shared_ptr file) Archive::~Archive() { mz_zip_end(&m_zip); } +void Archive::release_source(const AbsPath &path) const { + m_source->release_source(path); +} + mz_zip_archive *Archive::zip() const { return &m_zip; } std::shared_ptr Archive::file() const noexcept { diff --git a/src/odr/internal/zip/zip_util.hpp b/src/odr/internal/zip/zip_util.hpp index b9e28e97c..9894b8069 100644 --- a/src/odr/internal/zip/zip_util.hpp +++ b/src/odr/internal/zip/zip_util.hpp @@ -16,6 +16,7 @@ namespace odr::internal { class RelPath; +class AbsPath; } // namespace odr::internal namespace odr::internal::zip::util { @@ -31,6 +32,9 @@ enum class Method { class ReadSource final { public: explicit ReadSource(std::shared_ptr file); + /// Reads from a private copy from now on if the source is the file at + /// @p path, which a save is about to replace. + void release_source(const AbsPath &path); [[nodiscard]] std::size_t read(std::uint64_t offset, void *buffer, std::size_t size) const; @@ -48,6 +52,8 @@ class Archive final : public std::enable_shared_from_this { public: explicit Archive(std::shared_ptr file); ~Archive(); + /// See @ref ReadSource::release_source. + void release_source(const AbsPath &path) const; [[nodiscard]] mz_zip_archive *zip() const; diff --git a/test/src/document_test.cpp b/test/src/document_test.cpp index 3c3932cb3..239c39d14 100644 --- a/test/src/document_test.cpp +++ b/test/src/document_test.cpp @@ -2,7 +2,16 @@ #include #include #include +#include #include +#include +#include +#include +#include +#include +#include +#include +#include #include #include #include @@ -16,6 +25,7 @@ #include #include #include +#include #include #include #include @@ -798,3 +808,92 @@ TEST(Document, saving_an_unsavable_format_leaves_no_file) { EXPECT_THROW(document.save(out), UnsupportedOperation); EXPECT_THROW((void)document.save_to_memory(), UnsupportedOperation); } + +namespace { + +/// Bytes that do not compress, so the package reads them lazily from disk. +std::string incompressible_payload() { + std::string payload(100000, '\0'); + std::uint32_t state = 1; + for (char &byte : payload) { + state ^= state << 13; + state ^= state >> 17; + state ^= state << 5; + byte = static_cast(state); + } + return payload; +} + +/// An `.odt` on disk with one paragraph and @p payload as a picture. +odr::internal::TemporaryDiskFile odt_with(const std::string &payload) { + using namespace odr::internal; + zip::ZipArchive archive; + archive.insert_file( + archive.end(), RelPath("mimetype"), + std::make_shared("application/vnd.oasis.opendocument.text"), + 0); + archive.insert_file(archive.end(), RelPath("content.xml"), + std::make_shared( + "" + "hello" + "")); + archive.insert_file(archive.end(), RelPath("Pictures/data.bin"), + std::make_shared(payload)); + std::stringstream bytes; + archive.save(bytes); + return TemporaryDiskFileFactory::system_default().copy(bytes); +} + +} // namespace + +TEST(Document, repeated_saves_over_source_preserve_lazy_resources) { + using namespace odr::internal; + const std::string payload = incompressible_payload(); + const TemporaryDiskFile source = odt_with(payload); + const std::string path = source.disk_path()->string(); + const odr::Document document = odr::open(path).as_document_file().document(); + const odr::File resource = + document.as_filesystem().open("/Pictures/data.bin"); + const auto active = resource.stream(); + ASSERT_EQ(active->get(), static_cast(payload[0])); + std::filesystem::create_hard_link(path, path + ".alias"); + const TemporaryDiskFile alias(path + ".alias"); + for (const std::string &destination : + {alias.disk_path().value().string(), path}) { + for (const std::string text : + {std::string(2000, 'a'), std::string("bye")}) { + ASSERT_EQ(set_every_text(document.root_element(), text), 1u); + ASSERT_NO_THROW(document.save(destination)); + EXPECT_EQ(util::stream::read(*resource.stream()), payload); + const odr::Document reopened = + odr::open(destination).as_document_file().document(); + EXPECT_EQ(expect_every_text(reopened.root_element(), text), 1u); + EXPECT_EQ( + util::stream::read( + *reopened.as_filesystem().open("/Pictures/data.bin").stream()), + payload); + } + } + EXPECT_EQ(util::stream::read(*active), payload.substr(1)); +} + +// The pooled read streams keep the replaced bytes readable on POSIX, so this +// truncates the source in place: only a released source survives that. +TEST(Document, a_released_source_reads_from_its_own_copy) { + using namespace odr::internal; + const std::string payload = incompressible_payload(); + const TemporaryDiskFile source = odt_with(payload); + const AbsPath path = source.disk_path().value(); + const zip::ZipFile zip_file(std::make_shared(path)); + const auto document = + odf::OpenDocumentFile(zip_file.archive()->as_filesystem(), zip_file.zip()) + .document(); + const auto resource = + document->as_filesystem()->open(AbsPath("/Pictures/data.bin")); + ASSERT_EQ(util::stream::read(*resource->stream()), payload); + + document->release_source(path); + std::ofstream(path.string(), std::ios::binary | std::ios::trunc) << "x"; + + EXPECT_EQ(util::stream::read(*resource->stream()), payload); +} diff --git a/test/src/file_test.cpp b/test/src/file_test.cpp index eee22d4c2..35e62c4ea 100644 --- a/test/src/file_test.cpp +++ b/test/src/file_test.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -423,3 +424,87 @@ TEST(File, metadata_failures_propagate_to_the_caller) { const DecodedFile file(std::make_shared()); EXPECT_THROW((void)file.file_meta(), std::bad_alloc); } + +TEST(File, atomic_writes_preserve_the_destination_on_failure) { + using namespace odr::internal; + const AbsPath directory(std::filesystem::current_path()); + const std::string name = "odr-atomic-" + random_string(12); + const std::filesystem::path path = directory.join(RelPath(name)).path(); + util::file::write_atomic(name, [](std::ostream &out) { out << "original"; }); + const TemporaryDiskFile cleanup(path.string()); + for (const bool throws : {false, true}) { + EXPECT_ANY_THROW(util::file::write_atomic(name, [&](std::ostream &out) { + out << "partial"; + if (throws) { + out.exceptions(std::ios::badbit); + } + out.setstate(std::ios::badbit); + })); + EXPECT_EQ(util::file::read(name), "original"); + } + const auto permissions = std::filesystem::status(path).permissions(); + util::file::write_atomic(name, + [](std::ostream &out) { out << "replacement"; }); + EXPECT_EQ(util::file::read(name), "replacement"); + EXPECT_EQ(std::filesystem::status(path).permissions(), permissions); +} + +TEST(File, atomic_writes_refuse_a_read_only_destination) { + using namespace odr::internal; + const std::string name = "odr-atomic-" + random_string(12); + util::file::write_atomic(name, [](std::ostream &out) { out << "original"; }); + const TemporaryDiskFile cleanup(AbsPath(std::filesystem::absolute(name))); + std::filesystem::permissions(name, std::filesystem::perms::owner_read); + EXPECT_THROW(util::file::write_atomic( + name, [](std::ostream &out) { out << "replacement"; }), + FileWriteError); + EXPECT_EQ(util::file::read(name), "original"); +} + +TEST(File, temporary_copies_are_private_and_new_saves_are_not) { + using namespace odr::internal; + std::istringstream source("bytes"); + const auto copy = TemporaryDiskFileFactory::system_default().copy(source); + constexpr auto others = + std::filesystem::perms::group_all | std::filesystem::perms::others_all; + EXPECT_EQ( + std::filesystem::status(copy.disk_path().value().path()).permissions() & + others, + std::filesystem::perms::none); + + const std::string name = "odr-atomic-" + random_string(12); + const std::string plain = name + "-plain"; + std::ofstream(plain) << "plain"; + const TemporaryDiskFile plain_cleanup( + AbsPath(std::filesystem::absolute(plain))); + util::file::write_atomic(name, [](std::ostream &out) { out << "saved"; }); + const TemporaryDiskFile saved_cleanup( + AbsPath(std::filesystem::absolute(name))); + EXPECT_EQ(std::filesystem::status(name).permissions(), + std::filesystem::status(plain).permissions()); +} + +TEST(File, atomic_writes_follow_existing_symlinks_and_reject_dangling_ones) { + using namespace odr::internal; + const auto target = TemporaryDiskFileFactory::system_default().create( + [](std::ostream &out) { out << "original"; }); + const std::string link_path = target.disk_path().value().string() + ".link"; + std::error_code error; + std::filesystem::create_symlink(target.disk_path().value().path(), link_path, + error); + if (error) { + GTEST_SKIP() << "Symlinks unavailable: " << error.message(); + } + const TemporaryDiskFile link(link_path); + util::file::write_atomic(link.disk_path().value().string(), + [](std::ostream &out) { out << "replacement"; }); + EXPECT_TRUE(std::filesystem::is_symlink(link.disk_path().value().path())); + EXPECT_EQ(util::file::read(target.disk_path().value().string()), + "replacement"); + std::filesystem::remove(target.disk_path().value().path()); + EXPECT_THROW( + util::file::write_atomic(link.disk_path().value().string(), + [](std::ostream &out) { out << "lost"; }), + FileWriteError); + EXPECT_TRUE(std::filesystem::is_symlink(link.disk_path().value().path())); +}