From b1e430f1cabf59170ef8b627a3048134b25bdbfb Mon Sep 17 00:00:00 2001 From: jpirnay Date: Wed, 20 May 2026 14:11:50 +0200 Subject: [PATCH 1/6] Some font management improvements --- lib/EpdFont/SdCardFontRegistry.cpp | 58 +++++++-- lib/EpdFont/SdCardFontRegistry.h | 7 ++ lib/I18n/translations/english.yaml | 1 + lib/I18n/translations/german.yaml | 1 + open-x4-sdk | 2 +- scripts/generate-font-manifest.py | 17 ++- src/FontInstaller.cpp | 27 +++++ src/FontInstaller.h | 11 ++ .../settings/FontDownloadActivity.cpp | 110 +++++++++++++++--- .../settings/FontDownloadActivity.h | 7 ++ src/network/CrossPointWebServer.cpp | 55 +++++++-- 11 files changed, 256 insertions(+), 40 deletions(-) diff --git a/lib/EpdFont/SdCardFontRegistry.cpp b/lib/EpdFont/SdCardFontRegistry.cpp index 099732f2..9b1c6458 100644 --- a/lib/EpdFont/SdCardFontRegistry.cpp +++ b/lib/EpdFont/SdCardFontRegistry.cpp @@ -116,19 +116,16 @@ void SdCardFontRegistry::scanDirectory(const char* dirPath, SdCardFontFamilyInfo dir.close(); } -bool SdCardFontRegistry::discover() { - families_.clear(); - families_.reserve(MAX_SD_FAMILIES); - - FsFile root = Storage.open(FONTS_DIR); +void SdCardFontRegistry::scanRoot(const char* rootPath) { + FsFile root = Storage.open(rootPath); if (!root) { - LOG_DBG("SDREG", "Fonts directory not found: %s", FONTS_DIR); - return false; + LOG_DBG("SDREG", "Fonts directory not found: %s", rootPath); + return; } if (!root.isDirectory()) { - LOG_ERR("SDREG", "Fonts path is not a directory: %s", FONTS_DIR); + LOG_ERR("SDREG", "Fonts path is not a directory: %s", rootPath); root.close(); - return false; + return; } char nameBuffer[128]; @@ -143,21 +140,58 @@ bool SdCardFontRegistry::discover() { // Skip hidden/system directories (macOS ._*, .Trashes, etc.) if (nameBuffer[0] == '.' || nameBuffer[0] == '_') continue; + // Skip in-flight install/rollback dirs (Bookerly__staging, Bookerly__backup, ...) + // — these may contain valid-looking .cpfont files during a resumable + // download but must not be exposed as installable families. + const size_t nameLen = strlen(nameBuffer); + static constexpr const char* kStagingSuffix = "__staging"; + static constexpr const char* kBackupSuffix = "__backup"; + const size_t stagingLen = strlen(kStagingSuffix); + const size_t backupLen = strlen(kBackupSuffix); + if ((nameLen > stagingLen && strcmp(nameBuffer + nameLen - stagingLen, kStagingSuffix) == 0) || + (nameLen > backupLen && strcmp(nameBuffer + nameLen - backupLen, kBackupSuffix) == 0)) { + continue; + } + + // Primary wins: skip if the family was already discovered in an earlier root. + bool alreadyKnown = false; + for (const auto& f : families_) { + if (f.name == nameBuffer) { + alreadyKnown = true; + break; + } + } + if (alreadyKnown) { + LOG_DBG("SDREG", "Skipping duplicate family %s in %s (primary wins)", nameBuffer, rootPath); + continue; + } + SdCardFontFamilyInfo family; family.name = nameBuffer; - std::string subDirPath = std::string(FONTS_DIR) + "/" + nameBuffer; + std::string subDirPath = std::string(rootPath) + "/" + nameBuffer; scanDirectory(subDirPath.c_str(), family); if (!family.files.empty()) { families_.push_back(std::move(family)); - LOG_DBG("SDREG", "Found family: %s (%d files)", families_.back().name.c_str(), - static_cast(families_.back().files.size())); + LOG_DBG("SDREG", "Found family: %s (%d files) in %s", families_.back().name.c_str(), + static_cast(families_.back().files.size()), rootPath); } } else { entry.close(); } } root.close(); +} + +bool SdCardFontRegistry::discover() { + families_.clear(); + families_.reserve(MAX_SD_FAMILIES); + + // Primary first so it wins on family-name conflicts; the upstream roots are + // read-only fallbacks — installers/deleters always target only the primary. + scanRoot(FONTS_DIR); + scanRoot(FONTS_DIR_UPSTREAM_HIDDEN); + scanRoot(FONTS_DIR_UPSTREAM_VISIBLE); // Sort families alphabetically std::sort(families_.begin(), families_.end(), diff --git a/lib/EpdFont/SdCardFontRegistry.h b/lib/EpdFont/SdCardFontRegistry.h index ee8c34bb..138280a7 100644 --- a/lib/EpdFont/SdCardFontRegistry.h +++ b/lib/EpdFont/SdCardFontRegistry.h @@ -29,7 +29,13 @@ struct SdCardFontFamilyInfo { class SdCardFontRegistry { public: static constexpr int MAX_SD_FAMILIES = 128; + // Primary (writable) location. All installs/downloads/deletes target this root. static constexpr const char* FONTS_DIR = "/.crosspoint/fonts"; + // Read-only fallback roots for upstream crosspoint-reader compatibility. + // Upstream uses "/.fonts" (preferred, hidden) and "/fonts" (visible). Both are + // scanned after the primary; primary wins on family-name conflicts. + static constexpr const char* FONTS_DIR_UPSTREAM_HIDDEN = "/.fonts"; + static constexpr const char* FONTS_DIR_UPSTREAM_VISIBLE = "/fonts"; // Scan SD card, populate families_. Returns true if any families found. bool discover(); @@ -44,4 +50,5 @@ class SdCardFontRegistry { static bool parseFilename(const char* filename, uint8_t& size, uint8_t& style); void scanDirectory(const char* dirPath, SdCardFontFamilyInfo& family); + void scanRoot(const char* rootPath); }; diff --git a/lib/I18n/translations/english.yaml b/lib/I18n/translations/english.yaml index 6310742f..55f94b7d 100644 --- a/lib/I18n/translations/english.yaml +++ b/lib/I18n/translations/english.yaml @@ -305,6 +305,7 @@ STR_CONNECT: "Connect" STR_OPEN: "Open" STR_DOWNLOAD: "Download" STR_RETRY: "Retry" +STR_RESUME: "Resume" STR_YES: "Yes" STR_NO: "No" STR_SHOW: "Show" diff --git a/lib/I18n/translations/german.yaml b/lib/I18n/translations/german.yaml index ba37a442..71d1a1f8 100644 --- a/lib/I18n/translations/german.yaml +++ b/lib/I18n/translations/german.yaml @@ -244,6 +244,7 @@ STR_CONNECT: "Verbinden" STR_OPEN: "Öffnen" STR_DOWNLOAD: "Herunterladen" STR_RETRY: "Wiederh." +STR_RESUME: "Fortsetzen" STR_YES: "Ja" STR_NO: "Nein" STR_SHOW: "Zeigen" diff --git a/open-x4-sdk b/open-x4-sdk index 4d4c6678..570e9c4e 160000 --- a/open-x4-sdk +++ b/open-x4-sdk @@ -1 +1 @@ -Subproject commit 4d4c66787ff655adc93a1f37811610552a73aa1c +Subproject commit 570e9c4ee01c01abba9df52b4b3bda7ff610910b diff --git a/scripts/generate-font-manifest.py b/scripts/generate-font-manifest.py index aff5fde4..cc16f1df 100644 --- a/scripts/generate-font-manifest.py +++ b/scripts/generate-font-manifest.py @@ -22,6 +22,7 @@ import json import os import struct import sys +import zlib from pathlib import Path # --- .cpfont binary format constants --- @@ -96,6 +97,17 @@ def read_cpfont_styles(filepath: Path) -> list[str]: return styles +def compute_crc32(filepath: Path) -> int: + # Matches the on-device esp_rom_crc32_le(0, buf, len) accumulator used by + # FontDownloadActivity::computeFileCrc32. Originally introduced upstream in + # crosspoint-reader PR #1904 ("verify CRC32 checksum for font files"). + crc = 0 + with open(filepath, "rb") as f: + for chunk in iter(lambda: f.read(65536), b""): + crc = zlib.crc32(chunk, crc) + return crc & 0xFFFFFFFF + + def parse_filename(filename: str) -> tuple[str, str] | None: """Parse '_.cpfont' into (family, size_str). @@ -162,6 +174,7 @@ def build_manifest( { "name": filepath.relative_to(input_dir).as_posix(), "size": filepath.stat().st_size, + "crc32": compute_crc32(filepath), } ) @@ -175,7 +188,9 @@ def build_manifest( ) return { - "version": 1, + # v2 adds per-file crc32 (kept optional on the device side so old v1 + # manifests still load — see FontDownloadActivity::fetchAndParseManifest). + "version": 2, "baseUrl": base_url, "families": manifest_families, } diff --git a/src/FontInstaller.cpp b/src/FontInstaller.cpp index f0b9d55d..fa099d24 100644 --- a/src/FontInstaller.cpp +++ b/src/FontInstaller.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include @@ -90,6 +91,32 @@ void FontInstaller::buildFontPath(const char* family, const char* filename, char snprintf(outBuf, outBufSize, "%s/%s/%s", SdCardFontRegistry::FONTS_DIR, family, filename); } +void FontInstaller::buildStagingDirPath(const char* family, char* outBuf, size_t outBufSize) { + snprintf(outBuf, outBufSize, "%s/%s__staging", SdCardFontRegistry::FONTS_DIR, family); +} + +void FontInstaller::buildBackupDirPath(const char* family, char* outBuf, size_t outBufSize) { + snprintf(outBuf, outBufSize, "%s/%s__backup", SdCardFontRegistry::FONTS_DIR, family); +} + +bool FontInstaller::computeFileCrc32(const char* path, uint32_t& outCrc) { + FsFile f; + if (!Storage.openFileForRead("FONT", path, f)) { + return false; + } + constexpr size_t BUF_SIZE = 128; + uint8_t buf[BUF_SIZE]; + uint32_t crc = 0; + while (f.available()) { + const int n = f.read(buf, BUF_SIZE); + if (n <= 0) break; + crc = esp_rom_crc32_le(crc, buf, static_cast(n)); + } + f.close(); + outCrc = crc; + return true; +} + FontInstaller::Error FontInstaller::deleteFamily(const char* familyName) { if (!isValidFamilyName(familyName)) { return Error::INVALID_FAMILY_NAME; diff --git a/src/FontInstaller.h b/src/FontInstaller.h index c331b5d4..bb33fadd 100644 --- a/src/FontInstaller.h +++ b/src/FontInstaller.h @@ -31,10 +31,21 @@ class FontInstaller { /// Validate a .cpfont file on disk (check magic bytes). bool validateCpfontFile(const char* path); + /// Compute CRC32 of a file (esp_rom_crc32_le accumulator, matches + /// zlib.crc32 used by scripts/generate-font-manifest.py). Returns false if + /// the file cannot be opened. Mirrors the upstream PR #1904 approach so + /// our manifests stay binary-compatible. + static bool computeFileCrc32(const char* path, uint32_t& outCrc); + /// Build the full SD path for a font file. /// Writes "/.crosspoint/fonts//" to outBuf. static void buildFontPath(const char* family, const char* filename, char* outBuf, size_t outBufSize); + /// Build the staging-dir path "/__staging". + static void buildStagingDirPath(const char* family, char* outBuf, size_t outBufSize); + /// Build the backup-dir path "/__backup". + static void buildBackupDirPath(const char* family, char* outBuf, size_t outBufSize); + /// Delete a family directory and all .cpfont files in it. /// If the deleted family is the active reader font, clears the setting. Error deleteFamily(const char* familyName); diff --git a/src/activities/settings/FontDownloadActivity.cpp b/src/activities/settings/FontDownloadActivity.cpp index e7cd6818..869e740e 100644 --- a/src/activities/settings/FontDownloadActivity.cpp +++ b/src/activities/settings/FontDownloadActivity.cpp @@ -100,7 +100,10 @@ bool FontDownloadActivity::fetchAndParseManifest() { } int version = doc["version"] | 0; - if (version != 1) { + // v1 (legacy, no crc32) and v2 (with crc32) are both accepted; crc check is + // skipped per-file when the field is absent. See upstream PR #1904 for the + // CRC32 design we mirror. + if (version != 1 && version != 2) { LOG_ERR("FONT", "Unsupported manifest version: %d", version); errorMessage_ = "Unsupported manifest version"; return false; @@ -126,6 +129,10 @@ bool FontDownloadActivity::fetchAndParseManifest() { ManifestFile file; file.name = fileObj["name"] | ""; file.size = fileObj["size"] | 0; + if (fileObj["crc32"].is()) { + file.crc32 = fileObj["crc32"].as(); + file.hasCrc32 = true; + } family.totalSize += file.size; family.files.push_back(std::move(file)); } @@ -155,6 +162,12 @@ bool FontDownloadActivity::fetchAndParseManifest() { break; } } + } else { + // Surface leftover staging from a previously interrupted download so the + // UI can offer "Resume" instead of restarting from scratch. + char stagingDir[128]; + FontInstaller::buildStagingDirPath(family.name.c_str(), stagingDir, sizeof(stagingDir)); + family.hasResumableDownload = Storage.exists(stagingDir); } families_.push_back(std::move(family)); @@ -259,20 +272,14 @@ void FontDownloadActivity::downloadFamily(ManifestFamily& family) { char stagingDir[128]; char backupDir[128]; snprintf(liveDir, sizeof(liveDir), "%s/%s", SdCardFontRegistry::FONTS_DIR, family.name.c_str()); - snprintf(stagingDir, sizeof(stagingDir), "%s/%s__staging", SdCardFontRegistry::FONTS_DIR, family.name.c_str()); - snprintf(backupDir, sizeof(backupDir), "%s/%s__backup", SdCardFontRegistry::FONTS_DIR, family.name.c_str()); + FontInstaller::buildStagingDirPath(family.name.c_str(), stagingDir, sizeof(stagingDir)); + FontInstaller::buildBackupDirPath(family.name.c_str(), backupDir, sizeof(backupDir)); - if (Storage.exists(stagingDir) && !Storage.removeDir(stagingDir)) { - LOG_ERR("FONT", "Failed to clean staging dir: %s", stagingDir); - RenderLock lock(*this); - state_ = ERROR; - pendingErrorAction_ = PendingFontAction::Download; - downloadingFamilyIndex_ = static_cast(&family - families_.data()); - errorMessage_ = "Failed to prepare staging area"; - return; - } - - if (!Storage.mkdir(stagingDir)) { + // Resume-aware staging: if a __staging dir is left over from a previous + // interrupted download, keep it so files already on disk can be reused. + // Files are individually re-verified below (size + CRC) before being + // accepted, so half-written files are caught. + if (!Storage.exists(stagingDir) && !Storage.mkdir(stagingDir)) { LOG_ERR("FONT", "Failed to create staging dir: %s", stagingDir); RenderLock lock(*this); state_ = ERROR; @@ -304,6 +311,34 @@ void FontDownloadActivity::downloadFamily(ManifestFamily& family) { char stagedPath[128]; snprintf(stagedPath, sizeof(stagedPath), "%s/%s", stagingDir, localFilename.c_str()); + // If this file is already present in staging from a previous run and + // matches the manifest, skip the download. CRC32 is checked when the + // manifest carries one (v2+); otherwise size + magic-byte check is the + // best we can do. + if (Storage.exists(stagedPath)) { + FsFile f; + bool sizeOk = false; + if (Storage.openFileForRead("FONT", stagedPath, f)) { + sizeOk = (f.fileSize() == file.size); + f.close(); + } + bool crcOk = !file.hasCrc32; + if (sizeOk && file.hasCrc32) { + uint32_t actualCrc = 0; + if (FontInstaller::computeFileCrc32(stagedPath, actualCrc)) { + crcOk = (actualCrc == file.crc32); + } + } + if (sizeOk && crcOk && fontInstaller_.validateCpfontFile(stagedPath)) { + LOG_DBG("FONT", "Resuming: reusing %s", stagedPath); + fileProgress_ = file.size; + fileTotal_ = file.size; + continue; + } + LOG_DBG("FONT", "Resuming: re-downloading stale %s (sizeOk=%d crcOk=%d)", stagedPath, sizeOk, crcOk); + Storage.remove(stagedPath); + } + // Make sure parent directories exist for the file std::string stagedPathStr(stagedPath); size_t lastSlash = stagedPathStr.find_last_of('/'); @@ -336,7 +371,9 @@ void FontDownloadActivity::downloadFamily(ManifestFamily& family) { if (result == HttpDownloader::ABORTED) { LOG_INF("FONT", "Download cancelled: %s", file.name.c_str()); - Storage.removeDir(stagingDir); + // Keep staging dir so the next launch can resume. + Storage.remove(stagedPath); + family.hasResumableDownload = !family.installed; cancelRequested_ = true; RenderLock lock(*this); state_ = FAMILY_LIST; @@ -345,7 +382,10 @@ void FontDownloadActivity::downloadFamily(ManifestFamily& family) { if (result != HttpDownloader::OK) { LOG_ERR("FONT", "Download failed: %s (%d)", file.name.c_str(), result); - Storage.removeDir(stagingDir); + // Drop just the file that failed; keep already-downloaded siblings so + // the next retry resumes from here. + Storage.remove(stagedPath); + family.hasResumableDownload = !family.installed; RenderLock lock(*this); state_ = ERROR; pendingErrorAction_ = PendingFontAction::Download; @@ -354,9 +394,37 @@ void FontDownloadActivity::downloadFamily(ManifestFamily& family) { return; } + // CRC32: matches upstream PR #1904 — catches truncated/torn writes. + if (file.hasCrc32) { + uint32_t actualCrc = 0; + if (!FontInstaller::computeFileCrc32(stagedPath, actualCrc)) { + LOG_ERR("FONT", "Failed to read for CRC: %s", stagedPath); + Storage.remove(stagedPath); + family.hasResumableDownload = !family.installed; + RenderLock lock(*this); + state_ = ERROR; + pendingErrorAction_ = PendingFontAction::Download; + downloadingFamilyIndex_ = static_cast(&family - families_.data()); + errorMessage_ = "Failed to verify: " + file.name; + return; + } + if (actualCrc != file.crc32) { + LOG_ERR("FONT", "CRC32 mismatch for %s: got %08x expected %08x", file.name.c_str(), actualCrc, file.crc32); + Storage.remove(stagedPath); + family.hasResumableDownload = !family.installed; + RenderLock lock(*this); + state_ = ERROR; + pendingErrorAction_ = PendingFontAction::Download; + downloadingFamilyIndex_ = static_cast(&family - families_.data()); + errorMessage_ = "Checksum mismatch: " + file.name; + return; + } + } + if (!fontInstaller_.validateCpfontFile(stagedPath)) { LOG_ERR("FONT", "Invalid .cpfont: %s", stagedPath); - Storage.removeDir(stagingDir); + Storage.remove(stagedPath); + family.hasResumableDownload = !family.installed; RenderLock lock(*this); state_ = ERROR; pendingErrorAction_ = PendingFontAction::Download; @@ -411,6 +479,7 @@ void FontDownloadActivity::downloadFamily(ManifestFamily& family) { fontInstaller_.refreshRegistry(); family.installed = true; family.hasUpdate = false; + family.hasResumableDownload = false; syncSelectedIndexForNewActionCount(); RenderLock lock(*this); @@ -471,6 +540,7 @@ std::string FontDownloadActivity::confirmButtonLabel() const { const auto& family = families_[familyIndexFromList(selectedIndex_)]; if (family.installed && !family.hasUpdate) return tr(STR_DELETE); if (family.hasUpdate) return tr(STR_UPDATE); + if (family.hasResumableDownload) return tr(STR_RESUME); return tr(STR_DOWNLOAD); } @@ -615,6 +685,7 @@ void FontDownloadActivity::render(RenderLock&&) { const auto& f = families_[familyIndexFromList(index)]; if (f.hasUpdate) return tr(STR_UPDATE_AVAILABLE); if (f.installed) return tr(STR_INSTALLED); + if (f.hasResumableDownload) return tr(STR_RESUME); return ""; }, true); @@ -657,7 +728,10 @@ void FontDownloadActivity::render(RenderLock&&) { if (!errorMessage_.empty()) { renderer.drawCenteredText(UI_10_FONT_ID, centerY + metrics.verticalSpacing, errorMessage_.c_str()); } - const auto labels = mappedInput.mapLabels(tr(STR_BACK), tr(STR_RETRY), "", ""); + const bool canResume = pendingErrorAction_ == PendingFontAction::Download && downloadingFamilyIndex_ >= 0 && + downloadingFamilyIndex_ < static_cast(families_.size()) && + families_[downloadingFamilyIndex_].hasResumableDownload; + const auto labels = mappedInput.mapLabels(tr(STR_BACK), canResume ? tr(STR_RESUME) : tr(STR_RETRY), "", ""); GUI.drawButtonHints(renderer, labels.btn1, labels.btn2, labels.btn3, labels.btn4); } diff --git a/src/activities/settings/FontDownloadActivity.h b/src/activities/settings/FontDownloadActivity.h index dfe5f5f3..2ef7b8ed 100644 --- a/src/activities/settings/FontDownloadActivity.h +++ b/src/activities/settings/FontDownloadActivity.h @@ -1,5 +1,6 @@ #pragma once +#include #include #include @@ -36,6 +37,8 @@ class FontDownloadActivity : public Activity { struct ManifestFile { std::string name; size_t size = 0; + uint32_t crc32 = 0; + bool hasCrc32 = false; // false = legacy v1 manifest, fall back to size-only check }; struct ManifestFamily { @@ -46,6 +49,10 @@ class FontDownloadActivity : public Activity { size_t totalSize = 0; bool installed = false; bool hasUpdate = false; + // True iff a leftover __staging dir from a previous interrupted download + // exists for this not-yet-installed family — the next confirm will resume + // rather than restart. + bool hasResumableDownload = false; }; State state_ = WIFI_SELECTION; diff --git a/src/network/CrossPointWebServer.cpp b/src/network/CrossPointWebServer.cpp index 9aa114b0..693c66d4 100644 --- a/src/network/CrossPointWebServer.cpp +++ b/src/network/CrossPointWebServer.cpp @@ -1422,6 +1422,8 @@ namespace { struct RemoteManifestFile { std::string name; size_t size = 0; + uint32_t crc32 = 0; + bool hasCrc32 = false; }; struct RemoteManifestFamily { @@ -1475,7 +1477,9 @@ bool fetchRemoteFontManifest(FontInstaller& installer, std::vector(fileObj["size"] | 0); + if (fileObj["crc32"].is()) { + file.crc32 = fileObj["crc32"].as(); + file.hasCrc32 = true; + } if (!isValidFontFileName(file.name)) { LOG_ERR("WEB", "Manifest entry rejected, invalid file name in %s: %s", family.name.c_str(), file.name.c_str()); fileNamesOk = false; @@ -1577,11 +1585,9 @@ bool installRemoteFamily(const RemoteManifestFamily& family, const std::string& return false; } - if (Storage.exists(stagingDir) && !Storage.removeDir(stagingDir)) { - outError = "Failed to prepare staging area"; - return false; - } - if (!Storage.mkdir(stagingDir)) { + // Resumable staging: keep an existing __staging dir from a prior attempt so + // already-downloaded files can be reused (size + CRC validated per file). + if (!Storage.exists(stagingDir) && !Storage.mkdir(stagingDir)) { outError = "Failed to create staging area"; return false; } @@ -1600,11 +1606,34 @@ bool installRemoteFamily(const RemoteManifestFamily& family, const std::string& char stagedPath[128]; int sn = snprintf(stagedPath, sizeof(stagedPath), "%s/%s", stagingDir, localFilename.c_str()); if (sn < 0 || static_cast(sn) >= sizeof(stagedPath)) { + // Path-length bugs are not resumable; nuke staging so we don't get stuck. Storage.removeDir(stagingDir); outError = std::string("File path too long: ") + localFilename; return false; } + // Reuse a previously-downloaded file if it still matches the manifest. + if (Storage.exists(stagedPath)) { + FsFile f; + bool sizeOk = false; + if (Storage.openFileForRead("WEB", stagedPath, f)) { + sizeOk = (static_cast(f.size()) == file.size); + f.close(); + } + bool crcOk = !file.hasCrc32; + if (sizeOk && file.hasCrc32) { + uint32_t actualCrc = 0; + if (FontInstaller::computeFileCrc32(stagedPath, actualCrc)) { + crcOk = (actualCrc == file.crc32); + } + } + if (sizeOk && crcOk && installer.validateCpfontFile(stagedPath)) { + LOG_DBG("WEB", "Resuming: reusing %s", stagedPath); + continue; + } + Storage.remove(stagedPath); + } + // Ensure intermediate subdirectories exist inside stagingDir std::string stagedPathStr(stagedPath); size_t lastSlash = stagedPathStr.find_last_of('/'); @@ -1616,13 +1645,23 @@ bool installRemoteFamily(const RemoteManifestFamily& family, const std::string& auto result = HttpDownloader::downloadToFile(url, stagedPath, nullptr); if (result != HttpDownloader::OK) { - Storage.removeDir(stagingDir); + // Drop just the failed file; keep already-downloaded siblings. + Storage.remove(stagedPath); outError = std::string("Download failed: ") + file.name; return false; } + if (file.hasCrc32) { + uint32_t actualCrc = 0; + if (!FontInstaller::computeFileCrc32(stagedPath, actualCrc) || actualCrc != file.crc32) { + Storage.remove(stagedPath); + outError = std::string("Checksum mismatch: ") + file.name; + return false; + } + } + if (!installer.validateCpfontFile(stagedPath)) { - Storage.removeDir(stagingDir); + Storage.remove(stagedPath); outError = std::string("Invalid font file: ") + file.name; return false; } From 02adb40f95d440f867ba733b93be0c44d0850f6e Mon Sep 17 00:00:00 2001 From: jpirnay Date: Wed, 20 May 2026 14:35:41 +0200 Subject: [PATCH 2/6] Add delay + logs --- src/activities/settings/FontDownloadActivity.cpp | 7 +++++++ src/network/HttpDownloader.cpp | 3 +++ 2 files changed, 10 insertions(+) diff --git a/src/activities/settings/FontDownloadActivity.cpp b/src/activities/settings/FontDownloadActivity.cpp index 869e740e..69dd36da 100644 --- a/src/activities/settings/FontDownloadActivity.cpp +++ b/src/activities/settings/FontDownloadActivity.cpp @@ -348,6 +348,13 @@ void FontDownloadActivity::downloadFamily(ManifestFamily& family) { std::string url = baseUrl_ + file.name; + // Give the previous TLS connection (manifest fetch on first iteration, or + // the previous file otherwise) time to fully release its socket and TLS + // buffers. Without this, http.GET() can fail immediately with -1 + // (HTTPC_ERROR_CONNECTION_REFUSED) on the next request — same workaround + // the browser-driven install path uses in CrossPointWebServer. + delay(500); + auto result = HttpDownloader::downloadToFile(url, stagedPath, [this](unsigned int downloaded, unsigned int total) { mappedInput.update(); fileProgress_ = downloaded; diff --git a/src/network/HttpDownloader.cpp b/src/network/HttpDownloader.cpp index be044b52..070daa57 100644 --- a/src/network/HttpDownloader.cpp +++ b/src/network/HttpDownloader.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include @@ -127,6 +128,8 @@ HttpDownloader::DownloadError HttpDownloader::downloadToFile(const std::string& LOG_DBG("HTTP", "Downloading: %s", url.c_str()); LOG_DBG("HTTP", "Destination: %s", destPath.c_str()); + LOG_DBG("HTTP", "Heap free: %u, largest block: %u", esp_get_free_heap_size(), + heap_caps_get_largest_free_block(MALLOC_CAP_DEFAULT)); http.begin(*client, url.c_str()); http.setReuse(false); From 61e1f8b14566076b2ffeb2ca26a5780c1bc77cd9 Mon Sep 17 00:00:00 2001 From: jpirnay Date: Wed, 20 May 2026 18:51:24 +0200 Subject: [PATCH 3/6] Refactor font download --- .../settings/FontDownloadActivity.cpp | 308 +++++++-- .../settings/FontDownloadActivity.h | 35 +- src/network/CrossPointWebServer.cpp | 69 +- src/network/HttpDownloader.cpp | 618 +++++++++++------- src/network/HttpDownloader.h | 44 +- src/util/UrlUtils.cpp | 2 - src/util/UrlUtils.h | 5 - 7 files changed, 752 insertions(+), 329 deletions(-) diff --git a/src/activities/settings/FontDownloadActivity.cpp b/src/activities/settings/FontDownloadActivity.cpp index 69dd36da..5498eb95 100644 --- a/src/activities/settings/FontDownloadActivity.cpp +++ b/src/activities/settings/FontDownloadActivity.cpp @@ -34,11 +34,16 @@ void FontDownloadActivity::onEnter() { void FontDownloadActivity::onExit() { Activity::onExit(); + // Always silentRestart on exit, regardless of WiFi state. Even if a deep + // error path turned WiFi off, we still did expensive network/TLS work and + // the heap is fragmented past the point a normal session can recover (see + // [project-font-download-heap-stash]). A reboot here gives the next + // activity a pristine heap. if (WiFi.getMode() != WIFI_MODE_NULL) { WiFi.disconnect(false); delay(30); - silentRestart(); } + silentRestart(); } void FontDownloadActivity::onWifiSelectionComplete(const bool success) { @@ -72,6 +77,10 @@ void FontDownloadActivity::onWifiSelectionComplete(const bool success) { bool FontDownloadActivity::fetchAndParseManifest() { static constexpr const char* MANIFEST_TMP = "/fonts_manifest.tmp"; + // Standalone manifest fetch: closes the TLS connection before the JSON + // parse so the parser has full heap headroom. The Session is opened later + // for the per-file download loop, on a heap that's been slimmed by + // trimManifestForDownload(). auto result = HttpDownloader::downloadToFile(FONT_MANIFEST_URL, MANIFEST_TMP, nullptr); if (result != HttpDownloader::OK) { LOG_ERR("FONT", "Failed to fetch manifest from %s", FONT_MANIFEST_URL); @@ -119,10 +128,7 @@ bool FontDownloadActivity::fetchAndParseManifest() { ManifestFamily family; family.name = fObj["name"] | ""; family.description = fObj["description"] | ""; - - for (JsonVariant s : fObj["styles"].as()) { - family.styles.push_back(s.as()); - } + // styles[] in the JSON is intentionally ignored — see ManifestFamily. family.totalSize = 0; for (JsonObject fileObj : fObj["files"].as()) { @@ -177,13 +183,167 @@ bool FontDownloadActivity::fetchAndParseManifest() { return true; } +// --- Stash/Restore --- +// +// Persist families_ to a small binary file on SD so we can free the +// ~10 KB of scattered std::string allocations it holds. mbedtls's TLS +// handshake needs many small allocations from a defragmented heap; with +// families_ resident, the heap stays fragmented at ~36 KB largest contiguous +// and the handshake fails with -0x2700 / flags=0 (internal alloc failure). +// +// Format (little-endian): +// u32 magic = 'CPFM' (0x4D465043) +// u32 count = number of families +// for each family: +// u8 name_len, name bytes +// u8 desc_len, desc bytes +// u32 totalSize +// u8 flags bit0 installed, bit1 hasUpdate, bit2 hasResumableDownload +// u8 file_count +// for each file: +// u8 name_len, name bytes +// u32 size +// u32 crc32 +// u8 hasCrc32 + +static constexpr const char* FAMILIES_STASH_PATH = "/fonts_families.bin"; +static constexpr uint32_t FAMILIES_STASH_MAGIC = 0x4D465043; // 'CPFM' + +namespace { +bool writeU8(FsFile& f, uint8_t v) { return f.write(&v, 1) == 1; } +bool writeU32(FsFile& f, uint32_t v) { + uint8_t buf[4] = {static_cast(v), static_cast(v >> 8), static_cast(v >> 16), + static_cast(v >> 24)}; + return f.write(buf, 4) == 4; +} +bool writeStr(FsFile& f, const std::string& s) { + if (s.size() > 255) return false; + if (!writeU8(f, static_cast(s.size()))) return false; + return s.empty() || f.write(reinterpret_cast(s.data()), s.size()) == s.size(); +} +bool readU8(FsFile& f, uint8_t& v) { return f.read(&v, 1) == 1; } +bool readU32(FsFile& f, uint32_t& v) { + uint8_t buf[4]; + if (f.read(buf, 4) != 4) return false; + v = buf[0] | (buf[1] << 8) | (buf[2] << 16) | (buf[3] << 24); + return true; +} +bool readStr(FsFile& f, std::string& s) { + uint8_t len = 0; + if (!readU8(f, len)) return false; + s.resize(len); + if (len == 0) return true; + return f.read(reinterpret_cast(&s[0]), len) == len; +} +} // namespace + +bool FontDownloadActivity::stashFamiliesToSd() { + Storage.remove(FAMILIES_STASH_PATH); + FsFile file; + if (!Storage.openFileForWrite("FONT", FAMILIES_STASH_PATH, file)) { + LOG_ERR("FONT", "Stash open failed"); + return false; + } + + bool ok = writeU32(file, FAMILIES_STASH_MAGIC); + ok = ok && writeU32(file, static_cast(families_.size())); + for (const auto& fam : families_) { + if (!ok) break; + ok = ok && writeStr(file, fam.name); + ok = ok && writeStr(file, fam.description); + ok = ok && writeU32(file, static_cast(fam.totalSize)); + uint8_t flags = (fam.installed ? 1 : 0) | (fam.hasUpdate ? 2 : 0) | (fam.hasResumableDownload ? 4 : 0); + ok = ok && writeU8(file, flags); + ok = ok && writeU8(file, static_cast(fam.files.size())); + for (const auto& fl : fam.files) { + ok = ok && writeStr(file, fl.name); + ok = ok && writeU32(file, static_cast(fl.size)); + ok = ok && writeU32(file, fl.crc32); + ok = ok && writeU8(file, fl.hasCrc32 ? 1 : 0); + } + } + + file.flush(); + file.close(); + if (!ok) { + LOG_ERR("FONT", "Stash write failed"); + Storage.remove(FAMILIES_STASH_PATH); + return false; + } + // Free the in-memory representation now that it's safely on disk. + families_.clear(); + families_.shrink_to_fit(); + LOG_DBG("FONT", "Stashed families_ to %s and cleared in-memory copy", FAMILIES_STASH_PATH); + return true; +} + +bool FontDownloadActivity::restoreFamiliesFromSd() { + FsFile file; + if (!Storage.openFileForRead("FONT", FAMILIES_STASH_PATH, file)) { + LOG_ERR("FONT", "Stash file missing"); + return false; + } + + uint32_t magic = 0; + uint32_t count = 0; + bool ok = readU32(file, magic) && magic == FAMILIES_STASH_MAGIC && readU32(file, count); + if (ok) { + families_.clear(); + families_.reserve(count); + for (uint32_t i = 0; i < count && ok; i++) { + ManifestFamily fam; + ok = ok && readStr(file, fam.name); + ok = ok && readStr(file, fam.description); + uint32_t totalSize = 0; + ok = ok && readU32(file, totalSize); + fam.totalSize = totalSize; + uint8_t flags = 0; + ok = ok && readU8(file, flags); + fam.installed = (flags & 1) != 0; + fam.hasUpdate = (flags & 2) != 0; + fam.hasResumableDownload = (flags & 4) != 0; + uint8_t fileCount = 0; + ok = ok && readU8(file, fileCount); + fam.files.reserve(fileCount); + for (uint8_t j = 0; j < fileCount && ok; j++) { + ManifestFile fl; + ok = ok && readStr(file, fl.name); + uint32_t fsize = 0, fcrc = 0; + ok = ok && readU32(file, fsize); + fl.size = fsize; + ok = ok && readU32(file, fcrc); + fl.crc32 = fcrc; + uint8_t hasCrc = 0; + ok = ok && readU8(file, hasCrc); + fl.hasCrc32 = hasCrc != 0; + fam.files.push_back(std::move(fl)); + } + if (ok) families_.push_back(std::move(fam)); + } + } + file.close(); + if (!ok) { + LOG_ERR("FONT", "Stash read failed (magic=%08x count=%u)", magic, count); + return false; + } + // Keep the stash file around so a crash mid-download can still recover. + // It gets overwritten on next stash and is harmless if stale. + LOG_DBG("FONT", "Restored %zu families from stash", families_.size()); + return true; +} + // --- Download --- void FontDownloadActivity::downloadAll() { cancelRequested_ = false; + // Snapshot indices upfront because downloadFamily() stashes/restores + // families_ — indices remain valid as long as we don't sort or splice it. + std::vector targetIndices; for (size_t i = 0; i < families_.size(); i++) { - if (families_[i].installed) continue; - downloadFamily(families_[i]); + if (!families_[i].installed) targetIndices.push_back(static_cast(i)); + } + for (int idx : targetIndices) { + downloadFamily(idx); if (state_ == ERROR || cancelRequested_) return; } @@ -193,9 +353,12 @@ void FontDownloadActivity::downloadAll() { void FontDownloadActivity::updateAll() { cancelRequested_ = false; + std::vector targetIndices; for (size_t i = 0; i < families_.size(); i++) { - if (!families_[i].installed || !families_[i].hasUpdate) continue; - downloadFamily(families_[i]); + if (families_[i].installed && families_[i].hasUpdate) targetIndices.push_back(static_cast(i)); + } + for (int idx : targetIndices) { + downloadFamily(idx); if (state_ == ERROR || cancelRequested_) return; } @@ -255,12 +418,26 @@ bool FontDownloadActivity::hasUpdateCandidates() const { return false; } -void FontDownloadActivity::downloadFamily(ManifestFamily& family) { +void FontDownloadActivity::downloadFamily(int familyIdx) { + if (familyIdx < 0 || familyIdx >= static_cast(families_.size())) { + LOG_ERR("FONT", "downloadFamily: invalid index %d (size %zu)", familyIdx, families_.size()); + return; + } + + // Snapshot the target family by value, then stash + free families_ so the + // ~10 KB of scattered std::string allocations don't fragment the heap + // during the TLS handshake. Render-path caches (downloadingFamilyName_, + // downloadingFamilyHasResumable_) cover the family-name and Resume-label + // accesses that previously read families_ during DOWNLOADING/ERROR. + ManifestFamily family = families_[familyIdx]; + downloadingFamilyName_ = family.name; + downloadingFamilyHasResumable_ = family.hasResumableDownload; + cancelRequested_ = false; { RenderLock lock(*this); state_ = DOWNLOADING; - downloadingFamilyIndex_ = static_cast(&family - families_.data()); + downloadingFamilyIndex_ = familyIdx; currentFileIndex_ = 0; currentFileTotal_ = family.files.size(); fileProgress_ = 0; @@ -268,6 +445,35 @@ void FontDownloadActivity::downloadFamily(ManifestFamily& family) { } requestUpdateAndWait(); + if (!stashFamiliesToSd()) { + RenderLock lock(*this); + state_ = ERROR; + pendingErrorAction_ = PendingFontAction::Download; + errorMessage_ = "Failed to stash manifest"; + return; + } + + // Run the actual download with families_ empty (defragmented heap). + downloadFamilyImpl(family, familyIdx); + + // Update cached render state from the impl's mutations. + downloadingFamilyHasResumable_ = family.hasResumableDownload; + + // Restore families_ regardless of success/error/abort outcome, then merge + // back the mutations the impl made on the local family copy. + if (restoreFamiliesFromSd() && familyIdx >= 0 && familyIdx < static_cast(families_.size())) { + families_[familyIdx].installed = family.installed; + families_[familyIdx].hasUpdate = family.hasUpdate; + families_[familyIdx].hasResumableDownload = family.hasResumableDownload; + } + syncSelectedIndexForNewActionCount(); +} + +void FontDownloadActivity::downloadFamilyImpl(ManifestFamily& family, int familyIdx) { + // httpSession_ does the TLS handshake on its first downloadToFile call; + // subsequent files reuse the open keep-alive connection. If the server + // dropped the connection during the idle gap (user browsing the family + // list), the Session layer transparently reinitialises and retries once. char liveDir[128]; char stagingDir[128]; char backupDir[128]; @@ -284,7 +490,7 @@ void FontDownloadActivity::downloadFamily(ManifestFamily& family) { RenderLock lock(*this); state_ = ERROR; pendingErrorAction_ = PendingFontAction::Download; - downloadingFamilyIndex_ = static_cast(&family - families_.data()); + downloadingFamilyIndex_ = familyIdx; errorMessage_ = "Failed to create staging area"; return; } @@ -348,33 +554,27 @@ void FontDownloadActivity::downloadFamily(ManifestFamily& family) { std::string url = baseUrl_ + file.name; - // Give the previous TLS connection (manifest fetch on first iteration, or - // the previous file otherwise) time to fully release its socket and TLS - // buffers. Without this, http.GET() can fail immediately with -1 - // (HTTPC_ERROR_CONNECTION_REFUSED) on the next request — same workaround - // the browser-driven install path uses in CrossPointWebServer. - delay(500); + auto result = HttpDownloader::downloadToFile( + httpSession_, url, stagedPath, [this](unsigned int downloaded, unsigned int total) { + mappedInput.update(); + fileProgress_ = downloaded; + fileTotal_ = total; - auto result = HttpDownloader::downloadToFile(url, stagedPath, [this](unsigned int downloaded, unsigned int total) { - mappedInput.update(); - fileProgress_ = downloaded; - fileTotal_ = total; + const unsigned long now = millis(); + int percent = 0; + if (total > 0) { + percent = static_cast((static_cast(downloaded) * 100ULL + total / 2) / total); + } + const bool percentChanged = percent != lastProgressPercent_; + const bool timeElapsed = lastProgressUpdateMs_ == 0 || now - lastProgressUpdateMs_ > 2000; + if ((percentChanged && timeElapsed) || downloaded == total) { + requestUpdate(true); + lastProgressPercent_ = percent; + lastProgressUpdateMs_ = now; + } - const unsigned long now = millis(); - int percent = 0; - if (total > 0) { - percent = static_cast((static_cast(downloaded) * 100ULL + total / 2) / total); - } - const bool percentChanged = percent != lastProgressPercent_; - const bool timeElapsed = lastProgressUpdateMs_ == 0 || now - lastProgressUpdateMs_ > 2000; - if ((percentChanged && timeElapsed) || downloaded == total) { - requestUpdate(true); - lastProgressPercent_ = percent; - lastProgressUpdateMs_ = now; - } - - return !mappedInput.wasPressed(MappedInputManager::Button::Back); - }); + return !mappedInput.wasPressed(MappedInputManager::Button::Back); + }); if (result == HttpDownloader::ABORTED) { LOG_INF("FONT", "Download cancelled: %s", file.name.c_str()); @@ -396,7 +596,7 @@ void FontDownloadActivity::downloadFamily(ManifestFamily& family) { RenderLock lock(*this); state_ = ERROR; pendingErrorAction_ = PendingFontAction::Download; - downloadingFamilyIndex_ = static_cast(&family - families_.data()); + downloadingFamilyIndex_ = familyIdx; errorMessage_ = "Download failed: " + file.name; return; } @@ -411,7 +611,7 @@ void FontDownloadActivity::downloadFamily(ManifestFamily& family) { RenderLock lock(*this); state_ = ERROR; pendingErrorAction_ = PendingFontAction::Download; - downloadingFamilyIndex_ = static_cast(&family - families_.data()); + downloadingFamilyIndex_ = familyIdx; errorMessage_ = "Failed to verify: " + file.name; return; } @@ -422,7 +622,7 @@ void FontDownloadActivity::downloadFamily(ManifestFamily& family) { RenderLock lock(*this); state_ = ERROR; pendingErrorAction_ = PendingFontAction::Download; - downloadingFamilyIndex_ = static_cast(&family - families_.data()); + downloadingFamilyIndex_ = familyIdx; errorMessage_ = "Checksum mismatch: " + file.name; return; } @@ -435,7 +635,7 @@ void FontDownloadActivity::downloadFamily(ManifestFamily& family) { RenderLock lock(*this); state_ = ERROR; pendingErrorAction_ = PendingFontAction::Download; - downloadingFamilyIndex_ = static_cast(&family - families_.data()); + downloadingFamilyIndex_ = familyIdx; errorMessage_ = "Invalid font file: " + file.name; return; } @@ -449,7 +649,7 @@ void FontDownloadActivity::downloadFamily(ManifestFamily& family) { RenderLock lock(*this); state_ = ERROR; pendingErrorAction_ = PendingFontAction::Download; - downloadingFamilyIndex_ = static_cast(&family - families_.data()); + downloadingFamilyIndex_ = familyIdx; errorMessage_ = "Failed to prepare backup area"; return; } @@ -460,7 +660,7 @@ void FontDownloadActivity::downloadFamily(ManifestFamily& family) { RenderLock lock(*this); state_ = ERROR; pendingErrorAction_ = PendingFontAction::Download; - downloadingFamilyIndex_ = static_cast(&family - families_.data()); + downloadingFamilyIndex_ = familyIdx; errorMessage_ = "Failed to replace installed font"; return; } @@ -474,7 +674,7 @@ void FontDownloadActivity::downloadFamily(ManifestFamily& family) { RenderLock lock(*this); state_ = ERROR; pendingErrorAction_ = PendingFontAction::Download; - downloadingFamilyIndex_ = static_cast(&family - families_.data()); + downloadingFamilyIndex_ = familyIdx; errorMessage_ = "Failed to finalize font install"; return; } @@ -487,7 +687,8 @@ void FontDownloadActivity::downloadFamily(ManifestFamily& family) { family.installed = true; family.hasUpdate = false; family.hasResumableDownload = false; - syncSelectedIndexForNewActionCount(); + // syncSelectedIndexForNewActionCount() is deferred to downloadFamily() — + // it needs families_ which is empty during this impl. RenderLock lock(*this); state_ = COMPLETE; @@ -578,7 +779,7 @@ void FontDownloadActivity::loop() { if (family.installed && !family.hasUpdate) { promptDeleteFamily(familyIndex); } else { - downloadFamily(families_[familyIndex]); + downloadFamily(familyIndex); requestUpdateAndWait(); } } @@ -605,7 +806,7 @@ void FontDownloadActivity::loop() { if (pendingErrorAction_ == PendingFontAction::Delete) { deleteFamilyAtIndex(downloadingFamilyIndex_); } else { - downloadFamily(families_[downloadingFamilyIndex_]); + downloadFamily(downloadingFamilyIndex_); } requestUpdateAndWait(); } else { @@ -702,9 +903,9 @@ void FontDownloadActivity::render(RenderLock&&) { GUI.drawButtonHints(renderer, labels.btn1, labels.btn2, labels.btn3, labels.btn4); } } else if (state_ == DOWNLOADING) { - const auto& family = families_[downloadingFamilyIndex_]; - - std::string statusText = std::string(tr(STR_DOWNLOADING)) + " " + family.name + " (" + + // families_ is stashed to SD during downloadFamily(); read the cached + // name instead of indexing families_. + std::string statusText = std::string(tr(STR_DOWNLOADING)) + " " + downloadingFamilyName_ + " (" + std::to_string(currentFileIndex_ + 1) + "/" + std::to_string(currentFileTotal_) + ")"; renderer.drawCenteredText(UI_10_FONT_ID, centerY - lineHeight, statusText.c_str()); @@ -735,9 +936,10 @@ void FontDownloadActivity::render(RenderLock&&) { if (!errorMessage_.empty()) { renderer.drawCenteredText(UI_10_FONT_ID, centerY + metrics.verticalSpacing, errorMessage_.c_str()); } - const bool canResume = pendingErrorAction_ == PendingFontAction::Download && downloadingFamilyIndex_ >= 0 && - downloadingFamilyIndex_ < static_cast(families_.size()) && - families_[downloadingFamilyIndex_].hasResumableDownload; + // Use the cached value: families_ may have just been restored (post-impl) + // or still empty (if the failure was in the stash itself); either way the + // cache reflects the last update from the download attempt. + const bool canResume = pendingErrorAction_ == PendingFontAction::Download && downloadingFamilyHasResumable_; const auto labels = mappedInput.mapLabels(tr(STR_BACK), canResume ? tr(STR_RESUME) : tr(STR_RETRY), "", ""); GUI.drawButtonHints(renderer, labels.btn1, labels.btn2, labels.btn3, labels.btn4); } diff --git a/src/activities/settings/FontDownloadActivity.h b/src/activities/settings/FontDownloadActivity.h index 2ef7b8ed..3de73d1a 100644 --- a/src/activities/settings/FontDownloadActivity.h +++ b/src/activities/settings/FontDownloadActivity.h @@ -6,6 +6,7 @@ #include "../Activity.h" #include "FontInstaller.h" +#include "network/HttpDownloader.h" #include "util/ButtonNavigator.h" #ifndef FONT_MANIFEST_URL @@ -44,7 +45,9 @@ class FontDownloadActivity : public Activity { struct ManifestFamily { std::string name; std::string description; - std::vector styles; + // `styles` was once parsed here but never rendered — dropped to avoid + // ArduinoJson string allocations that fragmented the heap before the + // first TLS download. Resurrect if a UI surfaces style names. std::vector files; size_t totalSize = 0; bool installed = false; @@ -59,6 +62,14 @@ class FontDownloadActivity : public Activity { FontInstaller fontInstaller_; ButtonNavigator buttonNavigator_; + // HTTP/TLS session shared across all files of a single downloadFamily() + // call. Each family install pays the TLS handshake once (on its first + // file); subsequent files reuse the open keep-alive connection. + // NOT shared with the manifest fetch — holding the TLS context open + // through the JSON parse aborts on the ~36 KB contiguous allocation + // collision with ArduinoJson's working memory. + HttpDownloader::Session httpSession_; + std::string baseUrl_; std::vector families_; int selectedIndex_ = 0; @@ -74,6 +85,11 @@ class FontDownloadActivity : public Activity { size_t fileProgress_ = 0; size_t fileTotal_ = 0; int downloadingFamilyIndex_ = 0; + // Cached during downloadFamily() before families_ is stashed to SD, so the + // render path can show the family name and decide the Retry/Resume label + // without touching families_ (which is empty during the download). + std::string downloadingFamilyName_; + bool downloadingFamilyHasResumable_ = false; PendingFontAction pendingErrorAction_ = PendingFontAction::None; std::string errorMessage_; bool cancelRequested_ = false; @@ -83,9 +99,24 @@ class FontDownloadActivity : public Activity { void onWifiSelectionComplete(bool success); bool fetchAndParseManifest(); - void downloadFamily(ManifestFamily& family); + // Download a single family by its index into families_. Internally stashes + // families_ to SD so the TLS handshake runs on a defragmented heap; the + // selected family's state mutations (installed/hasUpdate/hasResumableDownload) + // are merged back into families_ on return. + void downloadFamily(int familyIdx); + // Internal: the body of downloadFamily after the stash. Operates only on + // the local family copy and constants like familyIdx; never touches + // families_ (which is empty during this call). + void downloadFamilyImpl(ManifestFamily& family, int familyIdx); void downloadAll(); void updateAll(); + + // Persist families_ to /fonts_families.bin and clear the in-memory vector. + // Used to free the ~10 KB of scattered std::string allocations that fragment + // the heap enough to break the TLS handshake during font downloads. + bool stashFamiliesToSd(); + // Read /fonts_families.bin back into families_. Returns true on success. + bool restoreFamiliesFromSd(); bool isDownloadAllSelected() const { return hasDownloadCandidates() && selectedIndex_ == 0; } bool isUpdateAllSelected() const { if (!hasUpdateCandidates()) return false; diff --git a/src/network/CrossPointWebServer.cpp b/src/network/CrossPointWebServer.cpp index 693c66d4..11df159c 100644 --- a/src/network/CrossPointWebServer.cpp +++ b/src/network/CrossPointWebServer.cpp @@ -1449,11 +1449,12 @@ bool isValidFontFileName(const std::string& name) { return true; } -bool fetchRemoteFontManifest(FontInstaller& installer, std::vector& outFamilies, - std::string& outBaseUrl, std::string& outError) { +bool fetchRemoteFontManifest(HttpDownloader::Session& session, FontInstaller& installer, + std::vector& outFamilies, std::string& outBaseUrl, + std::string& outError) { static constexpr const char* MANIFEST_TMP = "/fonts_manifest_web.tmp"; - auto result = HttpDownloader::downloadToFile(FONT_MANIFEST_URL, MANIFEST_TMP, nullptr); + auto result = HttpDownloader::downloadToFile(session, FONT_MANIFEST_URL, MANIFEST_TMP, nullptr); if (result != HttpDownloader::OK) { outError = "Failed to fetch font manifest"; Storage.remove(MANIFEST_TMP); @@ -1552,8 +1553,8 @@ bool fetchRemoteFontManifest(FontInstaller& installer, std::vector families; std::string baseUrl; std::string error; - if (!fetchRemoteFontManifest(installer, families, baseUrl, error)) { - JsonDocument errDoc; - errDoc["ok"] = false; - errDoc["error"] = error; - String out; - serializeJson(errDoc, out); - server->send(500, "application/json", out); - return; - } + { + HttpDownloader::Session manifestSession; + if (!fetchRemoteFontManifest(manifestSession, installer, families, baseUrl, error)) { + JsonDocument errDoc; + errDoc["ok"] = false; + errDoc["error"] = error; + String out; + serializeJson(errDoc, out); + server->send(500, "application/json", out); + return; + } + } // close TLS before the response JSON document is built JsonDocument doc; doc["ok"] = true; @@ -1792,18 +1797,24 @@ void CrossPointWebServer::handleFontDownload() { FontInstaller installer(sdFontSystem.registry()); installer.refreshRegistry(); + // Manifest fetch uses a local session that closes before parse, so the + // ArduinoJson parse runs on a clean heap. A separate install session is + // opened below for the actual family downloads. std::vector families; std::string baseUrl; std::string error; - if (!fetchRemoteFontManifest(installer, families, baseUrl, error)) { - JsonDocument errDoc; - errDoc["ok"] = false; - errDoc["error"] = error; - String out; - serializeJson(errDoc, out); - server->send(500, "application/json", out); - return; - } + { + HttpDownloader::Session manifestSession; + if (!fetchRemoteFontManifest(manifestSession, installer, families, baseUrl, error)) { + JsonDocument errDoc; + errDoc["ok"] = false; + errDoc["error"] = error; + String out; + serializeJson(errDoc, out); + server->send(500, "application/json", out); + return; + } + } // manifestSession destructor closes the TLS connection here std::vector targets; if (installAll) { @@ -1832,15 +1843,19 @@ void CrossPointWebServer::handleFontDownload() { families.clear(); families.shrink_to_fit(); + // One install session covers every family in this batch. TLS handshake + // happens once on the first file of the first family; subsequent files + // (within and across families) reuse the open keep-alive connection. + HttpDownloader::Session installSession; + size_t installedCount = 0; for (auto& family : targetCopies) { esp_task_wdt_reset(); yield(); - delay(500); // allow network stack to clean up sockets LOG_DBG("WEB", "Installing font family: %s", family.name.c_str()); - if (!installRemoteFamily(family, baseUrl, installer, error)) { + if (!installRemoteFamily(installSession, family, baseUrl, installer, error)) { JsonDocument errDoc; errDoc["ok"] = false; errDoc["error"] = error; diff --git a/src/network/HttpDownloader.cpp b/src/network/HttpDownloader.cpp index 070daa57..e7a7332a 100644 --- a/src/network/HttpDownloader.cpp +++ b/src/network/HttpDownloader.cpp @@ -1,294 +1,436 @@ #include "HttpDownloader.h" -#include +#include +#include #include -#include -#include -#include #include #include +#include #include +#include #include +#include #include -#include "util/UrlUtils.h" +// OtaUpdater workaround: the Arduino framework ships a stub esp_crt_bundle.h +// inside WiFiClientSecure that hides the real ESP-IDF symbol. Forward-declare +// the IDF entry point instead of including the header — see OtaUpdater.cpp. +extern "C" { +extern esp_err_t esp_crt_bundle_attach(void* conf); +} namespace { -class FileWriteStream final : public Stream { - public: - FileWriteStream(FsFile& file, size_t total, HttpDownloader::ProgressCallback progress) - : file_(file), total_(total), progress_(std::move(progress)), abortRequested_(false) {} +// ISRG Root X1 — Let's Encrypt's root CA. Pinned here because the Espressif +// crt_bundle's Subject-DN lookup can pick the wrong "ISRG Root X1" entry on +// cross-signed bundles and fail signature verification ("PK verify failed +// with error 0x4290" → MBEDTLS_ERR_X509_FATAL_ERROR -0x3000). We use this +// pin for raw.githubusercontent.com (Let's Encrypt-issued), and fall back to +// the default crt_bundle for all other hosts (DigiCert chain on +// github.com/api.github.com, etc.). +// +// Not-after: 2035-06-04. Update when Let's Encrypt rotates the root. +// Source: https://letsencrypt.org/certs/isrgrootx1.pem +constexpr const char ISRG_ROOT_X1_PEM[] = + "-----BEGIN CERTIFICATE-----\n" + "MIIFazCCA1OgAwIBAgIRAIIQz7DSQONZRGPgu2OCiwAwDQYJKoZIhvcNAQELBQAw\n" + "TzELMAkGA1UEBhMCVVMxKTAnBgNVBAoTIEludGVybmV0IFNlY3VyaXR5IFJlc2Vh\n" + "cmNoIEdyb3VwMRUwEwYDVQQDEwxJU1JHIFJvb3QgWDEwHhcNMTUwNjA0MTEwNDM4\n" + "WhcNMzUwNjA0MTEwNDM4WjBPMQswCQYDVQQGEwJVUzEpMCcGA1UEChMgSW50ZXJu\n" + "ZXQgU2VjdXJpdHkgUmVzZWFyY2ggR3JvdXAxFTATBgNVBAMTDElTUkcgUm9vdCBY\n" + "MTCCAiIwDQYJKoZIhvcNAQEBBQADggIPADCCAgoCggIBAK3oJHP0FDfzm54rVygc\n" + "h77ct984kIxuPOZXoHj3dcKi/vVqbvYATyjb3miGbESTtrFj/RQSa78f0uoxmyF+\n" + "0TM8ukj13Xnfs7j/EvEhmkvBioZxaUpmZmyPfjxwv60pIgbz5MDmgK7iS4+3mX6U\n" + "A5/TR5d8mUgjU+g4rk8Kb4Mu0UlXjIB0ttov0DiNewNwIRt18jA8+o+u3dpjq+sW\n" + "T8KOEUt+zwvo/7V3LvSye0rgTBIlDHCNAymg4VMk7BPZ7hm/ELNKjD+Jo2FR3qyH\n" + "B5T0Y3HsLuJvW5iB4YlcNHlsdu87kGJ55tukmi8mxdAQ4Q7e2RCOFvu396j3x+UC\n" + "B5iPNgiV5+I3lg02dZ77DnKxHZu8A/lJBdiB3QW0KtZB6awBdpUKD9jf1b0SHzUv\n" + "KBds0pjBqAlkd25HN7rOrFleaJ1/ctaJxQZBKT5ZPt0m9STJEadao0xAH0ahmbWn\n" + "OlFuhjuefXKnEgV4We0+UXgVCwOPjdAvBbI+e0ocS3MFEvzG6uBQE3xDk3SzynTn\n" + "jh8BCNAw1FtxNrQHusEwMFxIt4I7mKZ9YIqioymCzLq9gwQbooMDQaHWBfEbwrbw\n" + "qHyGO0aoSCqI3Haadr8faqU9GY/rOPNk3sgrDQoo//fb4hVC1CLQJ13hef4Y53CI\n" + "rU7m2Ys6xt0nUW7/vGT1M0NPAgMBAAGjQjBAMA4GA1UdDwEB/wQEAwIBBjAPBgNV\n" + "HRMBAf8EBTADAQH/MB0GA1UdDgQWBBR5tFnme7bl5AFzgAiIyBpY9umbbjANBgkq\n" + "hkiG9w0BAQsFAAOCAgEAVR9YqbyyqFDQDLHYGmkgJykIrGF1XIpu+ILlaS/V9lZL\n" + "ubhzEFnTIZd+50xx+7LSYK05qAvqFyFWhfFQDlnrzuBZ6brJFe+GnY+EgPbk6ZGQ\n" + "3BebYhtF8GaV0nxvwuo77x/Py9auJ/GpsMiu/X1+mvoiBOv/2X/qkSsisRcOj/KK\n" + "NFtY2PwByVS5uCbMiogziUwthDyC3+6WVwW6LLv3xLfHTjuCvjHIInNzktHCgKQ5\n" + "ORAzI4JMPJ+GslWYHb4phowim57iaztXOoJwTdwJx4nLCgdNbOhdjsnvzqvHu7Ur\n" + "TkXWStAmzOVyyghqpZXjFaH3pO3JLF+l+/+sKAIuvtd7u+Nxe5AW0wdeRlN8NwdC\n" + "jNPElpzVmbUq4JUagEiuTDkHzsxHpFKVK7q4+63SM1N95R1NbdWhscdCb+ZAJzVc\n" + "oyi3B43njTOQ5yOf+1CceWxG1bQVs5ZufpsMljq4Ui0/1lvh+wjChP4kqKOJ2qxq\n" + "4RgqsahDYVvTH9w7jXbyLeiNdd8XM2w9U/t7y0Ff/9yi0GE44Za4rF2LN9d11TPA\n" + "mRGunUHBcnWEvgJBQl9nJEiU0Zsnvgc/ubhPgXRR4Xq37Z0j4r7g1SgEEzwxA57d\n" + "emyPxgcYxn/eR44/KJ4EBs+lVDR3veyJm+kXQ99b21/+jh5Xos1AnX5iItreGCc=\n" + "-----END CERTIFICATE-----\n"; - size_t write(uint8_t byte) override { return write(&byte, 1); } +// mbedtls rejects certs whose notBefore lies in the future of the device +// clock, returning MBEDTLS_ERR_X509_CERT_VERIFY_FAILED (-0x2700). The +// ESP32-C3 has no battery-backed RTC, so cold-boot clocks default to 1970 +// (or, if HalClock restored from NVS, a stale "last known" time that may +// still predate the cert's notBefore). Fix it once per process before the +// first https request by running SNTP — WiFi is already up by the time +// runGet() is called, so this is essentially free. Subsequent calls reuse +// whatever the first attempt produced. +constexpr time_t MIN_PLAUSIBLE_EPOCH = 1735689600; // 2025-01-01 00:00:00 UTC +bool ensureClockForTls() { + static bool attempted = false; + if (attempted) return time(nullptr) >= MIN_PLAUSIBLE_EPOCH; + attempted = true; - size_t write(const uint8_t* buffer, size_t size) override { - // Write-through stream for HTTPClient::writeToStream with progress tracking. - const size_t written = file_.write(buffer, size); - if (written != size) { - writeOk_ = false; + if (HalClock::now() >= MIN_PLAUSIBLE_EPOCH && !HalClock::isApproximate()) { + return true; + } + LOG_INF("HTTP", "Clock looks unset/stale (epoch %ld); running SNTP before TLS", static_cast(time(nullptr))); + char err[64] = {0}; + if (!HalClock::syncNtp(err, sizeof(err))) { + LOG_ERR("HTTP", "SNTP sync failed: %s — TLS verification may fail until clock is set", err); + return false; + } + LOG_INF("HTTP", "SNTP sync complete; epoch now %ld", static_cast(time(nullptr))); + return true; +} + +// True if the URL's host is a *.githubusercontent.com host that's served by +// Let's Encrypt — needs the ISRG pin to dodge the crt_bundle Subject-collision +// bug. Adjust if more hosts hit the same issue. +bool needsLetsEncryptPin(const std::string& url) { + // Strip scheme://, then everything from the first / onward. + size_t schemeEnd = url.find("://"); + size_t hostStart = schemeEnd == std::string::npos ? 0 : schemeEnd + 3; + size_t hostEnd = url.find('/', hostStart); + if (hostEnd == std::string::npos) hostEnd = url.size(); + const std::string host = url.substr(hostStart, hostEnd - hostStart); + // raw.githubusercontent.com is the only one we've seen fail today. Match + // the suffix so codeload.githubusercontent.com / etc. get the same fix. + static constexpr const char* kSuffix = ".githubusercontent.com"; + const size_t suffixLen = strlen(kSuffix); + return host.size() >= suffixLen && host.compare(host.size() - suffixLen, suffixLen, kSuffix) == 0; +} + +// RX holds the response headers. 4096 fits real OPDS servers; GitHub's release +// CDN sends more and logs HTTP_HEADER "Buffer length is small", but that's +// non-fatal: the headers we read (Location, Content-Length) come first and +// survive. Smaller keeps contiguous heap free while WiFi and TLS are up. TX +// only carries our GET; the body streams in READ_CHUNK pieces. Matches +// upstream PR #2075 (port of OtaUpdater's PR #2074 sizing). +constexpr int HTTP_RX_BUF = 4096; +constexpr int HTTP_TX_BUF = 1024; +// Per-socket-op timeout. esp_http_client's timeout_ms is uint32, so unlike +// Arduino HTTPClient's uint16 setTimeout it doesn't silently truncate. 60s +// gives slow servers room to send their first headers. +constexpr int HTTP_TIMEOUT_MS = 60000; +constexpr size_t READ_CHUNK = 2048; + +struct Sink { + // Returns false to abort the transfer (e.g. SD write failure or user cancel). + std::function write; + HttpDownloader::ProgressCallback progress; + size_t total = 0; + size_t downloaded = 0; +}; + +bool isRedirect(int status) { + return status == 301 || status == 302 || status == 303 || status == 307 || status == 308; +} + +// Builds the esp_http_client_config_t for a given URL, picking the appropriate +// TLS root strategy (pinned ISRG vs default crt_bundle) based on the host. +void configureClient(const std::string& url, esp_http_client_config_t& config) { + config.url = url.c_str(); + config.buffer_size = HTTP_RX_BUF; + config.buffer_size_tx = HTTP_TX_BUF; + config.timeout_ms = HTTP_TIMEOUT_MS; + if (needsLetsEncryptPin(url)) { + config.cert_pem = ISRG_ROOT_X1_PEM; + config.cert_len = sizeof(ISRG_ROOT_X1_PEM); + } else { + config.crt_bundle_attach = esp_crt_bundle_attach; + } + config.keep_alive_enable = true; +} + +void applyRequestHeaders(esp_http_client_handle_t client, const std::string& username, const std::string& password) { + esp_http_client_set_header(client, "User-Agent", "CrossPoint-ESP32-" CROSSPOINT_VERSION); + if (!username.empty() && !password.empty()) { + const std::string credentials = username + ":" + password; + const String header = "Basic " + base64::encode(credentials.c_str()); + esp_http_client_set_header(client, "Authorization", header.c_str()); + } +} + +// Performs the per-request work on an already-initialised client: open the +// connection (does the TLS handshake on first call; reuses the open TCP/TLS +// connection on subsequent calls per HTTP keep-alive), read headers, follow +// redirects, then stream the body. Used by both the standalone runGet and the +// Session-based path. +HttpDownloader::DownloadError performGet(esp_http_client_handle_t client, Sink& sink) { + esp_err_t err = esp_http_client_open(client, 0); + if (err != ESP_OK) { + int tlsCode = 0; + int tlsFlags = 0; + esp_http_client_get_and_clear_last_tls_error(client, &tlsCode, &tlsFlags); + LOG_ERR("HTTP", "open failed: %s (tls_code=-0x%04x, tls_flags=0x%08x)", esp_err_to_name(err), -tlsCode, tlsFlags); + return HttpDownloader::HTTP_ERROR; + } + int64_t contentLength = esp_http_client_fetch_headers(client); + int status = esp_http_client_get_status_code(client); + for (int hop = 0; isRedirect(status) && hop < 5; ++hop) { + if (esp_http_client_set_redirection(client) != ESP_OK) break; + err = esp_http_client_open(client, 0); + if (err != ESP_OK) { + LOG_ERR("HTTP", "redirect open failed: %s", esp_err_to_name(err)); + return HttpDownloader::HTTP_ERROR; } - downloaded_ += written; - if (progress_) { - if (!progress_(downloaded_, total_)) { - abortRequested_ = true; - return 0; - } - } - return written; + contentLength = esp_http_client_fetch_headers(client); + status = esp_http_client_get_status_code(client); } - int available() override { return 0; } - int read() override { return -1; } - int peek() override { return -1; } - void flush() override { file_.flush(); } + if (status != 200) { + LOG_ERR("HTTP", "unexpected status: %d", status); + return HttpDownloader::HTTP_ERROR; + } - size_t downloaded() const { return downloaded_; } - bool ok() const { return writeOk_; } - bool aborted() const { return abortRequested_; } + sink.total = contentLength > 0 ? static_cast(contentLength) : 0; - private: - FsFile& file_; - size_t total_; - size_t downloaded_ = 0; - bool writeOk_ = true; - bool abortRequested_ = false; - HttpDownloader::ProgressCallback progress_; + std::unique_ptr buf(new (std::nothrow) char[READ_CHUNK]); + if (!buf) { + LOG_ERR("HTTP", "OOM: %u byte read buffer", (unsigned)READ_CHUNK); + return HttpDownloader::HTTP_ERROR; + } + + bool aborted = false; + while (true) { + const int read = esp_http_client_read(client, buf.get(), READ_CHUNK); + if (read < 0) { + LOG_ERR("HTTP", "read error after %zu bytes", sink.downloaded); + return HttpDownloader::HTTP_ERROR; + } + if (read == 0) break; // all data received + if (!sink.write(reinterpret_cast(buf.get()), read)) { + aborted = true; + break; + } + sink.downloaded += read; + if (sink.progress && sink.total > 0) { + if (!sink.progress(sink.downloaded, sink.total)) { + aborted = true; + break; + } + } + } + + if (aborted) { + return HttpDownloader::ABORTED; + } + if (!esp_http_client_is_complete_data_received(client)) { + LOG_ERR("HTTP", "incomplete: got %zu of %zu bytes", sink.downloaded, sink.total); + return HttpDownloader::HTTP_ERROR; + } + return HttpDownloader::OK; +} + +// Runs once per http call (or once per session for reused sessions): logs +// heap stats and ensures the wall clock is set so TLS cert-date validation +// can succeed. +void logPreCallContext(const std::string& url) { + LOG_DBG("HTTP", "Heap free: %u, largest block: %u", esp_get_free_heap_size(), + heap_caps_get_largest_free_block(MALLOC_CAP_DEFAULT)); + if (url.compare(0, 8, "https://") == 0) { + ensureClockForTls(); + } +} + +// Streams a GET body through sink.write in READ_CHUNK pieces. One-shot client: +// creates a fresh esp_http_client per call. See HttpDownloader::Session for +// the reusable variant that keeps the TLS handshake alive across files. +HttpDownloader::DownloadError runGet(const std::string& url, const std::string& username, const std::string& password, + Sink& sink) { + logPreCallContext(url); + + esp_http_client_config_t config = {}; + configureClient(url, config); + + esp_http_client_handle_t client = esp_http_client_init(&config); + if (!client) { + LOG_ERR("HTTP", "client init failed"); + return HttpDownloader::HTTP_ERROR; + } + applyRequestHeaders(client, username, password); + const HttpDownloader::DownloadError result = performGet(client, sink); + esp_http_client_cleanup(client); + return result; +} +} // namespace + +// ---- Session implementation ---- + +struct HttpDownloader::Session::Impl { + esp_http_client_handle_t client = nullptr; + std::string host; // scheme+authority of the first request; used to detect cross-host reuse + + ~Impl() { + if (client) { + esp_http_client_cleanup(client); + } + } }; + +HttpDownloader::Session::Session() : impl_(std::make_unique()) {} +HttpDownloader::Session::~Session() = default; + +namespace { +// Extract "scheme://host[:port]" from a URL — used to detect when a Session +// is asked to reuse across hosts (esp_http_client supports it via set_url but +// it tears down and reopens the TLS connection, losing the heap win). +std::string schemeAuthority(const std::string& url) { + size_t schemeEnd = url.find("://"); + if (schemeEnd == std::string::npos) return ""; + size_t pathStart = url.find('/', schemeEnd + 3); + return url.substr(0, pathStart == std::string::npos ? url.size() : pathStart); +} + +// Internal: initialise the session's underlying esp_http_client for the given +// URL. Used both for the first call and for reconnect-after-failure. +bool initSessionClient(HttpDownloader::Session::Impl* impl, const std::string& url) { + esp_http_client_config_t config = {}; + configureClient(url, config); + impl->client = esp_http_client_init(&config); + if (!impl->client) { + LOG_ERR("HTTP", "session client init failed"); + return false; + } + impl->host = schemeAuthority(url); + return true; +} + +HttpDownloader::DownloadError runGetOnSession(HttpDownloader::Session& session, const std::string& url, + const std::string& username, const std::string& password, Sink& sink) { + auto* impl = session.impl(); + + if (impl->client == nullptr) { + logPreCallContext(url); + if (!initSessionClient(impl, url)) { + return HttpDownloader::HTTP_ERROR; + } + } else { + const std::string nextHost = schemeAuthority(url); + if (nextHost != impl->host) { + LOG_INF("HTTP", "Session URL host changed (%s -> %s); reopening", impl->host.c_str(), nextHost.c_str()); + impl->host = nextHost; + } + esp_err_t setUrlErr = esp_http_client_set_url(impl->client, url.c_str()); + if (setUrlErr != ESP_OK) { + LOG_ERR("HTTP", "set_url failed: %s", esp_err_to_name(setUrlErr)); + return HttpDownloader::HTTP_ERROR; + } + } + + applyRequestHeaders(impl->client, username, password); + HttpDownloader::DownloadError result = performGet(impl->client, sink); + + // If a reused client's open() failed (e.g. server closed idle keep-alive), + // tear it down and try once more from a clean state. Critical for the + // manifest→file flow where the user can sit on the family list for a while + // before pressing confirm. + if (result == HttpDownloader::HTTP_ERROR && sink.downloaded == 0) { + LOG_INF("HTTP", "Session reuse failed; reinitialising client and retrying once"); + esp_http_client_cleanup(impl->client); + impl->client = nullptr; + if (!initSessionClient(impl, url)) { + return HttpDownloader::HTTP_ERROR; + } + applyRequestHeaders(impl->client, username, password); + result = performGet(impl->client, sink); + } + return result; +} } // namespace bool HttpDownloader::fetchUrl(const std::string& url, Stream& outContent, const std::string& username, const std::string& password) { - std::unique_ptr client; - if (UrlUtils::isHttpsUrl(url)) { - auto* secureClient = new NetworkClientSecure(); - secureClient->setInsecure(); - client.reset(secureClient); - } else { - client.reset(new NetworkClient()); - } - HTTPClient http; - LOG_DBG("HTTP", "Fetching: %s", url.c_str()); - - http.begin(*client, url.c_str()); - http.setReuse(false); - http.setFollowRedirects(HTTPC_FORCE_FOLLOW_REDIRECTS); - http.setTimeout(30000); - http.addHeader("User-Agent", "CrossPoint-ESP32-" CROSSPOINT_VERSION); - http.addHeader("Connection", "close"); - - if (!username.empty() || !password.empty()) { - std::string credentials = username + ":" + password; - String encoded = base64::encode(credentials.c_str()); - http.addHeader("Authorization", "Basic " + encoded); - } - - const int httpCode = http.GET(); - if (httpCode != HTTP_CODE_OK) { - LOG_ERR("HTTP", "Fetch failed: %d", httpCode); - http.end(); - client->stop(); - return false; - } - - http.writeToStream(&outContent); - - http.end(); - if (client) { - client->stop(); - } - - LOG_DBG("HTTP", "Fetch success"); - return true; + Sink sink; + sink.write = [&outContent](const uint8_t* data, size_t len) { return outContent.write(data, len) == len; }; + return runGet(url, username, password, sink) == OK; } bool HttpDownloader::fetchUrl(const std::string& url, std::string& outContent, const std::string& username, const std::string& password) { - StreamString stream; - if (!fetchUrl(url, stream, username, password)) { - return false; - } - outContent = stream.c_str(); - return true; + LOG_DBG("HTTP", "Fetching: %s", url.c_str()); + outContent.clear(); // start clean; the sink appends, so don't carry prior content + Sink sink; + sink.write = [&outContent](const uint8_t* data, size_t len) { + outContent.append(reinterpret_cast(data), len); + return true; + }; + return runGet(url, username, password, sink) == OK; } +namespace { +// Common file-sink plumbing used by both downloadToFile overloads. +HttpDownloader::DownloadError finishFileDownload(HttpDownloader::DownloadError result, const std::string& destPath, + FsFile& file, size_t downloaded) { + // Flush before any remove() on the same path; DESTRUCTOR_CLOSES_FILE would + // otherwise close only after the remove. + file.flush(); + file.close(); + if (result != HttpDownloader::OK) { + Storage.remove(destPath.c_str()); + return result; + } + if (downloaded == 0) { + LOG_ERR("HTTP", "no data received"); + Storage.remove(destPath.c_str()); + return HttpDownloader::HTTP_ERROR; + } + LOG_DBG("HTTP", "Downloaded %zu bytes", downloaded); + return HttpDownloader::OK; +} +} // namespace + HttpDownloader::DownloadError HttpDownloader::downloadToFile(const std::string& url, const std::string& destPath, ProgressCallback progress, const std::string& username, const std::string& password) { - std::unique_ptr client; - if (UrlUtils::isHttpsUrl(url)) { - auto* secureClient = new NetworkClientSecure(); - secureClient->setInsecure(); - client.reset(secureClient); - } else { - client.reset(new NetworkClient()); - } - HTTPClient http; - LOG_DBG("HTTP", "Downloading: %s", url.c_str()); LOG_DBG("HTTP", "Destination: %s", destPath.c_str()); - LOG_DBG("HTTP", "Heap free: %u, largest block: %u", esp_get_free_heap_size(), - heap_caps_get_largest_free_block(MALLOC_CAP_DEFAULT)); - http.begin(*client, url.c_str()); - http.setReuse(false); - http.setFollowRedirects(HTTPC_FORCE_FOLLOW_REDIRECTS); - http.setTimeout(30000); - http.addHeader("User-Agent", "CrossPoint-ESP32-" CROSSPOINT_VERSION); - http.addHeader("Connection", "close"); - - if (!username.empty() || !password.empty()) { - std::string credentials = username + ":" + password; - String encoded = base64::encode(credentials.c_str()); - http.addHeader("Authorization", "Basic " + encoded); - } - - const int httpCode = http.GET(); - if (httpCode != HTTP_CODE_OK) { - LOG_ERR("HTTP", "Download failed: %d", httpCode); - http.end(); - client->stop(); - return HTTP_ERROR; - } - - const int64_t reportedLength = http.getSize(); - const size_t contentLength = reportedLength > 0 ? static_cast(reportedLength) : 0; - if (contentLength > 0) { - LOG_DBG("HTTP", "Content-Length: %zu", contentLength); - } else { - LOG_DBG("HTTP", "Content-Length: unknown"); - } - - // Remove existing file if present if (Storage.exists(destPath.c_str())) { Storage.remove(destPath.c_str()); } - - // Open file for writing FsFile file; if (!Storage.openFileForWrite("HTTP", destPath.c_str(), file)) { LOG_ERR("HTTP", "Failed to open file for writing: %s", destPath.c_str()); - http.end(); return FILE_ERROR; } - LOG_DBG("HTTP", "Opened destination file for writing: %s", destPath.c_str()); + Sink sink; + sink.progress = std::move(progress); + sink.write = [&file](const uint8_t* data, size_t len) { return file.write(data, len) == len; }; - int writeResult = -1; - size_t downloaded = 0; - bool writeOk = true; + const DownloadError result = runGet(url, username, password, sink); + return finishFileDownload(result, destPath, file, sink.downloaded); +} - if (contentLength > 0) { - NetworkClient& stream = http.getStream(); - uint8_t buffer[1024]; - writeResult = 1; - bool aborted = false; - unsigned long lastAvailLog = millis(); - unsigned long startMs = millis(); - unsigned long lastProgressPoll = millis(); +HttpDownloader::DownloadError HttpDownloader::downloadToFile(Session& session, const std::string& url, + const std::string& destPath, ProgressCallback progress, + const std::string& username, const std::string& password) { + LOG_DBG("HTTP", "Downloading (session): %s", url.c_str()); + LOG_DBG("HTTP", "Destination: %s", destPath.c_str()); - while (http.connected() && downloaded < contentLength) { - size_t available = stream.available(); - if (available > 0) { - size_t toRead = available > sizeof(buffer) ? sizeof(buffer) : available; - if (downloaded + toRead > contentLength) { - toRead = contentLength - downloaded; - } - int readSize = stream.readBytes(reinterpret_cast(buffer), toRead); - if (readSize > 0) { - if (file.write(buffer, readSize) != static_cast(readSize)) { - LOG_ERR("HTTP", "File write failed: wrote %d/%zu bytes to %s", readSize, toRead, destPath.c_str()); - writeOk = false; - writeResult = -1; - break; - } - downloaded += readSize; - if (progress && !progress(downloaded, contentLength)) { - LOG_DBG("HTTP", "Download aborted by callback at %zu/%zu", downloaded, contentLength); - aborted = true; - break; - } - } else { - LOG_ERR("HTTP", "Stream readBytes returned %d after %zu bytes", readSize, downloaded); - break; - } - } else { - if (millis() - lastProgressPoll > 100) { - if (progress && !progress(downloaded, contentLength)) { - LOG_DBG("HTTP", "Download aborted by callback while waiting for data at %zu/%zu", downloaded, - contentLength); - aborted = true; - break; - } - lastProgressPoll = millis(); - } - if (millis() - lastAvailLog > 2000) { - LOG_DBG("HTTP", "Waiting for available data: downloaded=%zu connected=%d elapsed=%lums", downloaded, - http.connected(), millis() - startMs); - lastAvailLog = millis(); - } - delay(1); - } - } - - if (aborted) { - file.flush(); - file.close(); - http.end(); - client->stop(); - Storage.remove(destPath.c_str()); - return ABORTED; - } - - if (downloaded != contentLength) { - LOG_ERR("HTTP", "Download size mismatch after loop: got %zu expected %zu", downloaded, contentLength); - writeResult = -1; - } - } else { - FileWriteStream fileStream(file, contentLength, progress); - writeResult = http.writeToStream(&fileStream); - downloaded = fileStream.downloaded(); - writeOk = fileStream.ok(); - if (fileStream.aborted()) { - file.flush(); - file.close(); - http.end(); - client->stop(); - Storage.remove(destPath.c_str()); - return ABORTED; - } - } - - // Flush before closing to ensure data is written to the SD card. - // Without this, Storage.exists() might return false immediately after - // even though the file was written (FAT not yet updated on disk). - file.flush(); - file.close(); - http.end(); - client->stop(); - - if (writeResult < 0) { - LOG_ERR("HTTP", "writeToStream error: %d (downloaded %zu)", writeResult, downloaded); + if (Storage.exists(destPath.c_str())) { Storage.remove(destPath.c_str()); - return HTTP_ERROR; } - - LOG_DBG("HTTP", "Downloaded %zu bytes", downloaded); - - // Guard against partial writes even if HTTPClient completes. - if (!writeOk) { - LOG_ERR("HTTP", "Write failed during download (downloaded %zu)", downloaded); - Storage.remove(destPath.c_str()); + FsFile file; + if (!Storage.openFileForWrite("HTTP", destPath.c_str(), file)) { + LOG_ERR("HTTP", "Failed to open file for writing: %s", destPath.c_str()); return FILE_ERROR; } - // Verify download size if known - if (contentLength > 0 && downloaded != contentLength) { - LOG_ERR("HTTP", "Size mismatch: got %zu, expected %zu", downloaded, contentLength); - Storage.remove(destPath.c_str()); - return HTTP_ERROR; - } + Sink sink; + sink.progress = std::move(progress); + sink.write = [&file](const uint8_t* data, size_t len) { return file.write(data, len) == len; }; - return OK; + const DownloadError result = runGetOnSession(session, url, username, password, sink); + return finishFileDownload(result, destPath, file, sink.downloaded); } diff --git a/src/network/HttpDownloader.h b/src/network/HttpDownloader.h index 5129549c..fb7ef779 100644 --- a/src/network/HttpDownloader.h +++ b/src/network/HttpDownloader.h @@ -2,14 +2,18 @@ #include #include +#include #include /** - * HTTP client utility for fetching content and downloading files. - * Wraps NetworkClientSecure and HTTPClient for HTTPS requests. + * HTTP client utility for fetching content and downloading files. Built on + * esp_http_client: https is verified against the CA bundle, plain http is + * used for local servers (transport is chosen from the URL scheme). Ported + * from upstream PR #2075. */ class HttpDownloader { public: + // Progress callback. Return false to abort the transfer. using ProgressCallback = std::function; enum DownloadError { @@ -19,6 +23,33 @@ class HttpDownloader { ABORTED, }; + /** + * Reusable HTTP+TLS session. Holding one of these across multiple + * downloadToFile() calls keeps a single esp_http_client_handle_t alive, + * so the TLS handshake (≈36 KB of contiguous mbedtls buffers, RSA chain + * verify, etc.) runs once instead of per-file. This is the structural fix + * for back-to-back HTTPS calls failing on a fragmented heap. + * + * Usage: construct one, pass to downloadToFile(session, …) for every file + * served by the same host. Destroying it closes the connection. + * + * Cross-host reuse is technically supported (esp_http_client_set_url tears + * down and reopens) but defeats the heap win — group calls by host. + */ + class Session { + public: + Session(); + ~Session(); + Session(const Session&) = delete; + Session& operator=(const Session&) = delete; + + struct Impl; + Impl* impl() const { return impl_.get(); } + + private: + std::unique_ptr impl_; + }; + /** * Fetch text content from a URL with optional credentials. */ @@ -34,4 +65,13 @@ class HttpDownloader { static DownloadError downloadToFile(const std::string& url, const std::string& destPath, ProgressCallback progress = nullptr, const std::string& username = "", const std::string& password = ""); + + /** + * Session-based variant. The first call on a fresh session opens the + * connection (TLS handshake, cert verification, etc.); subsequent calls to + * URLs on the same host reuse the open client and skip the handshake. + */ + static DownloadError downloadToFile(Session& session, const std::string& url, const std::string& destPath, + ProgressCallback progress = nullptr, const std::string& username = "", + const std::string& password = ""); }; diff --git a/src/util/UrlUtils.cpp b/src/util/UrlUtils.cpp index 9bcb9fa2..7161624c 100644 --- a/src/util/UrlUtils.cpp +++ b/src/util/UrlUtils.cpp @@ -174,8 +174,6 @@ std::string normalizeJoinedPath(const std::string_view baseDir, const std::strin } } // namespace -bool isHttpsUrl(const std::string& url) { return url.rfind("https://", 0) == 0; } - std::string ensureProtocol(const std::string& url) { if (url.find("://") == std::string::npos) { return "http://" + url; diff --git a/src/util/UrlUtils.h b/src/util/UrlUtils.h index dee3e97e..bfe7352c 100644 --- a/src/util/UrlUtils.h +++ b/src/util/UrlUtils.h @@ -3,11 +3,6 @@ namespace UrlUtils { -/** - * Check if URL uses HTTPS protocol - */ -bool isHttpsUrl(const std::string& url); - /** * Prepend http:// if no protocol specified (server will redirect to https if needed) */ From dc15186e22b4a5af0605b9a78797159bb6231ebc Mon Sep 17 00:00:00 2001 From: jpirnay Date: Wed, 20 May 2026 19:31:37 +0200 Subject: [PATCH 4/6] Proper commit --- open-x4-sdk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/open-x4-sdk b/open-x4-sdk index 570e9c4e..4d4c6678 160000 --- a/open-x4-sdk +++ b/open-x4-sdk @@ -1 +1 @@ -Subproject commit 570e9c4ee01c01abba9df52b4b3bda7ff610910b +Subproject commit 4d4c66787ff655adc93a1f37811610552a73aa1c From 78e1d2eacb4c96fcf67490fce0151fdfb01e8031 Mon Sep 17 00:00:00 2001 From: jpirnay Date: Wed, 20 May 2026 19:34:27 +0200 Subject: [PATCH 5/6] Review changes --- src/FontInstaller.cpp | 13 +++++++++ src/FontInstaller.h | 5 ++++ .../settings/FontDownloadActivity.cpp | 28 +++++++++++++++++-- 3 files changed, 43 insertions(+), 3 deletions(-) diff --git a/src/FontInstaller.cpp b/src/FontInstaller.cpp index fa099d24..0f03ac8d 100644 --- a/src/FontInstaller.cpp +++ b/src/FontInstaller.cpp @@ -30,6 +30,19 @@ bool FontInstaller::isValidFamilyName(const char* name) { return true; } +bool FontInstaller::isValidFontFileName(const char* name) { + if (name == nullptr || name[0] == '\0') return false; + // Matches the cap used by the web-server manifest parser; the SD-side path buffer is 128 + // bytes including the family prefix, so 60 leaves plenty of headroom. + static constexpr size_t MAX_FONT_FILE_NAME_LEN = 60; + const size_t nameLen = strlen(name); + if (nameLen > MAX_FONT_FILE_NAME_LEN) return false; + if (name[0] == '/') return false; + if (strchr(name, '\\') != nullptr) return false; + if (strstr(name, "..") != nullptr) return false; + return true; +} + bool FontInstaller::ensureFamilyDir(const char* familyName) { if (!isValidFamilyName(familyName)) { LOG_ERR("FONT", "Invalid family name: %s", familyName ? familyName : ""); diff --git a/src/FontInstaller.h b/src/FontInstaller.h index bb33fadd..3ef0190c 100644 --- a/src/FontInstaller.h +++ b/src/FontInstaller.h @@ -25,6 +25,11 @@ class FontInstaller { /// Validate a family name: alphanumeric + hyphen + underscore only, no path traversal. static bool isValidFamilyName(const char* name); + /// Validate a font file name as it appears in a manifest entry: non-empty, length-bounded, + /// no absolute paths, no backslashes, no traversal components. Slashes are allowed for + /// "/"-style entries. + static bool isValidFontFileName(const char* name); + /// Ensure /.crosspoint/fonts// directory exists. bool ensureFamilyDir(const char* familyName); diff --git a/src/activities/settings/FontDownloadActivity.cpp b/src/activities/settings/FontDownloadActivity.cpp index 5498eb95..43973d01 100644 --- a/src/activities/settings/FontDownloadActivity.cpp +++ b/src/activities/settings/FontDownloadActivity.cpp @@ -130,7 +130,13 @@ bool FontDownloadActivity::fetchAndParseManifest() { family.description = fObj["description"] | ""; // styles[] in the JSON is intentionally ignored — see ManifestFamily. + if (!FontInstaller::isValidFamilyName(family.name.c_str())) { + LOG_ERR("FONT", "Manifest entry rejected, invalid family name: %s", family.name.c_str()); + continue; + } + family.totalSize = 0; + bool fileNamesOk = true; for (JsonObject fileObj : fObj["files"].as()) { ManifestFile file; file.name = fileObj["name"] | ""; @@ -139,9 +145,15 @@ bool FontDownloadActivity::fetchAndParseManifest() { file.crc32 = fileObj["crc32"].as(); file.hasCrc32 = true; } + if (!FontInstaller::isValidFontFileName(file.name.c_str())) { + LOG_ERR("FONT", "Manifest entry rejected, invalid file name in %s: %s", family.name.c_str(), file.name.c_str()); + fileNamesOk = false; + break; + } family.totalSize += file.size; family.files.push_back(std::move(file)); } + if (!fileNamesOk) continue; family.installed = fontInstaller_.isFamilyInstalled(family.name.c_str()); @@ -225,7 +237,8 @@ bool readU8(FsFile& f, uint8_t& v) { return f.read(&v, 1) == 1; } bool readU32(FsFile& f, uint32_t& v) { uint8_t buf[4]; if (f.read(buf, 4) != 4) return false; - v = buf[0] | (buf[1] << 8) | (buf[2] << 16) | (buf[3] << 24); + v = static_cast(buf[0]) | (static_cast(buf[1]) << 8) | (static_cast(buf[2]) << 16) | + (static_cast(buf[3]) << 24); return true; } bool readStr(FsFile& f, std::string& s) { @@ -460,8 +473,17 @@ void FontDownloadActivity::downloadFamily(int familyIdx) { downloadingFamilyHasResumable_ = family.hasResumableDownload; // Restore families_ regardless of success/error/abort outcome, then merge - // back the mutations the impl made on the local family copy. - if (restoreFamiliesFromSd() && familyIdx >= 0 && familyIdx < static_cast(families_.size())) { + // back the mutations the impl made on the local family copy. Without the + // restored manifest the activity can't render the family list, so a failed + // restore is fatal — drop to ERROR rather than continuing with empty state. + if (!restoreFamiliesFromSd()) { + RenderLock lock(*this); + state_ = ERROR; + pendingErrorAction_ = PendingFontAction::Download; + errorMessage_ = "Failed to restore manifest"; + return; + } + if (familyIdx >= 0 && familyIdx < static_cast(families_.size())) { families_[familyIdx].installed = family.installed; families_[familyIdx].hasUpdate = family.hasUpdate; families_[familyIdx].hasResumableDownload = family.hasResumableDownload; From ecd20f9512d170dc8ee5b480c19d800a566d57ba Mon Sep 17 00:00:00 2001 From: jpirnay Date: Wed, 20 May 2026 20:57:42 +0200 Subject: [PATCH 6/6] Review changes --- src/FontInstaller.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/FontInstaller.cpp b/src/FontInstaller.cpp index 0f03ac8d..b5695b9d 100644 --- a/src/FontInstaller.cpp +++ b/src/FontInstaller.cpp @@ -40,6 +40,14 @@ bool FontInstaller::isValidFontFileName(const char* name) { if (name[0] == '/') return false; if (strchr(name, '\\') != nullptr) return false; if (strstr(name, "..") != nullptr) return false; + // Reject multiple slashes or trailing slash to limit directory depth + int slashCount = 0; + for (const char* p = name; *p; ++p) { + if (*p == '/') { + ++slashCount; + if (slashCount > 1 || *(p + 1) == '\0' || *(p + 1) == '/') return false; + } + } return true; }