From 47ef92e8fd7d204fcf6ed0d20498f4886a83bbd8 Mon Sep 17 00:00:00 2001 From: KasyanDiGris Date: Wed, 21 Jan 2026 17:43:51 +0300 Subject: [PATCH] fix: OPDS browser OOM (#403) ## Summary - Rewrite OpdsParser to stream parsing instead of full content - Fix OOM due to big http xml response Closes #385 --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? _**NO**_ --- lib/OpdsParser/OpdsParser.cpp | 47 ++++++++++++------- lib/OpdsParser/OpdsParser.h | 25 ++++++---- lib/OpdsParser/OpdsStream.cpp | 15 ++++++ lib/OpdsParser/OpdsStream.h | 23 +++++++++ .../browser/OpdsBookBrowserActivity.cpp | 23 +++++---- src/network/HttpDownloader.cpp | 17 +++++-- src/network/HttpDownloader.h | 2 + 7 files changed, 112 insertions(+), 40 deletions(-) create mode 100644 lib/OpdsParser/OpdsStream.cpp create mode 100644 lib/OpdsParser/OpdsStream.h diff --git a/lib/OpdsParser/OpdsParser.cpp b/lib/OpdsParser/OpdsParser.cpp index da4042f0..4b58d8f8 100644 --- a/lib/OpdsParser/OpdsParser.cpp +++ b/lib/OpdsParser/OpdsParser.cpp @@ -4,6 +4,14 @@ #include +OpdsParser::OpdsParser() { + parser = XML_ParserCreate(nullptr); + if (!parser) { + errorOccured = true; + Serial.printf("[%lu] [OPDS] Couldn't allocate memory for parser\n", millis()); + } +} + OpdsParser::~OpdsParser() { if (parser) { XML_StopParser(parser, XML_FALSE); @@ -14,13 +22,11 @@ OpdsParser::~OpdsParser() { } } -bool OpdsParser::parse(const char* xmlData, const size_t length) { - clear(); +size_t OpdsParser::write(uint8_t c) { return write(&c, 1); } - parser = XML_ParserCreate(nullptr); - if (!parser) { - Serial.printf("[%lu] [OPDS] Couldn't allocate memory for parser\n", millis()); - return false; +size_t OpdsParser::write(const uint8_t* xmlData, const size_t length) { + if (errorOccured) { + return length; } XML_SetUserData(parser, this); @@ -28,43 +34,48 @@ bool OpdsParser::parse(const char* xmlData, const size_t length) { XML_SetCharacterDataHandler(parser, characterData); // Parse in chunks to avoid large buffer allocations - const char* currentPos = xmlData; + const char* currentPos = reinterpret_cast(xmlData); size_t remaining = length; constexpr size_t chunkSize = 1024; while (remaining > 0) { void* const buf = XML_GetBuffer(parser, chunkSize); if (!buf) { + errorOccured = true; Serial.printf("[%lu] [OPDS] Couldn't allocate memory for buffer\n", millis()); XML_ParserFree(parser); parser = nullptr; - return false; + return length; } const size_t toRead = remaining < chunkSize ? remaining : chunkSize; memcpy(buf, currentPos, toRead); - const bool isFinal = (remaining == toRead); - if (XML_ParseBuffer(parser, static_cast(toRead), isFinal) == XML_STATUS_ERROR) { + if (XML_ParseBuffer(parser, static_cast(toRead), 0) == XML_STATUS_ERROR) { + errorOccured = true; Serial.printf("[%lu] [OPDS] Parse error at line %lu: %s\n", millis(), XML_GetCurrentLineNumber(parser), XML_ErrorString(XML_GetErrorCode(parser))); XML_ParserFree(parser); parser = nullptr; - return false; + return length; } currentPos += toRead; remaining -= toRead; } - - // Clean up parser - XML_ParserFree(parser); - parser = nullptr; - - Serial.printf("[%lu] [OPDS] Parsed %zu entries\n", millis(), entries.size()); - return true; + return length; } +void OpdsParser::flush() { + if (XML_Parse(parser, nullptr, 0, XML_TRUE) != XML_STATUS_OK) { + errorOccured = true; + XML_ParserFree(parser); + parser = nullptr; + } +} + +bool OpdsParser::error() const { return errorOccured; } + void OpdsParser::clear() { entries.clear(); currentEntry = OpdsEntry{}; diff --git a/lib/OpdsParser/OpdsParser.h b/lib/OpdsParser/OpdsParser.h index acb4b694..570ac4cc 100644 --- a/lib/OpdsParser/OpdsParser.h +++ b/lib/OpdsParser/OpdsParser.h @@ -1,4 +1,5 @@ #pragma once +#include #include #include @@ -42,28 +43,30 @@ using OpdsBook = OpdsEntry; * } * } */ -class OpdsParser { +class OpdsParser final : public Print { public: - OpdsParser() = default; + OpdsParser(); ~OpdsParser(); // Disable copy OpdsParser(const OpdsParser&) = delete; OpdsParser& operator=(const OpdsParser&) = delete; - /** - * Parse an OPDS XML feed. - * @param xmlData Pointer to the XML data - * @param length Length of the XML data - * @return true if parsing succeeded, false on error - */ - bool parse(const char* xmlData, size_t length); + size_t write(uint8_t) override; + size_t write(const uint8_t*, size_t) override; + + void flush() override; + + bool error() const; + + operator bool() { return !error(); } /** * Get the parsed entries (both navigation and book entries). * @return Vector of OpdsEntry entries */ - const std::vector& getEntries() const { return entries; } + const std::vector& getEntries() const& { return entries; } + std::vector getEntries() && { return std::move(entries); } /** * Get only book entries (legacy compatibility). @@ -96,4 +99,6 @@ class OpdsParser { bool inAuthor = false; bool inAuthorName = false; bool inId = false; + + bool errorOccured = false; }; diff --git a/lib/OpdsParser/OpdsStream.cpp b/lib/OpdsParser/OpdsStream.cpp new file mode 100644 index 00000000..742624a8 --- /dev/null +++ b/lib/OpdsParser/OpdsStream.cpp @@ -0,0 +1,15 @@ +#include "OpdsStream.h" + +OpdsParserStream::OpdsParserStream(OpdsParser& parser) : parser(parser) {} + +int OpdsParserStream::available() { return 0; } + +int OpdsParserStream::peek() { abort(); } + +int OpdsParserStream::read() { abort(); } + +size_t OpdsParserStream::write(uint8_t c) { return parser.write(c); } + +size_t OpdsParserStream::write(const uint8_t* buffer, size_t size) { return parser.write(buffer, size); } + +OpdsParserStream::~OpdsParserStream() { parser.flush(); } diff --git a/lib/OpdsParser/OpdsStream.h b/lib/OpdsParser/OpdsStream.h new file mode 100644 index 00000000..c72f2b6b --- /dev/null +++ b/lib/OpdsParser/OpdsStream.h @@ -0,0 +1,23 @@ +#pragma once + +#include + +#include "OpdsParser.h" + +class OpdsParserStream : public Stream { + public: + explicit OpdsParserStream(OpdsParser& parser); + + // That functions are not implimented for that stream + int available() override; + int peek() override; + int read() override; + + virtual size_t write(uint8_t c) override; + virtual size_t write(const uint8_t* buffer, size_t size) override; + + ~OpdsParserStream() override; + + private: + OpdsParser& parser; +}; diff --git a/src/activities/browser/OpdsBookBrowserActivity.cpp b/src/activities/browser/OpdsBookBrowserActivity.cpp index 677f9cac..555cba91 100644 --- a/src/activities/browser/OpdsBookBrowserActivity.cpp +++ b/src/activities/browser/OpdsBookBrowserActivity.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include "CrossPointSettings.h" @@ -265,23 +266,27 @@ void OpdsBookBrowserActivity::fetchFeed(const std::string& path) { std::string url = UrlUtils::buildUrl(serverUrl, path); Serial.printf("[%lu] [OPDS] Fetching: %s\n", millis(), url.c_str()); - std::string content; - if (!HttpDownloader::fetchUrl(url, content)) { - state = BrowserState::ERROR; - errorMessage = "Failed to fetch feed"; - updateRequired = true; - return; + OpdsParser parser; + + { + OpdsParserStream stream{parser}; + if (!HttpDownloader::fetchUrl(url, stream)) { + state = BrowserState::ERROR; + errorMessage = "Failed to fetch feed"; + updateRequired = true; + return; + } } - OpdsParser parser; - if (!parser.parse(content.c_str(), content.size())) { + if (!parser) { state = BrowserState::ERROR; errorMessage = "Failed to parse feed"; updateRequired = true; return; } - entries = parser.getEntries(); + entries = std::move(parser).getEntries(); + Serial.printf("[%lu] [OPDS] Found %d entries\n", millis(), entries.size()); selectorIndex = 0; if (entries.empty()) { diff --git a/src/network/HttpDownloader.cpp b/src/network/HttpDownloader.cpp index c4de3a05..fe65ea6b 100644 --- a/src/network/HttpDownloader.cpp +++ b/src/network/HttpDownloader.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include @@ -9,7 +10,7 @@ #include "util/UrlUtils.h" -bool HttpDownloader::fetchUrl(const std::string& url, std::string& outContent) { +bool HttpDownloader::fetchUrl(const std::string& url, Stream& outContent) { // Use WiFiClientSecure for HTTPS, regular WiFiClient for HTTP std::unique_ptr client; if (UrlUtils::isHttpsUrl(url)) { @@ -34,10 +35,20 @@ bool HttpDownloader::fetchUrl(const std::string& url, std::string& outContent) { return false; } - outContent = http.getString().c_str(); + http.writeToStream(&outContent); + http.end(); - Serial.printf("[%lu] [HTTP] Fetched %zu bytes\n", millis(), outContent.size()); + Serial.printf("[%lu] [HTTP] Fetch success\n", millis()); + return true; +} + +bool HttpDownloader::fetchUrl(const std::string& url, std::string& outContent) { + StreamString stream; + if (!fetchUrl(url, stream)) { + return false; + } + outContent = stream.c_str(); return true; } diff --git a/src/network/HttpDownloader.h b/src/network/HttpDownloader.h index e6e0f163..ac520a42 100644 --- a/src/network/HttpDownloader.h +++ b/src/network/HttpDownloader.h @@ -27,6 +27,8 @@ class HttpDownloader { */ static bool fetchUrl(const std::string& url, std::string& outContent); + static bool fetchUrl(const std::string& url, Stream& stream); + /** * Download a file to the SD card. * @param url The URL to download