diff --git a/apple/include/OdrCoreObjC/ODRFile.h b/apple/include/OdrCoreObjC/ODRFile.h index 1d70afd63..337ded16f 100644 --- a/apple/include/OdrCoreObjC/ODRFile.h +++ b/apple/include/OdrCoreObjC/ODRFile.h @@ -97,7 +97,8 @@ typedef NS_ENUM(NSInteger, ODRFileCategory) { } NS_SWIFT_NAME(FileCategory); typedef NS_ENUM(NSInteger, ODRFileLocation) { - ODRFileLocationMemory = 0, + ODRFileLocationUnknown = 0, + ODRFileLocationMemory, ODRFileLocationDisk, } NS_SWIFT_NAME(FileLocation); diff --git a/apple/src/ODRFile.mm b/apple/src/ODRFile.mm index 7bdf264b9..f7167a1ec 100644 --- a/apple/src/ODRFile.mm +++ b/apple/src/ODRFile.mm @@ -100,6 +100,7 @@ ODR_SAME_ENUM(ODRFileCategoryAudio, odr::FileCategory::audio); ODR_SAME_ENUM(ODRFileCategoryVideo, odr::FileCategory::video); +ODR_SAME_ENUM(ODRFileLocationUnknown, odr::FileLocation::unknown); ODR_SAME_ENUM(ODRFileLocationMemory, odr::FileLocation::memory); ODR_SAME_ENUM(ODRFileLocationDisk, odr::FileLocation::disk); diff --git a/jni/java/app/opendocument/core/FileLocation.java b/jni/java/app/opendocument/core/FileLocation.java index caf3f73d6..b3fd4b4e3 100644 --- a/jni/java/app/opendocument/core/FileLocation.java +++ b/jni/java/app/opendocument/core/FileLocation.java @@ -2,7 +2,7 @@ /** Mirrors {@code odr::FileLocation}; constant order must match the C++ declaration. */ public enum FileLocation { - MEMORY, DISK; + UNKNOWN, MEMORY, DISK; static FileLocation fromNative(int code) { return code < 0 ? null : values()[code]; diff --git a/jni/src/jni_document.cpp b/jni/src/jni_document.cpp index 487cdc10f..6a51344c9 100644 --- a/jni/src/jni_document.cpp +++ b/jni/src/jni_document.cpp @@ -14,6 +14,7 @@ namespace { using odr_jni::destroy_handle; using odr_jni::from_handle; using odr_jni::guarded; +using odr_jni::HandleGuard; using odr_jni::make_handle; using odr_jni::to_jstring; using odr_jni::to_string; @@ -32,16 +33,18 @@ jlong wrap_element(odr::Element value) { } jlongArray wrap_elements(JNIEnv *env, const odr::ElementRange &range) { - std::vector handles; + HandleGuard guard; for (const odr::Element &value : range) { - handles.push_back(make_handle(odr::Element(value))); + guard.add(value); } + const std::vector &handles = guard.handles(); jlongArray result = env->NewLongArray(static_cast(handles.size())); if (result == nullptr) { return nullptr; } env->SetLongArrayRegion(result, 0, static_cast(handles.size()), handles.data()); + guard.release(); return result; } diff --git a/jni/src/jni_html.cpp b/jni/src/jni_html.cpp index a9fa42f8e..f4d282536 100644 --- a/jni/src/jni_html.cpp +++ b/jni/src/jni_html.cpp @@ -15,6 +15,7 @@ namespace { using odr_jni::destroy_handle; using odr_jni::from_handle; using odr_jni::guarded; +using odr_jni::HandleGuard; using odr_jni::make_handle; using odr_jni::to_jbytes; using odr_jni::to_jstring; @@ -31,12 +32,16 @@ jlongArray to_jlong_array(JNIEnv *env, const std::vector &values) { } jlongArray wrap_views(JNIEnv *env, const odr::HtmlViews &views) { - std::vector handles; - handles.reserve(views.size()); + HandleGuard guard(views.size()); for (const odr::HtmlView &view : views) { - handles.push_back(make_handle(odr::HtmlView(view))); + guard.add(view); } - return to_jlong_array(env, handles); + jlongArray result = to_jlong_array(env, guard.handles()); + if (result == nullptr) { + return nullptr; + } + guard.release(); + return result; } /// Builds an `app.opendocument.core.Html` from an offline result. @@ -49,9 +54,15 @@ jobject make_html(JNIEnv *env, odr::Html html) { } jmethodID page_ctor = env->GetMethodID( page_cls, "", "(Ljava/lang/String;Ljava/lang/String;)V"); + if (page_ctor == nullptr) { + return nullptr; + } const std::vector &pages = html.pages(); jobjectArray page_array = env->NewObjectArray(static_cast(pages.size()), page_cls, nullptr); + if (page_array == nullptr) { + return nullptr; + } for (jsize i = 0; i < static_cast(pages.size()); ++i) { jstring name = to_jstring(env, pages[i].name); jstring path = to_jstring(env, pages[i].path); @@ -70,6 +81,9 @@ jobject make_html(JNIEnv *env, odr::Html html) { jmethodID html_ctor = env->GetMethodID( html_cls, "", "(Lapp/opendocument/core/HtmlConfig;[Lapp/opendocument/core/HtmlPage;)V"); + if (html_ctor == nullptr) { + return nullptr; + } jobject result = env->NewObject(html_cls, html_ctor, config, page_array); env->DeleteLocalRef(html_cls); return result; @@ -87,15 +101,33 @@ jobject make_content(JNIEnv *env, const std::string &html, jmethodID located_ctor = env->GetMethodID( located_cls, "", "(Lapp/opendocument/core/HtmlResource;Ljava/lang/String;)V"); + if (located_ctor == nullptr) { + return nullptr; + } jclass resource_cls = env->FindClass("app/opendocument/core/HtmlResource"); + if (resource_cls == nullptr) { + return nullptr; + } jmethodID resource_ctor = env->GetMethodID(resource_cls, "", "(J)V"); + if (resource_ctor == nullptr) { + return nullptr; + } jobjectArray located_array = env->NewObjectArray( static_cast(resources.size()), located_cls, nullptr); + if (located_array == nullptr) { + return nullptr; + } for (jsize i = 0; i < static_cast(resources.size()); ++i) { const auto &[resource, location] = resources[i]; - jobject resource_obj = env->NewObject( - resource_cls, resource_ctor, make_handle(odr::HtmlResource(resource))); + HandleGuard guard(1); + jobject resource_obj = + env->NewObject(resource_cls, resource_ctor, guard.add(resource)); + if (resource_obj == nullptr) { + return nullptr; + } + // the Java wrapper exists, so its post-mortem destroyer owns the handle now + guard.release(); jstring location_str = location.has_value() ? to_jstring(env, *location) : nullptr; jobject located = @@ -117,6 +149,9 @@ jobject make_content(JNIEnv *env, const std::string &html, jmethodID content_ctor = env->GetMethodID( content_cls, "", "(Ljava/lang/String;[Lapp/opendocument/core/Html$LocatedResource;)V"); + if (content_ctor == nullptr) { + return nullptr; + } jobject result = env->NewObject(content_cls, content_ctor, to_jstring(env, html), located_array); env->DeleteLocalRef(content_cls); diff --git a/jni/src/jni_logger.cpp b/jni/src/jni_logger.cpp index 438af5fac..b35244c72 100644 --- a/jni/src/jni_logger.cpp +++ b/jni/src/jni_logger.cpp @@ -4,6 +4,7 @@ #include #include +#include #include #include @@ -150,10 +151,16 @@ class JavaLogger final : public odr::ILogger { clear_pending(env.get()); } + /// False when the constructor could not resolve `LoggerBridge`, which leaves + /// a Java exception pending. + [[nodiscard]] bool valid() const { + return m_sink != nullptr && m_bridge != nullptr && m_will_log != nullptr && + m_log != nullptr && m_flush != nullptr; + } + private: [[nodiscard]] bool usable(JNIEnv *env) const { - return env != nullptr && m_sink != nullptr && m_bridge != nullptr && - m_will_log != nullptr && m_log != nullptr && m_flush != nullptr; + return env != nullptr && valid(); } /// A logger must not derail the operation it is reporting on, so an exception @@ -196,8 +203,17 @@ Java_app_opendocument_core_Logger_createStdio(JNIEnv *env, jclass, jstring name, extern "C" JNIEXPORT jlong JNICALL Java_app_opendocument_core_Logger_createFromSink(JNIEnv *env, jclass, jobject sink) { - return guarded(env, [&] { - return make_handle(odr::Logger(std::make_shared(env, sink))); + return guarded(env, [&]() -> jlong { + auto logger = std::make_shared(env, sink); + if (!logger->valid()) { + // no handle is made, so the value the JVM discards while throwing cannot + // strand an odr::Logger + if (env->ExceptionCheck() == JNI_FALSE) { + throw std::runtime_error("could not bind the Java log sink"); + } + return 0; + } + return make_handle(odr::Logger(std::move(logger))); }); } diff --git a/jni/src/odr_jni.hpp b/jni/src/odr_jni.hpp index 0427d3a8c..88ca5420a 100644 --- a/jni/src/odr_jni.hpp +++ b/jni/src/odr_jni.hpp @@ -2,10 +2,12 @@ #include +#include #include #include #include #include +#include namespace odr_jni { @@ -50,4 +52,34 @@ template void destroy_handle(JNIEnv *env, jlong handle) { guarded(env, [&] { delete from_handle(handle); }); } +/// Owns the handles made for one Java allocation until `release` hands them +/// over. Without it a failing `NewLongArray`/`NewObject` strands the C++ copies +/// the handles point at. +template class HandleGuard { +public: + explicit HandleGuard(const std::size_t capacity = 0) { + m_handles.reserve(capacity); + } + + HandleGuard(const HandleGuard &) = delete; + HandleGuard &operator=(const HandleGuard &) = delete; + + ~HandleGuard() { + for (const jlong handle : m_handles) { + delete from_handle(handle); + } + } + + jlong add(T value) { + return m_handles.emplace_back(make_handle(std::move(value))); + } + + [[nodiscard]] const std::vector &handles() const { return m_handles; } + + void release() { m_handles.clear(); } + +private: + std::vector m_handles; +}; + } // namespace odr_jni diff --git a/python/src/bind_core.cpp b/python/src/bind_core.cpp index 56718059b..4a1277764 100644 --- a/python/src/bind_core.cpp +++ b/python/src/bind_core.cpp @@ -36,7 +36,10 @@ void odr_python::bind_core(py::module_ &m) { py::arg("path")); // Mirrors odr::Exception, so `except odr.Error` catches the whole library. - const py::exception error(m, "Error"); + // `register_exception`, not `py::exception`: the latter has no translator. + // Registered first so it is tried last - pybind11 reverses that order. + const py::exception &error = + py::register_exception(m, "Error", PyExc_RuntimeError); py::register_exception(m, "UnsupportedOperation", error); @@ -130,19 +133,22 @@ void odr_python::bind_functions(py::module_ &m) { py::arg("path"), py::arg("logger") = odr::Logger::null(), "Determine the MIME type of a file."); + // Decoding is long-running, so it must not hold the GIL. Re-entry is safe: a + // Python `ILogger` re-acquires it in the trampoline. m.def( "open", [](const std::string &path, const odr::Logger &logger) { return odr::open(path, logger); }, py::arg("path"), py::arg("logger") = odr::Logger::null(), - "Open and decode a file."); + py::call_guard(), "Open and decode a file."); m.def( "open", [](const std::string &path, const odr::FileType as, const odr::Logger &logger) { return odr::open(path, as, logger); }, py::arg("path"), py::arg("as_type"), py::arg("logger") = odr::Logger::null(), + py::call_guard(), "Open and decode a file as a specific file type."); m.def( "open", @@ -152,5 +158,6 @@ void odr_python::bind_functions(py::module_ &m) { }, py::arg("path"), py::arg("preference"), py::arg("logger") = odr::Logger::null(), + py::call_guard(), "Open and decode a file with a decode preference."); } diff --git a/python/src/bind_document.cpp b/python/src/bind_document.cpp index 3e4fbbf65..d596228ae 100644 --- a/python/src/bind_document.cpp +++ b/python/src/bind_document.cpp @@ -313,14 +313,17 @@ void odr_python::bind_document(py::module_ &m) { .def("is_editable", &odr::Document::is_editable) .def("is_savable", &odr::Document::is_savable, py::arg("encrypted") = false) + // saving serialises the whole document; holding the GIL for it blocks + // every other Python thread .def("save", py::overload_cast(&odr::Document::save, py::const_), - py::arg("path")) + py::arg("path"), py::call_guard()) .def("save", py::overload_cast( &odr::Document::save, py::const_), - py::arg("path"), py::arg("password")) + py::arg("path"), py::arg("password"), + py::call_guard()) .def("file_type", &odr::Document::file_type) .def("document_type", &odr::Document::document_type) .def("root_element", &odr::Document::root_element, keep_self_alive) diff --git a/python/src/bind_file.cpp b/python/src/bind_file.cpp index 0fb1230ce..1484eef6e 100644 --- a/python/src/bind_file.cpp +++ b/python/src/bind_file.cpp @@ -95,6 +95,7 @@ void odr_python::bind_file(py::module_ &m) { .value("video", odr::FileCategory::video); py::enum_(m, "FileLocation") + .value("unknown", odr::FileLocation::unknown) .value("memory", odr::FileLocation::memory) .value("disk", odr::FileLocation::disk); @@ -179,7 +180,10 @@ void odr_python::bind_file(py::module_ &m) { .def("file_meta", &odr::DecodedFile::file_meta) .def("password_encrypted", &odr::DecodedFile::password_encrypted) .def("encryption_state", &odr::DecodedFile::encryption_state) - .def("decrypt", &odr::DecodedFile::decrypt, py::arg("password")) + // decrypting rewrites the whole file; holding the GIL for it blocks + // every other Python thread + .def("decrypt", &odr::DecodedFile::decrypt, py::arg("password"), + py::call_guard()) .def("is_decodable", &odr::DecodedFile::is_decodable) .def("capabilities", &odr::DecodedFile::capabilities) .def("is_text_file", &odr::DecodedFile::is_text_file) @@ -214,11 +218,13 @@ void odr_python::bind_file(py::module_ &m) { .def_static("type_by_path", &odr::DocumentFile::type, py::arg("path")) .def_static("meta_by_path", &odr::DocumentFile::meta, py::arg("path")) .def("document_type", &odr::DocumentFile::document_type) - .def("decrypt", &odr::DocumentFile::decrypt, py::arg("password")) + .def("decrypt", &odr::DocumentFile::decrypt, py::arg("password"), + py::call_guard()) .def("document", &odr::DocumentFile::document); py::class_(m, "PdfFile") - .def("decrypt", &odr::PdfFile::decrypt, py::arg("password")); + .def("decrypt", &odr::PdfFile::decrypt, py::arg("password"), + py::call_guard()); py::class_(m, "FontFile") .def("read", [](const odr::FontFile &file) { diff --git a/python/src/bind_html.cpp b/python/src/bind_html.cpp index e6f00e0ae..e5487e004 100644 --- a/python/src/bind_html.cpp +++ b/python/src/bind_html.cpp @@ -127,9 +127,10 @@ void odr_python::bind_html(py::module_ &m) { auto resources = view.write_html(out); return std::make_pair(out.str(), std::move(resources)); }, + py::call_guard(), "Render this view; returns (html, resources).") .def("bring_offline", &odr::HtmlView::bring_offline, - py::arg("output_path")); + py::arg("output_path"), py::call_guard()); py::class_(m, "HtmlService") .def("config", &odr::HtmlService::config) @@ -146,14 +147,20 @@ void odr_python::bind_html(py::module_ &m) { } return views; }) - .def("warmup", &odr::HtmlService::warmup) + .def("warmup", &odr::HtmlService::warmup, + py::call_guard()) .def("exists", &odr::HtmlService::exists, py::arg("path")) .def("mimetype", &odr::HtmlService::mimetype, py::arg("path")) .def( "write", [](const odr::HtmlService &service, const std::string &path) { std::ostringstream out; - service.write(path, out); + { + // scoped rather than a `call_guard`: the `py::bytes` below needs + // the GIL back + const py::gil_scoped_release release; + service.write(path, out); + } return py::bytes(out.str()); }, py::arg("path")) @@ -164,21 +171,27 @@ void odr_python::bind_html(py::module_ &m) { auto resources = service.write_html(path, out); return std::make_pair(out.str(), std::move(resources)); }, - py::arg("path"), "Render one view path; returns (html, resources).") + py::arg("path"), py::call_guard(), + "Render one view path; returns (html, resources).") .def("bring_offline", py::overload_cast( &odr::HtmlService::bring_offline, py::const_), - py::arg("output_path")) + py::arg("output_path"), py::call_guard()) .def("bring_offline", py::overload_cast &>( &odr::HtmlService::bring_offline, py::const_), - py::arg("output_path"), py::arg("views")); + py::arg("output_path"), py::arg("views"), + py::call_guard()); auto html = m.def_submodule("html", "Translate decoded files to HTML."); html.def("standard_resource_locator", &odr::html::standard_resource_locator); + // Rendering is long-running, so it must not hold the GIL. Re-entry is safe: + // both ways back into Python - a `Logger` sink through the trampoline and a + // `resource_locator` through pybind11's `std::function` wrapper - acquire the + // GIL themselves. html.def( "translate", [](const odr::DecodedFile &file, const std::string &cache_path, @@ -187,6 +200,7 @@ void odr_python::bind_html(py::module_ &m) { }, py::arg("file"), py::arg("cache_path"), py::arg("config"), py::arg("logger") = odr::Logger::null(), + py::call_guard(), "Translate a decoded file to HTML."); html.def( "translate", @@ -195,7 +209,9 @@ void odr_python::bind_html(py::module_ &m) { return odr::html::translate(document, cache_path, config, logger); }, py::arg("document"), py::arg("cache_path"), py::arg("config"), - py::arg("logger") = odr::Logger::null(), "Translate a document to HTML."); + py::arg("logger") = odr::Logger::null(), + py::call_guard(), + "Translate a document to HTML."); html.def( "translate", [](const odr::Filesystem &filesystem, const std::string &cache_path, @@ -204,6 +220,7 @@ void odr_python::bind_html(py::module_ &m) { }, py::arg("filesystem"), py::arg("cache_path"), py::arg("config"), py::arg("logger") = odr::Logger::null(), + py::call_guard(), "Translate a filesystem to HTML."); html.def( diff --git a/python/tests/test_http_server.py b/python/tests/test_http_server.py index e7951843b..554a90940 100644 --- a/python/tests/test_http_server.py +++ b/python/tests/test_http_server.py @@ -55,7 +55,7 @@ def test_bind_reports_what_it_got(): assert port != 0 # a second bind would leak the first socket, so it is refused - with pytest.raises(RuntimeError): + with pytest.raises(pyodr.Error): server.bind("127.0.0.1", 0) server.stop() @@ -70,7 +70,7 @@ def test_bind_reports_a_port_in_use(): other = pyodr.HttpServer() # reuse_port defaults off, or the two would share the port - with pytest.raises(RuntimeError): + with pytest.raises(pyodr.Error): other.bind("127.0.0.1", port) taken.stop() @@ -81,7 +81,7 @@ def test_listen_without_bind_raises(): server = pyodr.HttpServer() # cpp-httplib reports success for this, hence the guard being tested - with pytest.raises(RuntimeError): + with pytest.raises(pyodr.Error): server.listen() diff --git a/src/odr/file.cpp b/src/odr/file.cpp index 085822ef1..73158568d 100644 --- a/src/odr/file.cpp +++ b/src/odr/file.cpp @@ -16,30 +16,53 @@ namespace odr { +namespace { + +/// The other constructors reject a null impl, so only a default-constructed +/// @ref File reaches the throw. +const internal::abstract::File & +deref(const std::shared_ptr &impl) { + if (impl == nullptr) { + throw NullPointerError("impl"); + } + return *impl; +} + +} // namespace + File::File() = default; File::File(std::shared_ptr impl) - : m_impl{std::move(impl)} {} + : m_impl{std::move(impl)} { + if (m_impl == nullptr) { + throw NullPointerError("impl"); + } +} File::File(const std::string &path) : m_impl{std::make_shared(path)} {} -FileLocation File::location() const noexcept { return m_impl->location(); } +/// `noexcept` leaves no way to report a null impl, hence `unknown`. +FileLocation File::location() const noexcept { + return m_impl == nullptr ? FileLocation::unknown : m_impl->location(); +} -std::size_t File::size() const { return m_impl->size(); } +std::size_t File::size() const { return deref(m_impl).size(); } std::optional File::disk_path() const { - if (const std::optional path = m_impl->disk_path()) { + if (const std::optional path = deref(m_impl).disk_path()) { return path->string(); } return {}; } std::optional File::memory_data() const { - return m_impl->memory_data(); + return deref(m_impl).memory_data(); } -std::unique_ptr File::stream() const { return m_impl->stream(); } +std::unique_ptr File::stream() const { + return deref(m_impl).stream(); +} void File::pipe(std::ostream &out) const { internal::util::stream::pipe(*stream(), out); diff --git a/src/odr/file.hpp b/src/odr/file.hpp index 5e3561d59..d4f3ad3f9 100644 --- a/src/odr/file.hpp +++ b/src/odr/file.hpp @@ -163,6 +163,7 @@ enum class FileCategory { /// @brief Collection of file locations. enum class FileLocation { + unknown, ///< no file behind the handle memory, disk, }; @@ -234,7 +235,10 @@ struct FileMeta final { /// @brief Represents a file. class File final { public: + /// Constructs the null file — every accessor but @ref location throws @ref + /// NullPointerError on it, so assign a real one before use. File(); + /// @throws NullPointerError if the impl is null. explicit File(std::shared_ptr); explicit File(const std::string &path); diff --git a/src/odr/html.cpp b/src/odr/html.cpp index 8341b9c68..76c67eae8 100644 --- a/src/odr/html.cpp +++ b/src/odr/html.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include @@ -124,12 +125,15 @@ Html HtmlService::bring_offline(const std::string &output_path, pages.emplace_back(view.name(), path.string()); } + // a resource shared by two views appears once per view, and those need not be + // adjacent - dedup by path, keeping the first occurrence and the order { - const auto it = - std::ranges::unique(resources, [](const auto &lhs, const auto &rhs) { - return lhs.first.path() == rhs.first.path(); - }).begin(); - resources.erase(it, resources.end()); + std::unordered_set seen; + const auto removed = + std::ranges::remove_if(resources, [&seen](const auto &resource) { + return !seen.insert(resource.first.path()).second; + }); + resources.erase(removed.begin(), removed.end()); } odr::bring_offline(resources, output_path); diff --git a/src/odr/internal/cfb/cfb_impl.cpp b/src/odr/internal/cfb/cfb_impl.cpp index 26375e91c..ec92ddfc9 100644 --- a/src/odr/internal/cfb/cfb_impl.cpp +++ b/src/odr/internal/cfb/cfb_impl.cpp @@ -59,7 +59,13 @@ CompoundFileReader::CompoundFileReader(std::istream &in, throw NoCfbFile(); } - m_sector_size = m_header.major_version == 3 ? 512 : 4096; + // [MS-CFB] 2.2: major version 3 or 4, pinned to sector shift 9 resp. 12; the + // sector size is 1 << sector_shift. + if (!(m_header.major_version == 3 && m_header.sector_shift == 9) && + !(m_header.major_version == 4 && m_header.sector_shift == 12)) { + throw CfbFileCorrupted(); + } + m_sector_size = std::uint64_t{1} << m_header.sector_shift; // The file must contain at least 3 sectors if (m_file_size < m_sector_size * 3) { diff --git a/src/odr/internal/common/filesystem.cpp b/src/odr/internal/common/filesystem.cpp index ac8e81e5a..bb64dc8ea 100644 --- a/src/odr/internal/common/filesystem.cpp +++ b/src/odr/internal/common/filesystem.cpp @@ -170,9 +170,16 @@ class VirtualFileWalker final : public abstract::FileWalker { return std::make_unique(*this); } + /// The iterators belong to different maps, so only the keys can be compared. [[nodiscard]] bool equals(const FileWalker &rhs_) const override { - auto &&rhs = dynamic_cast(rhs_); - return m_iterator == rhs.m_iterator; + const auto *rhs = dynamic_cast(&rhs_); + if (rhs == nullptr) { + return false; + } + if (end() || rhs->end()) { + return end() && rhs->end(); + } + return m_iterator->first == rhs->m_iterator->first; } [[nodiscard]] bool end() const override { diff --git a/src/odr/internal/common/temporary_file.cpp b/src/odr/internal/common/temporary_file.cpp index f7e173cca..7e16470dd 100644 --- a/src/odr/internal/common/temporary_file.cpp +++ b/src/odr/internal/common/temporary_file.cpp @@ -9,6 +9,15 @@ namespace odr::internal { +namespace { + +void remove_quietly(const AbsPath &path) { + std::error_code error_code; + std::filesystem::remove(path.string(), error_code); +} + +} // namespace + TemporaryDiskFile::TemporaryDiskFile(const char *path) : DiskFile{path} {} TemporaryDiskFile::TemporaryDiskFile(const std::string &path) @@ -17,21 +26,31 @@ TemporaryDiskFile::TemporaryDiskFile(const std::string &path) TemporaryDiskFile::TemporaryDiskFile(AbsPath path) : DiskFile{std::move(path)} {} -TemporaryDiskFile::TemporaryDiskFile(const TemporaryDiskFile &) = default; - -TemporaryDiskFile::TemporaryDiskFile(TemporaryDiskFile &&) noexcept = default; +TemporaryDiskFile::TemporaryDiskFile(TemporaryDiskFile &&other) noexcept + : DiskFile{std::move(other)}, + m_owns_path{std::exchange(other.m_owns_path, false)} {} TemporaryDiskFile::~TemporaryDiskFile() { + if (!m_owns_path) { + return; + } assert(disk_path().has_value()); - std::error_code ec; - std::filesystem::remove(disk_path()->string(), ec); + remove_quietly(*disk_path()); } TemporaryDiskFile & -TemporaryDiskFile::operator=(const TemporaryDiskFile &) = default; - -TemporaryDiskFile & -TemporaryDiskFile::operator=(TemporaryDiskFile &&) noexcept = default; +TemporaryDiskFile::operator=(TemporaryDiskFile &&other) noexcept { + if (this == &other) { + return *this; + } + if (m_owns_path) { + assert(disk_path().has_value()); + remove_quietly(*disk_path()); + } + DiskFile::operator=(std::move(other)); + m_owns_path = std::exchange(other.m_owns_path, false); + return *this; +} const TemporaryDiskFileFactory &TemporaryDiskFileFactory::system_default() { static TemporaryDiskFileFactory instance( diff --git a/src/odr/internal/common/temporary_file.hpp b/src/odr/internal/common/temporary_file.hpp index 5d4a3df01..f93dff545 100644 --- a/src/odr/internal/common/temporary_file.hpp +++ b/src/odr/internal/common/temporary_file.hpp @@ -7,16 +7,20 @@ namespace odr::internal { +/// Owns the path it names and removes it on destruction, hence move-only. class TemporaryDiskFile final : public DiskFile { public: explicit TemporaryDiskFile(const char *path); explicit TemporaryDiskFile(const std::string &path); explicit TemporaryDiskFile(AbsPath path); - TemporaryDiskFile(const TemporaryDiskFile &); + TemporaryDiskFile(const TemporaryDiskFile &) = delete; TemporaryDiskFile(TemporaryDiskFile &&) noexcept; ~TemporaryDiskFile() override; - TemporaryDiskFile &operator=(const TemporaryDiskFile &); + TemporaryDiskFile &operator=(const TemporaryDiskFile &) = delete; TemporaryDiskFile &operator=(TemporaryDiskFile &&) noexcept; + +private: + bool m_owns_path{true}; ///< cleared by a move, so only one owner removes }; class TemporaryDiskFileFactory final { diff --git a/src/odr/internal/crypto/crypto_util.cpp b/src/odr/internal/crypto/crypto_util.cpp index 1588a1c70..00bb4e61d 100644 --- a/src/odr/internal/crypto/crypto_util.cpp +++ b/src/odr/internal/crypto/crypto_util.cpp @@ -31,7 +31,7 @@ namespace odr::internal::crypto { using byte = std::uint8_t; -std::string util::base64_encode(const std::string &in) { +std::string util::base64_encode(const std::string_view in) { std::string out; CryptoPP::Base64Encoder b(new CryptoPP::StringSink(out), false); b.Put(reinterpret_cast(in.data()), in.size()); @@ -39,7 +39,7 @@ std::string util::base64_encode(const std::string &in) { return out; } -std::string util::base64_decode(const std::string &in) { +std::string util::base64_decode(const std::string_view in) { std::string out; CryptoPP::Base64Decoder b(new CryptoPP::StringSink(out)); b.Put(reinterpret_cast(in.data()), in.size()); @@ -47,7 +47,7 @@ std::string util::base64_decode(const std::string &in) { return out; } -std::string util::hex_encode(const std::string &in) { +std::string util::hex_encode(const std::string_view in) { std::string out; CryptoPP::HexEncoder e(new CryptoPP::StringSink(out), false); e.Put(reinterpret_cast(in.data()), in.size()); @@ -55,7 +55,7 @@ std::string util::hex_encode(const std::string &in) { return out; } -std::string util::hex_decode(const std::string &in) { +std::string util::hex_decode(const std::string_view in) { if (in.size() % 2 != 0) { throw std::invalid_argument("hex_decode: odd number of digits"); } @@ -82,42 +82,43 @@ std::uint32_t util::crc32(const std::string_view in) { return value; } -std::string util::md5(const std::string &in) { +std::string util::md5(const std::string_view in) { std::array out; CryptoPP::Weak::MD5().CalculateDigest( out.data(), reinterpret_cast(in.data()), in.size()); return {reinterpret_cast(out.data()), out.size()}; } -std::string util::sha1(const std::string &in) { +std::string util::sha1(const std::string_view in) { std::array out; CryptoPP::SHA1().CalculateDigest( out.data(), reinterpret_cast(in.data()), in.size()); return {reinterpret_cast(out.data()), out.size()}; } -std::string util::sha256(const std::string &in) { +std::string util::sha256(const std::string_view in) { std::array out; CryptoPP::SHA256().CalculateDigest( out.data(), reinterpret_cast(in.data()), in.size()); return {reinterpret_cast(out.data()), out.size()}; } -std::string util::sha384(const std::string &in) { +std::string util::sha384(const std::string_view in) { std::array out; CryptoPP::SHA384().CalculateDigest( out.data(), reinterpret_cast(in.data()), in.size()); return {reinterpret_cast(out.data()), out.size()}; } -std::string util::sha512(const std::string &in) { +std::string util::sha512(const std::string_view in) { std::array out; CryptoPP::SHA512().CalculateDigest( out.data(), reinterpret_cast(in.data()), in.size()); return {reinterpret_cast(out.data()), out.size()}; } -std::string util::rc4(const std::string &key, const std::string &input) { +std::string util::rc4(const std::string_view key, + const std::string_view input) { std::string result(input.size(), '\0'); CryptoPP::Weak::ARC4 rc4(reinterpret_cast(key.data()), key.size()); @@ -127,7 +128,8 @@ std::string util::rc4(const std::string &key, const std::string &input) { } std::string util::pbkdf2(const std::size_t key_size, - const std::string &start_key, const std::string &salt, + const std::string_view start_key, + const std::string_view salt, const std::size_t iteration_count) { std::string result(key_size, '\0'); const CryptoPP::PKCS5_PBKDF2_HMAC pbkdf2; @@ -140,15 +142,15 @@ std::string util::pbkdf2(const std::size_t key_size, } std::string util::argon2id(const std::size_t key_size, - const std::string &start_key, - const std::string &salt, + const std::string_view start_key, + const std::string_view salt, const std::size_t iteration_count, const std::size_t memory, const std::size_t lanes) { return argon2::id(key_size, start_key, salt, iteration_count, memory, lanes); } -std::string util::decrypt_aes_ecb(const std::string &key, - const std::string &input) { +std::string util::decrypt_aes_ecb(const std::string_view key, + const std::string_view input) { std::string result(input.size(), '\0'); CryptoPP::ECB_Mode::Decryption decryption; decryption.SetKey(reinterpret_cast(key.data()), key.size()); @@ -158,8 +160,9 @@ std::string util::decrypt_aes_ecb(const std::string &key, return result; } -std::string util::decrypt_aes_cbc(const std::string &key, const std::string &iv, - const std::string &input) { +std::string util::decrypt_aes_cbc(const std::string_view key, + const std::string_view iv, + const std::string_view input) { std::string result(input.size(), '\0'); CryptoPP::CBC_Mode::Decryption decryption; decryption.SetKeyWithIV(reinterpret_cast(key.data()), @@ -171,8 +174,9 @@ std::string util::decrypt_aes_cbc(const std::string &key, const std::string &iv, return result; } -std::string util::encrypt_aes_cbc(const std::string &key, const std::string &iv, - const std::string &input) { +std::string util::encrypt_aes_cbc(const std::string_view key, + const std::string_view iv, + const std::string_view input) { std::string result(input.size(), '\0'); CryptoPP::CBC_Mode::Encryption encryption; encryption.SetKeyWithIV(reinterpret_cast(key.data()), @@ -184,8 +188,9 @@ std::string util::encrypt_aes_cbc(const std::string &key, const std::string &iv, return result; } -std::string util::decrypt_aes_gcm(const std::string &key, const std::string &iv, - const std::string &input) { +std::string util::decrypt_aes_gcm(const std::string_view key, + const std::string_view iv, + const std::string_view input) { // follows https://www.w3.org/TR/xmlenc-core1/#sec-AES-GCM const std::size_t iv_size = iv.size(); @@ -224,9 +229,9 @@ std::string util::decrypt_aes_gcm(const std::string &key, const std::string &iv, return result; } -std::string util::decrypt_triple_des(const std::string &key, - const std::string &iv, - const std::string &input) { +std::string util::decrypt_triple_des(const std::string_view key, + const std::string_view iv, + const std::string_view input) { std::string result(input.size(), '\0'); CryptoPP::CBC_Mode::Decryption decryption; decryption.SetKeyWithIV(reinterpret_cast(key.data()), @@ -238,9 +243,9 @@ std::string util::decrypt_triple_des(const std::string &key, return result; } -std::string util::decrypt_blowfish(const std::string &key, - const std::string &iv, - const std::string &input) { +std::string util::decrypt_blowfish(const std::string_view key, + const std::string_view iv, + const std::string_view input) { std::string result(input.size(), '\0'); CryptoPP::CFB_Mode::Decryption decryption; decryption.SetKeyWithIV(reinterpret_cast(key.data()), @@ -272,7 +277,7 @@ class MyInflator final : public CryptoPP::Inflator { }; } // namespace -std::string util::inflate(const std::string &input) { +std::string util::inflate(const std::string_view input) { std::string result; MyInflator inflator(new CryptoPP::StringSink(result)); inflator.Put(reinterpret_cast(input.data()), input.size()); @@ -280,14 +285,14 @@ std::string util::inflate(const std::string &input) { return result; } -std::size_t util::padding(const std::string &input) { +std::size_t util::padding(const std::string_view input) { MyInflator inflator; inflator.Put(reinterpret_cast(input.data()), input.size()); inflator.MessageEnd(); return inflator.GetPadding(); } -std::string util::zlib_inflate(const std::string &input) { +std::string util::zlib_inflate(const std::string_view input) { std::string result; CryptoPP::ZlibDecompressor inflator(new CryptoPP::StringSink(result)); inflator.Put(reinterpret_cast(input.data()), input.size()); @@ -295,7 +300,7 @@ std::string util::zlib_inflate(const std::string &input) { return result; } -std::string util::zlib_deflate(const std::string &input) { +std::string util::zlib_deflate(const std::string_view input) { std::string out; CryptoPP::ZlibCompressor compressor(new CryptoPP::StringSink(out)); compressor.Put(reinterpret_cast(input.data()), diff --git a/src/odr/internal/crypto/crypto_util.hpp b/src/odr/internal/crypto/crypto_util.hpp index 20b335989..b6b681f55 100644 --- a/src/odr/internal/crypto/crypto_util.hpp +++ b/src/odr/internal/crypto/crypto_util.hpp @@ -8,50 +8,50 @@ namespace odr::internal::crypto::util { -std::string base64_encode(const std::string &); -std::string base64_decode(const std::string &); +std::string base64_encode(std::string_view); +std::string base64_decode(std::string_view); -std::string hex_encode(const std::string &); -std::string hex_decode(const std::string &); +std::string hex_encode(std::string_view); +std::string hex_decode(std::string_view); /// CRC-32 (ISO 3309 / PNG Annex D, polynomial 0xEDB88320). std::uint32_t crc32(std::string_view input); -std::string md5(const std::string &); -std::string sha1(const std::string &); -std::string sha256(const std::string &); -std::string sha384(const std::string &); -std::string sha512(const std::string &); +std::string md5(std::string_view); +std::string sha1(std::string_view); +std::string sha256(std::string_view); +std::string sha384(std::string_view); +std::string sha512(std::string_view); /// RC4 stream cipher; symmetric, so the same call encrypts and decrypts. -std::string rc4(const std::string &key, const std::string &input); +std::string rc4(std::string_view key, std::string_view input); -std::string pbkdf2(std::size_t key_size, const std::string &start_key, - const std::string &salt, std::size_t iteration_count); -std::string argon2id(std::size_t key_size, const std::string &start_key, - const std::string &salt, std::size_t iteration_count, +std::string pbkdf2(std::size_t key_size, std::string_view start_key, + std::string_view salt, std::size_t iteration_count); +std::string argon2id(std::size_t key_size, std::string_view start_key, + std::string_view salt, std::size_t iteration_count, std::size_t memory, std::size_t lanes); -std::string decrypt_aes_ecb(const std::string &key, const std::string &input); -std::string decrypt_aes_cbc(const std::string &key, const std::string &iv, - const std::string &input); +std::string decrypt_aes_ecb(std::string_view key, std::string_view input); +std::string decrypt_aes_cbc(std::string_view key, std::string_view iv, + std::string_view input); /// Raw AES-CBC encryption, no padding (`input` must be a multiple of the block /// size). Needed by the PDF R 6 hardened-hash algorithm (ISO 32000-2 2.B). -std::string encrypt_aes_cbc(const std::string &key, const std::string &iv, - const std::string &input); +std::string encrypt_aes_cbc(std::string_view key, std::string_view iv, + std::string_view input); /// AES-GCM per XML Encryption 1.1 §5.2.4: @p input is `iv || ciphertext || /// 16-byte tag` and must repeat @p iv. Throws if it does not, if @p input is /// too short to hold both, or if the tag fails to verify. -std::string decrypt_aes_gcm(const std::string &key, const std::string &iv, - const std::string &input); -std::string decrypt_triple_des(const std::string &key, const std::string &iv, - const std::string &input); -std::string decrypt_blowfish(const std::string &key, const std::string &iv, - const std::string &input); - -std::string inflate(const std::string &input); -std::size_t padding(const std::string &input); - -std::string zlib_inflate(const std::string &input); -std::string zlib_deflate(const std::string &input); +std::string decrypt_aes_gcm(std::string_view key, std::string_view iv, + std::string_view input); +std::string decrypt_triple_des(std::string_view key, std::string_view iv, + std::string_view input); +std::string decrypt_blowfish(std::string_view key, std::string_view iv, + std::string_view input); + +std::string inflate(std::string_view input); +std::size_t padding(std::string_view input); + +std::string zlib_inflate(std::string_view input); +std::string zlib_deflate(std::string_view input); } // namespace odr::internal::crypto::util diff --git a/src/odr/internal/font/sfnt_transform.cpp b/src/odr/internal/font/sfnt_transform.cpp index 1b192bc3a..e0abb0591 100644 --- a/src/odr/internal/font/sfnt_transform.cpp +++ b/src/odr/internal/font/sfnt_transform.cpp @@ -58,6 +58,9 @@ struct SearchHints { std::uint16_t range_shift; }; SearchHints search_hints(const std::uint16_t count, const std::uint16_t unit) { + if (count == 0) { + return {0, 0, 0}; // log2(0) is undefined; an empty directory has no range + } const auto entry_selector = static_cast(std::bit_width(count) - 1); const auto search_range = diff --git a/src/odr/internal/oldms/presentation/ppt_style.cpp b/src/odr/internal/oldms/presentation/ppt_style.cpp index dbe8ef4bf..eb83f4f1d 100644 --- a/src/odr/internal/oldms/presentation/ppt_style.cpp +++ b/src/odr/internal/oldms/presentation/ppt_style.cpp @@ -1,41 +1,14 @@ #include -#include -#include +#include + #include namespace odr::internal::oldms::presentation { namespace { -/// Bounds-checked reader over an in-memory record body. -class BodyCursor final { -public: - explicit BodyCursor(const std::string_view body) : m_body(body) {} - - [[nodiscard]] std::size_t remaining() const { return m_body.size() - m_at; } - - template T read() { - if (remaining() < sizeof(T)) { - throw std::runtime_error("ppt: truncated StyleTextPropAtom"); - } - T value; - std::memcpy(&value, m_body.data() + m_at, sizeof(T)); - m_at += sizeof(T); - return value; - } - - void skip(const std::size_t count) { - if (remaining() < count) { - throw std::runtime_error("ppt: truncated StyleTextPropAtom"); - } - m_at += count; - } - -private: - std::string_view m_body; - std::size_t m_at{0}; -}; +using BodyCursor = util::byte_string::Reader; /// PFMasks bits ([MS-PPT] 2.9.21). enum PFMask : std::uint32_t { diff --git a/src/odr/internal/ooxml/ooxml_crypto.cpp b/src/odr/internal/ooxml/ooxml_crypto.cpp index ee68aef9a..66b312dd0 100644 --- a/src/odr/internal/ooxml/ooxml_crypto.cpp +++ b/src/odr/internal/ooxml/ooxml_crypto.cpp @@ -3,10 +3,11 @@ #include #include +#include #include #include -#include +#include #include #include @@ -19,27 +20,35 @@ ECMA376Standard::ECMA376Standard(const EncryptionHeader &encryption_header, m_encryption_verifier{encryption_verifier}, m_encrypted_verifier_hash{std::move(encrypted_verifier_hash)} {} -ECMA376Standard::ECMA376Standard(const std::string &encryption_info) { - const char *offset = encryption_info.data() + sizeof(VersionInfo); +ECMA376Standard::ECMA376Standard(const std::string_view encryption_info) { + util::byte_string::Reader reader(encryption_info); + reader.seek(sizeof(VersionInfo)); StandardHeader standard_header{}; - std::memcpy(&standard_header, offset, sizeof(standard_header)); - offset += sizeof(standard_header); + reader.read(standard_header); - std::memcpy(&m_encryption_header, offset, sizeof(m_encryption_header)); - // the trailing CSP name is skipped; it is not needed to derive the key - offset += standard_header.encryption_header_size; + // [MS-OFFCRYPTO] 2.3.4.5: `encryption_header_size` spans the header plus the + // trailing CSP name, which is not needed to derive the key. + if (standard_header.encryption_header_size < sizeof(EncryptionHeader)) { + throw std::runtime_error("bad ooxml crypto header size"); + } + const std::size_t encryption_header_begin = reader.position(); + reader.read(m_encryption_header); + reader.seek(encryption_header_begin + standard_header.encryption_header_size); - std::memcpy(&m_encryption_verifier, offset, sizeof(m_encryption_verifier)); - offset += sizeof(m_encryption_verifier); + reader.read(m_encryption_verifier); - m_encrypted_verifier_hash = std::string( - offset, encryption_info.size() - (offset - encryption_info.data())); + m_encrypted_verifier_hash = reader.rest(); } -std::string ECMA376Standard::derive_key(const std::string &password) const { +std::string ECMA376Standard::derive_key(const std::string_view password) const { // https://msdn.microsoft.com/en-us/library/dd925430(v=office.12).aspx + // [MS-OFFCRYPTO] 2.3.3: `salt_size` is fixed at the size of the salt field. + if (m_encryption_verifier.salt_size != sizeof(m_encryption_verifier.salt)) { + throw std::runtime_error("bad ooxml crypto salt size"); + } + std::string hash; { const std::u16string password_u16 = @@ -84,12 +93,12 @@ std::string ECMA376Standard::derive_key(const std::string &password) const { return result; } -bool ECMA376Standard::verify(const std::string &key) const { +bool ECMA376Standard::verify(const std::string_view key) const { // https://msdn.microsoft.com/en-us/library/dd926426(v=office.12).aspx const std::string verifier = internal::crypto::util::decrypt_aes_ecb( - key, std::string(m_encryption_verifier.encrypted_verifier, - sizeof(m_encryption_verifier.encrypted_verifier))); + key, std::string_view(m_encryption_verifier.encrypted_verifier, + sizeof(m_encryption_verifier.encrypted_verifier))); const std::string hash = internal::crypto::util::sha1(verifier); const std::string verifier_hash = internal::crypto::util::decrypt_aes_ecb(key, m_encrypted_verifier_hash) @@ -98,18 +107,17 @@ bool ECMA376Standard::verify(const std::string &key) const { return hash == verifier_hash; } -std::string ECMA376Standard::decrypt(const std::string &encrypted_package, - const std::string &key) const { - const std::uint64_t total_size = - *reinterpret_cast(encrypted_package.data()); - std::string result = - internal::crypto::util::decrypt_aes_ecb(key, encrypted_package.substr(8)) - .substr(0, total_size); +std::string ECMA376Standard::decrypt(const std::string_view encrypted_package, + const std::string_view key) const { + util::byte_string::Reader reader(encrypted_package); + // [MS-OFFCRYPTO] 2.3.4.4: the stream opens with the plaintext size. + const auto total_size = reader.read(); - return result; + return internal::crypto::util::decrypt_aes_ecb(key, reader.rest()) + .substr(0, total_size); } -Util::Util(const std::string &encryption_info) { +Util::Util(const std::string_view encryption_info) { { // big endian is not supported constexpr std::uint16_t num = 1; @@ -118,8 +126,8 @@ Util::Util(const std::string &encryption_info) { } } - VersionInfo version_info{}; - std::memcpy(&version_info, encryption_info.data(), sizeof(version_info)); + util::byte_string::Reader reader(encryption_info); + const auto version_info = reader.read(); if ((version_info.major == 2 || version_info.major == 3 || version_info.major == 4) && version_info.minor == 2) { @@ -132,14 +140,16 @@ Util::Util(const std::string &encryption_info) { Util::~Util() = default; -std::string Util::derive_key(const std::string &password) const { +std::string Util::derive_key(const std::string_view password) const { return impl->derive_key(password); } -bool Util::verify(const std::string &key) const { return impl->verify(key); } +bool Util::verify(const std::string_view key) const { + return impl->verify(key); +} -std::string Util::decrypt(const std::string &encrypted_package, - const std::string &key) const { +std::string Util::decrypt(const std::string_view encrypted_package, + const std::string_view key) const { return impl->decrypt(encrypted_package, key); } diff --git a/src/odr/internal/ooxml/ooxml_crypto.hpp b/src/odr/internal/ooxml/ooxml_crypto.hpp index e677a10fc..df9680f2a 100644 --- a/src/odr/internal/ooxml/ooxml_crypto.hpp +++ b/src/odr/internal/ooxml/ooxml_crypto.hpp @@ -1,7 +1,9 @@ #pragma once +#include #include #include +#include namespace odr::internal::ooxml::crypto { @@ -48,24 +50,23 @@ class Algorithm { public: virtual ~Algorithm() = default; [[nodiscard]] virtual std::string - derive_key(const std::string &password) const = 0; - [[nodiscard]] virtual bool verify(const std::string &key) const = 0; - [[nodiscard]] virtual std::string - decrypt(const std::string &encrypted_package, - const std::string &key) const = 0; + derive_key(std::string_view password) const = 0; + [[nodiscard]] virtual bool verify(std::string_view key) const = 0; + [[nodiscard]] virtual std::string decrypt(std::string_view encrypted_package, + std::string_view key) const = 0; }; class ECMA376Standard final : public Algorithm { public: ECMA376Standard(const EncryptionHeader &, const EncryptionVerifier &, std::string encrypted_verifier_hash); - explicit ECMA376Standard(const std::string &encryption_info); + explicit ECMA376Standard(std::string_view encryption_info); [[nodiscard]] std::string - derive_key(const std::string &password) const override; - [[nodiscard]] bool verify(const std::string &key) const override; - [[nodiscard]] std::string decrypt(const std::string &encrypted_package, - const std::string &key) const override; + derive_key(std::string_view password) const override; + [[nodiscard]] bool verify(std::string_view key) const override; + [[nodiscard]] std::string decrypt(std::string_view encrypted_package, + std::string_view key) const override; private: static constexpr auto ITER_COUNT = 50000; @@ -77,14 +78,14 @@ class ECMA376Standard final : public Algorithm { class Util final : public Algorithm { public: - explicit Util(const std::string &encryption_info); + explicit Util(std::string_view encryption_info); ~Util() override; [[nodiscard]] std::string - derive_key(const std::string &password) const override; - [[nodiscard]] bool verify(const std::string &key) const override; - [[nodiscard]] std::string decrypt(const std::string &encrypted_package, - const std::string &key) const override; + derive_key(std::string_view password) const override; + [[nodiscard]] bool verify(std::string_view key) const override; + [[nodiscard]] std::string decrypt(std::string_view encrypted_package, + std::string_view key) const override; private: std::unique_ptr impl; diff --git a/src/odr/internal/open_strategy.cpp b/src/odr/internal/open_strategy.cpp index 0398187f0..04dcb1ed3 100644 --- a/src/odr/internal/open_strategy.cpp +++ b/src/odr/internal/open_strategy.cpp @@ -213,44 +213,54 @@ open_strategy::list_file_types(const std::shared_ptr &file, if (file_type == FileType::zip) { ODR_VERBOSE(logger, "open as zip"); - zip::ZipFile zip_file(file); - result.push_back(FileType::zip); + // a container the magic promised but that does not open is just another + // failed probe here — the callers degrade on an empty result + try { + zip::ZipFile zip_file(file); + result.push_back(FileType::zip); - auto filesystem = zip_file.archive()->as_filesystem(); + auto filesystem = zip_file.archive()->as_filesystem(); - try { - ODR_VERBOSE(logger, "try open as odf"); - result.push_back(odf::OpenDocumentFile(filesystem).file_type()); - } catch (...) { - ODR_VERBOSE(logger, "failed to open as odf"); - } + try { + ODR_VERBOSE(logger, "try open as odf"); + result.push_back(odf::OpenDocumentFile(filesystem).file_type()); + } catch (...) { + ODR_VERBOSE(logger, "failed to open as odf"); + } - try { - ODR_VERBOSE(logger, "try open as ooxml"); - result.push_back(ooxml::OfficeOpenXmlFile(filesystem).file_type()); + try { + ODR_VERBOSE(logger, "try open as ooxml"); + result.push_back(ooxml::OfficeOpenXmlFile(filesystem).file_type()); + } catch (...) { + ODR_VERBOSE(logger, "failed to open as ooxml"); + } } catch (...) { - ODR_VERBOSE(logger, "failed to open as ooxml"); + ODR_VERBOSE(logger, "failed to open as zip"); } } else if (file_type == FileType::compound_file_binary_format) { ODR_VERBOSE(logger, "open as cbf"); - cfb::CfbFile cfb_file(file); - result.push_back(FileType::compound_file_binary_format); + try { + cfb::CfbFile cfb_file(file); + result.push_back(FileType::compound_file_binary_format); - auto filesystem = cfb_file.archive()->as_filesystem(); + auto filesystem = cfb_file.archive()->as_filesystem(); - try { - ODR_VERBOSE(logger, "try open as legacy ms"); - result.push_back(oldms::LegacyMicrosoftFile(filesystem).file_type()); - } catch (...) { - ODR_VERBOSE(logger, "failed to open as legacy ms"); - } + try { + ODR_VERBOSE(logger, "try open as legacy ms"); + result.push_back(oldms::LegacyMicrosoftFile(filesystem).file_type()); + } catch (...) { + ODR_VERBOSE(logger, "failed to open as legacy ms"); + } - try { - ODR_VERBOSE(logger, "try open as ooxml"); - result.push_back(ooxml::OfficeOpenXmlFile(filesystem).file_type()); + try { + ODR_VERBOSE(logger, "try open as ooxml"); + result.push_back(ooxml::OfficeOpenXmlFile(filesystem).file_type()); + } catch (...) { + ODR_VERBOSE(logger, "failed to open as ooxml"); + } } catch (...) { - ODR_VERBOSE(logger, "failed to open as ooxml"); + ODR_VERBOSE(logger, "failed to open as cfb"); } } else if (file_type == FileType::starview_metafile) { try { diff --git a/src/odr/internal/pdf/pdf_document_parser.cpp b/src/odr/internal/pdf/pdf_document_parser.cpp index 1baa00903..9449b065d 100644 --- a/src/odr/internal/pdf/pdf_document_parser.cpp +++ b/src/odr/internal/pdf/pdf_document_parser.cpp @@ -592,66 +592,90 @@ void parse_type3_font(State &state, const Dictionary &dictionary, Font &font) { font.type3 = std::move(type3); } -Font *parse_font(State &state, const ObjectReference &reference) { - // Shared fonts are parsed once; every page referencing the same font object - // resolves to the one element so the HTML writer inlines it a single time. - if (Font *cached = state.find_font(reference); cached != nullptr) { - return cached; - } - +/// Parse a font dictionary onto `font` — everything but the identity and +/// memoization, which only the indirect form has. +void parse_font_dictionary(State &state, const Dictionary &dictionary, + Font &font) { DocumentParser &parser = state.parser(); - Document &document = state.document(); - Font *font = document.create_element(); - state.cache_font(reference, font); - - IndirectObject object = parser.read_object(reference); - const Dictionary &dictionary = object.object.as_dictionary(); - - font->object_reference = reference; - font->object = Object(dictionary); + font.object = Object(dictionary); const bool is_type0 = dictionary.get("Subtype").is_name() && dictionary["Subtype"].as_name() == "Type0"; const bool is_type3 = dictionary.get("Subtype").is_name() && dictionary["Subtype"].as_name() == "Type3"; - if (dictionary.has_key("ToUnicode")) { + // `/ToUnicode` is a stream, hence always an indirect reference (ISO 32000-1 + // 7.3.8); anything else (`null`, a name) carries no CMap. + if (const Object &to_unicode = dictionary.get("ToUnicode"); + to_unicode.is_reference()) { const std::string stream = - parser.read_decoded_stream(dictionary["ToUnicode"].as_reference()); + parser.read_decoded_stream(to_unicode.as_reference()); util::stream::ViewStream ss(stream); CMapParser cmap_parser(ss, parser.logger()); - font->cmap = cmap_parser.parse_cmap(); + font.cmap = cmap_parser.parse_cmap(); } if (is_type0) { // Composite (Type0) font: the `/Encoding` is a code -> CID CMap, not a // simple-font glyph-name encoding, so it must not go through // `parse_encoding`. Extraction relies on `/ToUnicode` (parsed above). - parse_composite_font(parser, dictionary, *font); + parse_composite_font(parser, dictionary, font); } else { - parse_simple_font_widths(parser, dictionary, *font); + parse_simple_font_widths(parser, dictionary, font); if (dictionary.has_key("Encoding")) { // Simple-font `/Encoding`: a base-encoding name, or a dictionary with // `/BaseEncoding` + `/Differences`. The text-extraction fallback for // fonts without a `ToUnicode` CMap. Type3 fonts map codes to their // `/CharProcs` glyph names through the same `/Differences` mechanism. - font->encoding = parse_encoding(parser, dictionary["Encoding"]); + font.encoding = parse_encoding(parser, dictionary["Encoding"]); } if (is_type3) { // Type3 glyphs are drawn by their char procs, not substituted; the // widths parsed above are in glyph space (scaled by `/FontMatrix`). - parse_type3_font(state, dictionary, *font); + parse_type3_font(state, dictionary, font); } else { // Non-embedded simple fonts render in a substitute family with AFM // widths. - resolve_font_substitute(parser, dictionary, *font); + resolve_font_substitute(parser, dictionary, font); } } +} + +Font *parse_font(State &state, const ObjectReference &reference) { + // Shared fonts are parsed once; every page referencing the same font object + // resolves to the one element so the HTML writer inlines it a single time. + if (Font *cached = state.find_font(reference); cached != nullptr) { + return cached; + } + + DocumentParser &parser = state.parser(); + + Font *font = state.document().create_element(); + state.cache_font(reference, font); + + IndirectObject object = parser.read_object(reference); + font->object_reference = reference; + parse_font_dictionary(state, object.object.as_dictionary(), *font); return font; } +/// A `/Font` resource entry (ISO 32000-1 7.8.3). Normally an indirect +/// reference, so the element is shared; a direct font dictionary is legal too +/// and is parsed in place, unshared. Null for anything else. +Font *parse_font(State &state, const Object &object) { + if (object.is_reference()) { + return parse_font(state, object.as_reference()); + } + if (!object.is_dictionary()) { + return nullptr; + } + Font *font = state.document().create_element(); + parse_font_dictionary(state, object.as_dictionary(), *font); + return font; +} + Element *parse_page_or_pages(State &state, const ObjectReference &reference, Pages *parent, const PageAttributes &inherited); @@ -904,6 +928,23 @@ XObject *parse_x_object(State &state, const ObjectReference &reference, return x_object; } +/// An `/XObject` resource entry (ISO 32000-1 8.8). An XObject is a stream and +/// streams are always indirect (7.3.8), so a direct value carries no content: +/// keep its dictionary inspectable but leave the element inexecutable rather +/// than failing the document. Null for anything but a reference or dictionary. +XObject *parse_x_object(State &state, const Object &object, + const Resources *resources) { + if (object.is_reference()) { + return parse_x_object(state, object.as_reference(), resources); + } + if (!object.is_dictionary()) { + return nullptr; + } + auto *x_object = state.document().create_element(); + x_object->object = object; + return x_object; +} + /// A `ColorSpaceContext` over the parser, resolving a base/alternate space /// named by name against the (being-built) `/ColorSpace` table of `resources`. ColorSpaceContext make_color_space_context(DocumentParser &parser, @@ -1044,7 +1085,9 @@ Resources *parse_resources(State &state, const Object &object) { const Dictionary font_table = parser.resolve_object_copy(dictionary["Font"]).as_dictionary(); for (const auto &[key, value] : font_table) { - resources->font[key] = parse_font(state, value.as_reference()); + if (Font *font = parse_font(state, value); font != nullptr) { + resources->font[key] = font; + } } } @@ -1082,8 +1125,10 @@ Resources *parse_resources(State &state, const Object &object) { const Dictionary x_object_table = parser.resolve_object_copy(dictionary["XObject"]).as_dictionary(); for (const auto &[key, value] : x_object_table) { - resources->x_object[key] = - parse_x_object(state, value.as_reference(), resources); + if (XObject *x_object = parse_x_object(state, value, resources); + x_object != nullptr) { + resources->x_object[key] = x_object; + } } } @@ -1440,6 +1485,12 @@ DocumentParser::load_object_stream(const ObjectReference &reference) { return it->second; } + const ObjectStreamScope scope(*this, reference); + if (!scope.entered()) { + throw std::runtime_error("cyclic object stream reference " + + reference.to_string()); + } + const IndirectObject &object = read_object(reference); if (!object.has_stream) { throw std::runtime_error("object stream " + reference.to_string() + @@ -1458,6 +1509,25 @@ DocumentParser::load_object_stream(const ObjectReference &reference) { .first->second; } +bool DocumentParser::enter_object_stream(const ObjectReference &reference) { + return m_active_object_streams.insert(reference).second; +} + +void DocumentParser::leave_object_stream(const ObjectReference &reference) { + m_active_object_streams.erase(reference); +} + +DocumentParser::ObjectStreamScope::ObjectStreamScope( + DocumentParser &parser, const ObjectReference &reference) + : m_parser{&parser}, m_reference{reference}, + m_entered{parser.enter_object_stream(reference)} {} + +DocumentParser::ObjectStreamScope::~ObjectStreamScope() { + if (m_entered) { + m_parser->leave_object_stream(m_reference); + } +} + std::string DocumentParser::read_object_stream(const ObjectReference &reference) { return read_object_stream(read_object(reference)); diff --git a/src/odr/internal/pdf/pdf_document_parser.hpp b/src/odr/internal/pdf/pdf_document_parser.hpp index d3f602363..e148d8ab1 100644 --- a/src/odr/internal/pdf/pdf_document_parser.hpp +++ b/src/odr/internal/pdf/pdf_document_parser.hpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -115,6 +116,31 @@ class DocumentParser { [[nodiscard]] const ObjectStream & load_object_stream(const ObjectReference &reference); + /// The object streams currently being loaded. Loading one resolves its + /// `/Length`, `/N` and `/First`, and a file may put those in an object + /// compressed inside that very stream; the cache fills only on completion, so + /// without this the resolution recurses. `enter_object_stream` returns false + /// on re-entry. + [[nodiscard]] bool enter_object_stream(const ObjectReference &reference); + void leave_object_stream(const ObjectReference &reference); + + /// RAII counterpart of `enter_object_stream` / `leave_object_stream`. + class ObjectStreamScope final { + public: + ObjectStreamScope(DocumentParser &parser, const ObjectReference &reference); + ~ObjectStreamScope(); + + ObjectStreamScope(const ObjectStreamScope &) = delete; + ObjectStreamScope &operator=(const ObjectStreamScope &) = delete; + + [[nodiscard]] bool entered() const { return m_entered; } + + private: + DocumentParser *m_parser{nullptr}; + ObjectReference m_reference; + bool m_entered{false}; + }; + std::unique_ptr m_stream; FileParser m_parser; Logger m_logger; @@ -129,6 +155,7 @@ class DocumentParser { std::map m_objects; std::map m_object_streams; + std::set m_active_object_streams; }; } // namespace odr::internal::pdf diff --git a/src/odr/internal/pdf/pdf_function.cpp b/src/odr/internal/pdf/pdf_function.cpp index 21acde231..1cd2138c2 100644 --- a/src/odr/internal/pdf/pdf_function.cpp +++ b/src/odr/internal/pdf/pdf_function.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -227,6 +228,27 @@ class SampledFunction final : public Function { // --- type 4: PostScript calculator (ISO 32000-1 7.10.5) -------------------- +/// The integer operand of `idiv`/`mod`/the bitwise operators. Every type-4 +/// value is held as a `double`, and converting one outside the destination +/// range is undefined behaviour, so saturate to the 32-bit signed range a +/// PostScript integer occupies and map NaN to zero. +std::int32_t to_int32(const double v) { + constexpr auto lowest = + static_cast(std::numeric_limits::min()); + constexpr auto highest = + static_cast(std::numeric_limits::max()); + if (std::isnan(v)) { + return 0; + } + return static_cast(std::clamp(v, lowest, highest)); +} + +/// Whether `x / y` (and `x % y`) is undefined in C++: a zero divisor, or the +/// non-representable `INT32_MIN / -1`. +bool is_undefined_division(const std::int32_t x, const std::int32_t y) { + return y == 0 || (y == -1 && x == std::numeric_limits::min()); +} + /// One token of a type-4 program: a literal number, an operator name, or a /// nested `{ ... }` procedure block (used by `if`/`ifelse`). struct PostScriptItem { @@ -326,19 +348,21 @@ class PostScriptFunction final : public Function { } else if (op == "div") { binary([](double a, double b) { return b == 0 ? 0.0 : a / b; }); } else if (op == "idiv") { - binary([](double a, double b) { - if (b == 0) { + binary([](const double a, const double b) { + const std::int32_t x = to_int32(a); + const std::int32_t y = to_int32(b); + // A zero divisor already yielded 0 before; INT32_MIN / -1 joins it. + if (is_undefined_division(x, y)) { return 0.0; } // NOLINTNEXTLINE(bugprone-integer-division): idiv is integer division - return static_cast(static_cast(a) / - static_cast(b)); + return static_cast(x / y); }); } else if (op == "mod") { - binary([](double a, double b) { - return b == 0 ? 0.0 - : static_cast(static_cast(a) % - static_cast(b)); + binary([](const double a, const double b) { + const std::int32_t x = to_int32(a); + const std::int32_t y = to_int32(b); + return is_undefined_division(x, y) ? 0.0 : static_cast(x % y); }); } else if (op == "neg") { unary([](double a) { return -a; }); @@ -403,11 +427,11 @@ class PostScriptFunction final : public Function { if (a == 0.0 || a == 1.0) { s.emplace_back(a == 0.0 ? 1.0 : 0.0); } else { - s.emplace_back(static_cast(~static_cast(a))); + s.emplace_back(static_cast(~to_int32(a))); } } else if (op == "bitshift") { const double shift = pop_number(s); - const auto value = static_cast(pop_number(s)); + const std::int32_t value = to_int32(pop_number(s)); // Shifting a 32-bit value by 32 or more is undefined in C++; PostScript // shifts every bit out. (The comparison also catches a NaN shift.) const double magnitude = std::abs(shift); @@ -476,8 +500,7 @@ class PostScriptFunction final : public Function { if (boolean) { s.emplace_back(bool_op(a != 0.0, b != 0.0) ? 1.0 : 0.0); } else { - s.emplace_back(static_cast( - int_op(static_cast(a), static_cast(b)))); + s.emplace_back(static_cast(int_op(to_int32(a), to_int32(b)))); } } diff --git a/src/odr/internal/pdf/pdf_graphics_state.cpp b/src/odr/internal/pdf/pdf_graphics_state.cpp index fe9f4d0f6..1ecdaea37 100644 --- a/src/odr/internal/pdf/pdf_graphics_state.cpp +++ b/src/odr/internal/pdf/pdf_graphics_state.cpp @@ -172,12 +172,28 @@ void GraphicsState::save() { stack.push_back(stack.back()); } void GraphicsState::restore() { // A `Q` without a matching `q` is malformed (ISO 32000-1 8.4.4) and real - // files emit it; keep the initial state so `current()` stays valid. - if (stack.size() > 1) { + // files emit it; keep the running stream's entry state so `current()` stays + // valid and no enclosing stream's state is popped away. + if (stack.size() > m_restore_floor) { stack.pop_back(); } } +GraphicsState::ContentScope::ContentScope(GraphicsState &state) + : m_state{&state}, m_depth{state.stack.size()}, + m_floor{state.m_restore_floor} { + state.save(); + state.m_restore_floor = state.stack.size(); +} + +GraphicsState::ContentScope::~ContentScope() { + // Only ever shrinks: the floor kept every `Q` above `m_depth`. + while (m_state->stack.size() > m_depth) { + m_state->stack.pop_back(); + } + m_state->m_restore_floor = m_floor; +} + void GraphicsState::concat_matrix(const util::math::Transform2D &matrix) { // CTM = matrix * CTM (ISO 32000-1 8.4.4). current().general.transform_matrix = diff --git a/src/odr/internal/pdf/pdf_graphics_state.hpp b/src/odr/internal/pdf/pdf_graphics_state.hpp index fc97207dc..9ed5a974f 100644 --- a/src/odr/internal/pdf/pdf_graphics_state.hpp +++ b/src/odr/internal/pdf/pdf_graphics_state.hpp @@ -3,6 +3,7 @@ #include #include +#include #include #include #include @@ -174,7 +175,8 @@ struct GraphicsState { /// `q`: push a copy of the current state. void save(); - /// `Q`: pop it. An unmatched `Q` is ignored — the base state always remains. + /// `Q`: pop it. An unmatched `Q` is ignored — the state the running content + /// stream started from always remains. void restore(); /// `CTM = matrix * CTM`, as `cm` and form invocation do. void concat_matrix(const util::math::Transform2D &matrix); @@ -188,6 +190,26 @@ struct GraphicsState { /// unaffected. void advance_text(double tx, double ty); + /// Scopes a nested content stream (form XObject, Type3 char proc): pushes a + /// state like `q` and pops back to exactly the entry depth on destruction, + /// while confining the stream's own `q`/`Q` to that region. Content streams + /// are required to be q/Q-balanced (ISO 32000-1 8.10.1) and real files are + /// not, so an unbalanced stream must neither leak a state to nor pop one off + /// its caller. + class ContentScope final { + public: + explicit ContentScope(GraphicsState &state); + ~ContentScope(); + + ContentScope(const ContentScope &) = delete; + ContentScope &operator=(const ContentScope &) = delete; + + private: + GraphicsState *m_state{nullptr}; + std::size_t m_depth{0}; + std::size_t m_floor{0}; + }; + private: /// `Tlm = translate(tx, ty) * Tlm`, `Tm = Tlm` — behind /// `Td`/`TD`/`T*`/`'`/`"`. @@ -205,6 +227,9 @@ struct GraphicsState { /// by a painting/`n` operator before any `q`/`Q` (ISO 32000-1 8.5.4). enum class PendingClip { none, nonzero, even_odd }; PendingClip m_pending_clip{PendingClip::none}; + + /// Lowest stack size a `Q` may pop to; raised by `ContentScope`. + std::size_t m_restore_floor{1}; }; } // namespace odr::internal::pdf diff --git a/src/odr/internal/pdf/pdf_page_extractor.cpp b/src/odr/internal/pdf/pdf_page_extractor.cpp index 4d2a7229a..d4014327f 100644 --- a/src/odr/internal/pdf/pdf_page_extractor.cpp +++ b/src/odr/internal/pdf/pdf_page_extractor.cpp @@ -612,7 +612,7 @@ void begin_marked_content(const GraphicsOperator &op, } /// Invoke the XObject `Do` names (ISO 32000-1 8.10.1): an image emits an -/// `ImageElement`; a form runs inside a `save`/`restore` with its `/Matrix` +/// `ImageElement`; a form runs inside a `ContentScope` with its `/Matrix` /// concatenated, clipped to its `/BBox` and scoped to its own `/Resources`. /// Unknown subtypes and forms already on the render stack are skipped. void invoke_x_object(const std::string &name, const Resources &resources, @@ -684,38 +684,41 @@ void invoke_x_object(const std::string &name, const Resources &resources, const bool group_unit = group && (group_alpha < 1.0 || !group_blend.empty() || group_mask); - state.save(); - state.concat_matrix(x_object->matrix); - if (group_unit) { - // Isolate the group-level parameters so interior paintings are relative to - // the group; they are applied once to the composited group (below), not - // inherited by each interior element as well. - GraphicsState::General &inner = state.current().general; - inner.fill_alpha = 1.0; - inner.stroke_alpha = 1.0; - inner.blend_mode.clear(); - inner.soft_mask = nullptr; - } - // `/BBox` clips the form's content to its bounding box (ISO 32000-1 8.10.2), - // mapped through the (now form-matrix-concatenated) CTM. Scoped by the - // surrounding save/restore. - if (x_object->bbox.has_value()) { - const std::array &b = *x_object->bbox; - state.clip_bounding_box(b[0], b[1], b[2], b[3]); - } - const Resources &scope = - x_object->resources != nullptr ? *x_object->resources : resources; - // A form's marked content must be self-balanced; truncate back to the entry - // depth afterwards so an unbalanced form cannot corrupt the enclosing scope. - const std::size_t depth = marked.size(); // A group painted as a unit collects its content into its own child list; a // plain form appends straight to the page stream. std::vector group_out; - std::vector &sink = group_unit ? group_out : out; - run_content(x_object->content, scope, state, sink, logger, warned, active, - marked, pen); - marked.resize(depth); - state.restore(); + { + // Scoped like `q`/`Q`, but pinned: the form's own `q`/`Q` cannot escape it. + const GraphicsState::ContentScope content_scope(state); + state.concat_matrix(x_object->matrix); + if (group_unit) { + // Isolate the group-level parameters so interior paintings are relative + // to the group; they are applied once to the composited group (below), + // not inherited by each interior element as well. + GraphicsState::General &inner = state.current().general; + inner.fill_alpha = 1.0; + inner.stroke_alpha = 1.0; + inner.blend_mode.clear(); + inner.soft_mask = nullptr; + } + // `/BBox` clips the form's content to its bounding box (ISO 32000-1 + // 8.10.2), mapped through the (now form-matrix-concatenated) CTM. Scoped by + // the surrounding content scope. + if (x_object->bbox.has_value()) { + const std::array &b = *x_object->bbox; + state.clip_bounding_box(b[0], b[1], b[2], b[3]); + } + const Resources &scope = + x_object->resources != nullptr ? *x_object->resources : resources; + // A form's marked content must be self-balanced; truncate back to the + // entry depth afterwards so an unbalanced form cannot corrupt the + // enclosing scope. + const std::size_t depth = marked.size(); + std::vector &sink = group_unit ? group_out : out; + run_content(x_object->content, scope, state, sink, logger, warned, active, + marked, pen); + marked.resize(depth); + } if (group_unit) { auto children = std::make_shared(); @@ -854,14 +857,15 @@ void show_type3(std::vector &out, const Resources &resources, ? *font->type3->resources : resources; - state.save(); + // Scoped like `q`/`Q`, but pinned: the char proc's own `q`/`Q` cannot + // escape it and disturb the text state driving the glyph loop. + const GraphicsState::ContentScope content_scope(state); state.current().general.transform_matrix = glyph_to_user; const std::size_t marked_depth = marked.size(); std::optional inner_pen; run_content(it->second, scope, state, out, logger, warned, active, marked, inner_pen); marked.resize(marked_depth); - state.restore(); } state.advance_text(advance, 0); } diff --git a/src/odr/internal/svm/svm_format.cpp b/src/odr/internal/svm/svm_format.cpp index cd198f3d4..de9fc7c5c 100644 --- a/src/odr/internal/svm/svm_format.cpp +++ b/src/odr/internal/svm/svm_format.cpp @@ -2,26 +2,39 @@ #include +#include #include #include #include #include +#include namespace odr::internal { +namespace { + +std::string read_bytes(std::istream &in, const std::uint64_t size) { + try { + return util::byte_stream::read_u8s(in, size); + } catch (const std::runtime_error &) { + throw MalformedSvmFile(); + } +} + +} // namespace + std::string svm::read_ascii_string(std::istream &in, const std::uint32_t length) { - std::string result(length, ' '); - in.read(result.data(), static_cast(result.size())); - return result; + return read_bytes(in, length); } std::string svm::read_utf16_string(std::istream &in, const std::uint32_t length) { - std::u16string result_u16(length, ' '); - in.read(reinterpret_cast(result_u16.data()), - static_cast(length) * 2); + const std::string bytes = + read_bytes(in, static_cast(length) * 2); + std::u16string result_u16(length, u' '); + std::memcpy(result_u16.data(), bytes.data(), bytes.size()); return util::string::u16string_to_string(result_u16); } @@ -310,9 +323,12 @@ svm::TextArrayAction svm::read_text_array_action(std::istream &in, read_primitive(in, result.length); std::uint32_t dx_array_length; read_primitive(in, dx_array_length); - result.dx_array.resize(dx_array_length); + // grown entry by entry: the declared length is only trustworthy as far as the + // stream actually reaches for (std::uint32_t i = 0; i < dx_array_length; ++i) { - read_primitive(in, result.dx_array[i]); + std::uint32_t dx; + read_primitive(in, dx); + result.dx_array.push_back(dx); } if (vl.version >= 2) { diff --git a/src/odr/internal/svm/svm_format.hpp b/src/odr/internal/svm/svm_format.hpp index 18d512b5e..043914672 100644 --- a/src/odr/internal/svm/svm_format.hpp +++ b/src/odr/internal/svm/svm_format.hpp @@ -1,5 +1,7 @@ #pragma once +#include + #include #include #include @@ -204,8 +206,12 @@ struct TextLineAction final { std::uint32_t overline{}; }; +/// Reads a fixed-size field. A short read leaves the destination untouched, so +/// the stream ending mid-field is malformed input rather than a stale value. template void read_primitive(std::istream &in, T &out) { - in.read(reinterpret_cast(&out), sizeof(out)); + if (!in.read(reinterpret_cast(&out), sizeof(out))) { + throw MalformedSvmFile(); + } } std::string read_ascii_string(std::istream &in, std::uint32_t length); diff --git a/src/odr/internal/util/byte_stream_util.cpp b/src/odr/internal/util/byte_stream_util.cpp index 6a12db648..fba64f92a 100644 --- a/src/odr/internal/util/byte_stream_util.cpp +++ b/src/odr/internal/util/byte_stream_util.cpp @@ -2,10 +2,21 @@ #include +#include #include +#include namespace odr::internal::util { +namespace { + +/// The one failure every read here reports. +[[noreturn]] void throw_exhausted() { + throw std::runtime_error("byte_stream: unexpected stream exhaust"); +} + +} // namespace + bool byte_stream::try_read(std::istream &in, char *out, std::size_t count) { while (count > 0) { in.read(out, static_cast(count)); @@ -21,7 +32,7 @@ bool byte_stream::try_read(std::istream &in, char *out, std::size_t count) { void byte_stream::read(std::istream &in, char *out, std::size_t count) { if (!try_read(in, out, count)) { - throw std::runtime_error("byte_stream: failed to read from stream"); + throw_exhausted(); } } @@ -29,16 +40,21 @@ std::uint8_t byte_stream::read_u8(std::istream &in) { const auto c = in.rdbuf()->sbumpc(); if (c == eof) { in.setstate(std::ios::eofbit); - throw std::runtime_error("unexpected stream exhaust"); + throw_exhausted(); } return static_cast(c); } -std::string byte_stream::read_u8s(std::istream &in, const std::size_t n) { - std::string result(n, '\0'); - if (const auto m = static_cast(n); - in.rdbuf()->sgetn(result.data(), m) != m) { - throw std::runtime_error("unexpected stream exhaust"); +std::string byte_stream::read_u8s(std::istream &in, const std::uint64_t n) { + constexpr std::uint64_t chunk_size = 4096; + + std::string result; + while (result.size() < n) { + const std::size_t offset = result.size(); + const auto step = + static_cast(std::min(chunk_size, n - offset)); + result.resize(offset + step); + read(in, result.data() + offset, step); } return result; } diff --git a/src/odr/internal/util/byte_stream_util.hpp b/src/odr/internal/util/byte_stream_util.hpp index 1cd896be7..a49db9305 100644 --- a/src/odr/internal/util/byte_stream_util.hpp +++ b/src/odr/internal/util/byte_stream_util.hpp @@ -5,7 +5,6 @@ #include #include #include -#include #include #include @@ -47,13 +46,13 @@ std::uint8_t read_u8(std::istream &in); template std::array read_u8s(std::istream &in) { std::array result{}; - if (in.rdbuf()->sgetn(result.data(), N) != static_cast(N)) { - throw std::runtime_error("unexpected stream exhaust"); - } + read(in, result.data(), N); return result; } -std::string read_u8s(std::istream &in, std::size_t n); +/// Reads exactly @p n bytes, growing the buffer as the data arrives so that a +/// bogus length prefix cannot allocate ahead of the stream. +std::string read_u8s(std::istream &in, std::uint64_t n); std::uint16_t read_u16_le(std::istream &in); std::uint32_t read_u32_le(std::istream &in); diff --git a/src/odr/internal/util/byte_string.cpp b/src/odr/internal/util/byte_string.cpp index 45f9696bf..eae3c622e 100644 --- a/src/odr/internal/util/byte_string.cpp +++ b/src/odr/internal/util/byte_string.cpp @@ -2,6 +2,7 @@ #include +#include #include namespace odr::internal::util { @@ -109,4 +110,24 @@ void byte_string::write_u32_be(std::string &out, const std::size_t pos, out[pos + 3] = static_cast(value & 0xff); } +void byte_string::Reader::skip(const std::size_t count) { + if (count > remaining()) { + throw std::runtime_error("byte_string: read past end"); + } + m_position += count; +} + +void byte_string::Reader::seek(const std::size_t position) { + if (position > m_data.size()) { + throw std::runtime_error("byte_string: seek past end"); + } + m_position = position; +} + +void byte_string::Reader::read_bytes(void *const out, const std::size_t size) { + const std::size_t at = m_position; + skip(size); + std::memcpy(out, m_data.data() + at, size); +} + } // namespace odr::internal::util diff --git a/src/odr/internal/util/byte_string.hpp b/src/odr/internal/util/byte_string.hpp index 3ee5c1716..11729b0ef 100644 --- a/src/odr/internal/util/byte_string.hpp +++ b/src/odr/internal/util/byte_string.hpp @@ -4,6 +4,7 @@ #include #include #include +#include namespace odr::internal::util::byte_string { @@ -41,4 +42,42 @@ void put_u32_be(std::string &out, std::uint32_t value); void write_u16_be(std::string &out, std::size_t pos, std::uint16_t value); void write_u32_be(std::string &out, std::size_t pos, std::uint32_t value); +/// A bounds-checked cursor over an in-memory byte range. Reads copy the bytes +/// out, so packed structs are read without an unaligned dereference. Does not +/// own the range. +class Reader final { +public: + explicit Reader(const std::string_view data) noexcept : m_data{data} {} + + template void read(T &out) { + static_assert(std::is_trivially_copyable_v); + read_bytes(&out, sizeof(T)); + } + + template [[nodiscard]] T read() { + T out{}; + read(out); + return out; + } + + void skip(std::size_t count); + /// Jumps to @p position, which must lie inside the range. + void seek(std::size_t position); + + [[nodiscard]] std::size_t position() const noexcept { return m_position; } + [[nodiscard]] std::size_t remaining() const noexcept { + return m_data.size() - m_position; + } + /// The bytes from the cursor to the end; leaves the cursor where it is. + [[nodiscard]] std::string_view rest() const noexcept { + return m_data.substr(m_position); + } + +private: + void read_bytes(void *out, std::size_t size); + + std::string_view m_data; + std::size_t m_position{0}; +}; + } // namespace odr::internal::util::byte_string diff --git a/src/odr/internal/util/document_util.cpp b/src/odr/internal/util/document_util.cpp index e8ca856d2..58ed0fb06 100644 --- a/src/odr/internal/util/document_util.cpp +++ b/src/odr/internal/util/document_util.cpp @@ -88,6 +88,12 @@ document::extract_path(const abstract::ElementAdapter &element_adapter, const ElementIdentifier parent_id = element_adapter.element_parent(current_id); if (parent_id == null_element_id) { + // the walk reached the root; a named origin we never met is not an + // ancestor, and the path collected so far would be rooted elsewhere + if (from_element_id != null_element_id) { + throw std::invalid_argument( + "Element is not a descendant of the specified root."); + } break; } diff --git a/src/odr/internal/util/document_util.hpp b/src/odr/internal/util/document_util.hpp index e4d6321b6..c49196504 100644 --- a/src/odr/internal/util/document_util.hpp +++ b/src/odr/internal/util/document_util.hpp @@ -12,6 +12,8 @@ class ElementAdapter; namespace odr::internal::util::document { +/// The path from @p from_element_id (`null_element_id` for the root) down to +/// @p to_element_id; throws if the latter is not a descendant of the former. DocumentPath extract_path(const abstract::ElementAdapter &element_adapter, ElementIdentifier to_element_id, ElementIdentifier from_element_id); diff --git a/src/odr/internal/util/string_util.cpp b/src/odr/internal/util/string_util.cpp index bde1b6bff..1fa0f33a1 100644 --- a/src/odr/internal/util/string_util.cpp +++ b/src/odr/internal/util/string_util.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include @@ -96,6 +97,11 @@ std::string string::repeat(const std::string &unit, const std::size_t count) { void string::split(const std::string &string, const std::string &delimiter, const std::function &callback) { + // an empty delimiter never advances the scan, so it would loop forever + if (delimiter.empty()) { + throw std::invalid_argument("delimiter must not be empty"); + } + std::size_t last_end = 0; while (true) { const std::size_t pos = string.find(delimiter, last_end); @@ -130,7 +136,7 @@ std::string string::u16string_to_string(const std::u16string &string) { return utf8::utf16to8(string); } -std::u16string string::string_to_u16string(const std::string &string) { +std::u16string string::string_to_u16string(const std::string_view string) { return utf8::utf8to16(string); } diff --git a/src/odr/internal/util/string_util.hpp b/src/odr/internal/util/string_util.hpp index 3965c4ac5..b3b6f0a58 100644 --- a/src/odr/internal/util/string_util.hpp +++ b/src/odr/internal/util/string_util.hpp @@ -42,6 +42,8 @@ void replace_all(std::string &string, const std::string &search, /// Concatenate `count` copies of `unit` (empty for `count == 0`). std::string repeat(const std::string &unit, std::size_t count); +/// Splits on every occurrence of @p delimiter; throws `std::invalid_argument` +/// if it is empty. void split(const std::string &string, const std::string &delimiter, const std::function &callback); std::vector split(const std::string &string, @@ -52,7 +54,7 @@ std::string to_string(double d, int precision); std::size_t utf8_length(const std::string &string); std::string u16string_to_string(const std::u16string &string); -std::u16string string_to_u16string(const std::string &string); +std::u16string string_to_u16string(std::string_view string); /// @p length is a byte count, not a number of code units. std::string c16str_to_string(const char16_t *c16str, std::size_t length); void append_c32(char32_t c, std::string &string); diff --git a/src/odr/logger.cpp b/src/odr/logger.cpp index ffc537e53..d81c4f77a 100644 --- a/src/odr/logger.cpp +++ b/src/odr/logger.cpp @@ -1,6 +1,7 @@ #include #include +#include #include #include #include @@ -96,6 +97,18 @@ std::shared_ptr null_impl() { return instance; } +/// `std::localtime` hands out a pointer into one shared `std::tm`, which two +/// logging threads would race on. +std::tm local_time(const std::time_t time) { + std::tm result{}; +#ifdef _WIN32 + localtime_s(&result, &time); +#else + localtime_r(&time, &result); +#endif + return result; +} + /// Writes @p text clipped to @p width, padded to it, and one trailing space. void print_padded(std::ostream &out, const std::string_view text, const std::size_t width) { @@ -165,8 +178,8 @@ void Logger::print_head(std::ostream &out, Time time, LogLevel level, const std::source_location &location, const LogFormat &format) { if (!format.time_format.empty()) { - auto t = Clock::to_time_t(time); - out << std::put_time(std::localtime(&t), format.time_format.c_str()) << " "; + const std::tm local = local_time(Clock::to_time_t(time)); + out << std::put_time(&local, format.time_format.c_str()) << " "; } if (format.level_width > 0) { diff --git a/src/odr/quantity.cpp b/src/odr/quantity.cpp index 638b800d9..6bf41b1d1 100644 --- a/src/odr/quantity.cpp +++ b/src/odr/quantity.cpp @@ -2,7 +2,10 @@ #include +#include #include +#include +#include #include namespace odr { @@ -25,6 +28,15 @@ class DynamicUnit::Registry final { } private: + /// Transparent, so the lookup path does not have to allocate a key. + struct Hash final { + using is_transparent = void; + + std::size_t operator()(const std::string_view name) const noexcept { + return std::hash{}(name); + } + }; + static Registry ®istry_() { static Registry registry; return registry; @@ -32,9 +44,23 @@ class DynamicUnit::Registry final { Registry() = default; - std::unordered_map> m_registry; + std::shared_mutex m_mutex; + std::unordered_map, Hash, std::equal_to<>> + m_registry; + /// `std::unordered_map` keeps element addresses stable across a rehash, so a + /// `Unit *` already handed out survives later insertions and only the map + /// access itself needs guarding. Every `Measure` goes through here and the + /// http server renders on a thread pool, hence the shared read path. const Unit *unit_(const std::string_view name) { + { + const std::shared_lock lock(m_mutex); + if (const auto it = m_registry.find(name); it != m_registry.end()) { + return it->second.get(); + } + } + + const std::unique_lock lock(m_mutex); std::unique_ptr &unit = m_registry[std::string(name)]; if (unit == nullptr) { unit = std::make_unique(); diff --git a/src/odr/table_position.cpp b/src/odr/table_position.cpp index e92278c88..cc1f55f72 100644 --- a/src/odr/table_position.cpp +++ b/src/odr/table_position.cpp @@ -2,6 +2,7 @@ #include +#include #include namespace odr { @@ -21,8 +22,26 @@ std::uint32_t TablePosition::to_column_num(const std::string &string) { return result - 1; } +/// @param string the 1-based row number, as written in a cell reference. std::uint32_t TablePosition::to_row_num(const std::string &string) { - return std::stoul(string) - 1; + if (string.empty()) { + throw std::invalid_argument("s is empty"); + } + + std::uint64_t result = 0; + for (const char c : string) { + if (c < '0' || c > '9') { + throw std::invalid_argument("illegal character in \"" + string + "\""); + } + result = result * 10 + static_cast(c - '0'); + if (result > std::numeric_limits::max()) { + throw std::invalid_argument("row out of range in \"" + string + "\""); + } + } + if (result == 0) { + throw std::invalid_argument("row is not 1-based in \"" + string + "\""); + } + return static_cast(result) - 1; } std::string TablePosition::to_column_string(const std::uint32_t column) { diff --git a/test/src/file_test.cpp b/test/src/file_test.cpp index 25cb9972f..3251e8b21 100644 --- a/test/src/file_test.cpp +++ b/test/src/file_test.cpp @@ -7,6 +7,7 @@ #include #include +#include #include @@ -27,6 +28,15 @@ TEST(File, memory_file_reports_memory_and_its_bytes) { EXPECT_EQ(*file.memory_data(), "hello"); } +/// The null file has no bytes anywhere; every other accessor throws. +TEST(File, default_constructed_reports_unknown_location) { + const File file; + + EXPECT_EQ(file.location(), FileLocation::unknown); + EXPECT_THROW(std::ignore = file.size(), NullPointerError); + EXPECT_THROW(std::ignore = file.stream(), NullPointerError); +} + TEST(File, disk_file_has_no_memory_data) { const File file(std::make_shared( TestData::test_file_path("odr-public/odt/about.odt")));