diff --git a/CHANGELOG.md b/CHANGELOG.md index b4a66b16e..1e062c565 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,9 @@ The release run heads these entries with the version and opens a fresh ## Unreleased +- Empty file and HTML handles throw instead of dereferencing null. + Missing master pages and table rows/columns return empty element handles. + - Copied filesystem walkers keep independent traversal positions on disk, matching walkers over archives and memory filesystems. diff --git a/src/odr/document.cpp b/src/odr/document.cpp index 7be6769b3..33003293c 100644 --- a/src/odr/document.cpp +++ b/src/odr/document.cpp @@ -478,6 +478,9 @@ Element Document::root_element() const { Element Document::element_by_id(const ElementIdentifier identifier) const { const internal::abstract::ElementAdapter *adapter = m_impl->element_adapter(); + if (adapter == nullptr || identifier == null_element_id) { + return {}; + } try { // throwing is how a registry answers an id it does not hold static_cast(adapter->element_type(identifier)); @@ -488,7 +491,7 @@ Element Document::element_by_id(const ElementIdentifier identifier) const { } ElementIdentifier Document::check_(const Element &element) const { - if (element_by_id(element.identifier()) != element) { + if (!element || element_by_id(element.identifier()) != element) { throw std::invalid_argument("element is not this document's"); } return element.identifier(); diff --git a/src/odr/document_element.cpp b/src/odr/document_element.cpp index 1a7832d0b..01defa59d 100644 --- a/src/odr/document_element.cpp +++ b/src/odr/document_element.cpp @@ -341,8 +341,7 @@ MasterPage TextRoot::first_master_page() const { } const ElementIdentifier master_page_id = m_adapter2->text_root_first_master_page(m_identifier); - return {m_adapter, master_page_id, - m_adapter->master_page_adapter(master_page_id)}; + return Element(m_adapter, master_page_id).as_master_page(); } std::string Slide::name() const { @@ -359,8 +358,7 @@ MasterPage Slide::master_page() const { } const ElementIdentifier master_page_id = m_adapter2->slide_master_page(m_identifier); - return {m_adapter, master_page_id, - m_adapter->master_page_adapter(master_page_id)}; + return Element(m_adapter, master_page_id).as_master_page(); } std::string Sheet::name() const { @@ -569,8 +567,7 @@ MasterPage Page::master_page() const { } const ElementIdentifier master_page_id = m_adapter2->page_master_page(m_identifier); - return {m_adapter, master_page_id, - m_adapter->master_page_adapter(master_page_id)}; + return Element(m_adapter, master_page_id).as_master_page(); } PageLayout MasterPage::page_layout() const { @@ -668,7 +665,7 @@ TableRow Table::first_row() const { return {}; } const ElementIdentifier row_id = m_adapter2->table_first_row(m_identifier); - return {m_adapter, row_id, m_adapter->table_row_adapter(row_id)}; + return Element(m_adapter, row_id).as_table_row(); } TableColumn Table::first_column() const { @@ -677,7 +674,7 @@ TableColumn Table::first_column() const { } const ElementIdentifier column_id = m_adapter2->table_first_column(m_identifier); - return {m_adapter, column_id, m_adapter->table_column_adapter(column_id)}; + return Element(m_adapter, column_id).as_table_column(); } ElementRange Table::columns() const { diff --git a/src/odr/html.cpp b/src/odr/html.cpp index 0c67e574f..06c93490d 100644 --- a/src/odr/html.cpp +++ b/src/odr/html.cpp @@ -29,6 +29,13 @@ namespace odr { namespace { +template const T &deref(const std::shared_ptr &impl) { + if (impl == nullptr) { + throw NullPointerError("HTML handle"); + } + return *impl; +} + void bring_offline(const HtmlResources &resources, const std::string &output_path) { for (const auto &[resource, location] : resources) { @@ -70,32 +77,34 @@ HtmlPage::HtmlPage(std::string name, std::string path) HtmlService::HtmlService() = default; HtmlService::HtmlService(std::shared_ptr impl) - : m_impl{std::move(impl)} {} + : m_impl{std::move(impl)} { + static_cast(deref(m_impl)); +} -const HtmlConfig &HtmlService::config() const { return m_impl->config(); } +const HtmlConfig &HtmlService::config() const { return deref(m_impl).config(); } const HtmlViews &HtmlService::list_views() const { - return m_impl->list_views(); + return deref(m_impl).list_views(); } -void HtmlService::warmup() const { m_impl->warmup(); } +void HtmlService::warmup() const { deref(m_impl).warmup(); } bool HtmlService::exists(const std::string &path) const { - return m_impl->exists(path); + return deref(m_impl).exists(path); } std::string HtmlService::mimetype(const std::string &path) const { - return m_impl->mimetype(path); + return deref(m_impl).mimetype(path); } void HtmlService::write(const std::string &path, std::ostream &out) const { - m_impl->write(path, out); + deref(m_impl).write(path, out); } HtmlResources HtmlService::write_html(const std::string &path, std::ostream &out) const { internal::html::HtmlWriter writer(out, config()); - return m_impl->write_html(path, writer); + return deref(m_impl).write_html(path, writer); } Html HtmlService::bring_offline(const std::string &output_path) const { @@ -144,23 +153,25 @@ const std::shared_ptr &HtmlService::impl() const { HtmlView::HtmlView() = default; HtmlView::HtmlView(std::shared_ptr impl) - : m_impl{std::move(impl)} {} + : m_impl{std::move(impl)} { + static_cast(deref(m_impl)); +} -const std::string &HtmlView::name() const { return m_impl->name(); } +const std::string &HtmlView::name() const { return deref(m_impl).name(); } -std::size_t HtmlView::index() const { return m_impl->index(); } +std::size_t HtmlView::index() const { return deref(m_impl).index(); } -const std::string &HtmlView::path() const { return m_impl->path(); } +const std::string &HtmlView::path() const { return deref(m_impl).path(); } -const HtmlConfig &HtmlView::config() const { return m_impl->config(); } +const HtmlConfig &HtmlView::config() const { return deref(m_impl).config(); } const std::optional &HtmlView::sheet_cut() const { - return m_impl->sheet_cut(); + return deref(m_impl).sheet_cut(); } HtmlResources HtmlView::write_html(std::ostream &out) const { internal::html::HtmlWriter writer(out, config()); - return m_impl->write_html(writer); + return deref(m_impl).write_html(writer); } Html HtmlView::bring_offline(const std::string &output_path) const { @@ -186,28 +197,34 @@ const std::shared_ptr &HtmlView::impl() const { HtmlResource::HtmlResource() = default; HtmlResource::HtmlResource(std::shared_ptr impl) - : m_impl{std::move(impl)} {} + : m_impl{std::move(impl)} { + static_cast(deref(m_impl)); +} -HtmlResourceType HtmlResource::type() const { return m_impl->type(); } +HtmlResourceType HtmlResource::type() const { return deref(m_impl).type(); } const std::string &HtmlResource::mime_type() const { - return m_impl->mime_type(); + return deref(m_impl).mime_type(); } -const std::string &HtmlResource::name() const { return m_impl->name(); } +const std::string &HtmlResource::name() const { return deref(m_impl).name(); } -const std::string &HtmlResource::path() const { return m_impl->path(); } +const std::string &HtmlResource::path() const { return deref(m_impl).path(); } -const std::optional &HtmlResource::file() const { return m_impl->file(); } +const std::optional &HtmlResource::file() const { + return deref(m_impl).file(); +} -bool HtmlResource::is_shipped() const { return m_impl->is_shipped(); } +bool HtmlResource::is_shipped() const { return deref(m_impl).is_shipped(); } -bool HtmlResource::is_external() const { return m_impl->is_external(); } +bool HtmlResource::is_external() const { return deref(m_impl).is_external(); } -bool HtmlResource::is_accessible() const { return m_impl->is_accessible(); } +bool HtmlResource::is_accessible() const { + return deref(m_impl).is_accessible(); +} void HtmlResource::write_resource(std::ostream &os) const { - m_impl->write_resource(os); + deref(m_impl).write_resource(os); } namespace { diff --git a/src/odr/html.hpp b/src/odr/html.hpp index 827ba3a0a..251376644 100644 --- a/src/odr/html.hpp +++ b/src/odr/html.hpp @@ -43,6 +43,7 @@ enum class HtmlResourceType { /// font. @ref HtmlConfig decides which are embedded and which are linked. class HtmlResource final { public: + /// Empty handle; accessing it throws `NullPointerError`. HtmlResource(); explicit HtmlResource(std::shared_ptr impl); @@ -276,6 +277,7 @@ struct HtmlSheetCut final { /// needs. class HtmlView final { public: + /// Empty handle; accessing it throws `NullPointerError`. HtmlView(); explicit HtmlView(std::shared_ptr impl); @@ -306,6 +308,7 @@ using HtmlViews = std::vector; /// puts on a socket. class HtmlService final { public: + /// Empty handle; accessing it throws `NullPointerError`. HtmlService(); explicit HtmlService(std::shared_ptr impl); diff --git a/src/odr/internal/open_strategy.cpp b/src/odr/internal/open_strategy.cpp index 814dc84c6..de1c4dd7f 100644 --- a/src/odr/internal/open_strategy.cpp +++ b/src/odr/internal/open_strategy.cpp @@ -326,6 +326,9 @@ open_file_as(const std::shared_ptr &file, const FileType as, std::vector open_strategy::list_file_types(const std::shared_ptr &file, const Logger &logger) { + if (file == nullptr) { + throw NullPointerError("file"); + } std::vector result; auto file_type = magic::file_type(*file); @@ -659,6 +662,9 @@ open_by_probe(const std::shared_ptr &file, std::unique_ptr open_strategy::open_file(const std::shared_ptr &file, const DecodeOptions &options, const Logger &logger) { + if (file == nullptr) { + throw NullPointerError("file"); + } // A named type is not a probe with one candidate: there is nothing to move // on to, so the format's own "not a ..." reaches the caller rather than // being collapsed into UnknownFileType. diff --git a/test/src/file_test.cpp b/test/src/file_test.cpp index 7a4d919a3..e56f60e8f 100644 --- a/test/src/file_test.cpp +++ b/test/src/file_test.cpp @@ -250,6 +250,15 @@ TEST(File, default_constructed_reports_unknown_location) { EXPECT_THROW(std::ignore = file.stream(), NullPointerError); } +TEST(File, opening_or_probing_an_empty_handle_throws) { + const File file; + EXPECT_THROW(std::ignore = open(file), NullPointerError); + EXPECT_THROW(std::ignore = open(file, DecodeOptions::as(FileType::text_file)), + NullPointerError); + EXPECT_THROW(std::ignore = list_file_types(file), NullPointerError); + EXPECT_THROW(std::ignore = mimetype(file), NullPointerError); +} + TEST(File, disk_file_has_no_memory_data) { const File file(std::make_shared( TestData::test_file_path("odr-public/odt/about.odt"))); diff --git a/test/src/html_test.cpp b/test/src/html_test.cpp index ce099a318..de9a3a6d7 100644 --- a/test/src/html_test.cpp +++ b/test/src/html_test.cpp @@ -26,6 +26,15 @@ using namespace odr; using namespace odr::internal; using namespace odr::test; +TEST(html, empty_handles_throw_on_access) { + EXPECT_THROW(std::ignore = HtmlService().config(), NullPointerError); + EXPECT_THROW(std::ignore = HtmlView().name(), NullPointerError); + EXPECT_THROW(std::ignore = HtmlResource().type(), NullPointerError); + EXPECT_THROW(HtmlService(nullptr), NullPointerError); + EXPECT_THROW(HtmlView(nullptr), NullPointerError); + EXPECT_THROW(HtmlResource(nullptr), NullPointerError); +} + // A linked stylesheet is of no use to a host serving the service over http if // the service cannot answer for the path the markup names. TEST(html, linked_resources_are_served) { diff --git a/test/src/internal/odf/odf_flat_file_test.cpp b/test/src/internal/odf/odf_flat_file_test.cpp index bee7648c4..3fbf087e2 100644 --- a/test/src/internal/odf/odf_flat_file_test.cpp +++ b/test/src/internal/odf/odf_flat_file_test.cpp @@ -82,6 +82,20 @@ std::string packaged_text(const std::string &name, const std::string &body) { } // namespace +TEST(FlatOdf, missing_related_elements_return_empty_handles) { + const Document document = open(File::from_memory(flat_text(""))) + .as_document_file() + .document(); + EXPECT_FALSE(document.root_element().as_text_root().first_master_page()); + const Table table = + first_of_type(document.root_element(), ElementType::table).as_table(); + ASSERT_TRUE(table); + EXPECT_FALSE(table.first_row()); + EXPECT_FALSE(table.first_column()); + EXPECT_FALSE(document.element_by_id(null_element_id)); + EXPECT_THROW(document.remove(Element()), std::invalid_argument); +} + TEST(FlatOpenDocumentFile, the_root_mimetype_names_the_document_type) { const struct { const char *mimetype;