diff --git a/src/shell/package/CLAUDE.md b/src/shell/package/CLAUDE.md index 0caf183..7835e65 100644 --- a/src/shell/package/CLAUDE.md +++ b/src/shell/package/CLAUDE.md @@ -53,8 +53,12 @@ belong to the verbs. "Did this call create it" is structural: only exclusively-created paths are recorded, resolved absolute at record time so a later CWD change cannot re-aim the delete. "Did anything ever reference it" is a **contract the import verb must - honour**: it MUST call `markIndexCommitted()` at the moment it commits the index, - after which `rollback()` refuses and `writeLandedFile` refuses. + honour**: it MUST call `markIndexCommitted()` only AFTER the index write has + returned success — calling it before, then having that write fail, strands the + landed files with no index entry and a journal that now refuses to roll them + back — after which `rollback()` refuses and `writeLandedFile` refuses. (Destroying + an armed journal without calling either does NOT roll it back — see + `LandedFileJournal`'s own doc comment.) - **Both pickers ride `GetUserFileName`** — mode 1 for import, mode 0 for export. There is no platform split and no fallback: `main.cpp` defines `REAPERAPI_IMPLEMENT` without `REAPERAPI_MINIMAL` and aborts the extension load if any single name fails @@ -80,7 +84,11 @@ belong to the verbs. pair format but are `[verify — DAW]` on all three platforms — neither picker is exercised outside a live REAPER session. `GetUserFileName` also takes no owner window, so dialog parenting is REAPER's to do; the superseded Win32 path passed - `GetMainHwnd()` explicitly. + `GetMainHwnd()` explicitly. Also `[verify — DAW]`: whether mode 0's picker appends + an extension from `extension_list` when the user omits one — `pickPackageSavePath` + re-appends `.rsbank` itself so the returned path is correct regardless of how that + lands (the superseded Win32 path had `ofn.lpstrDefExt` for this; `GetUserFileName` + has no equivalent parameter). - `pickPackageSavePath`'s `suggestedPath` doubles as the dialog's starting directory when it is a full path. The verbs should seed it from the project directory — passing a bare name leaves the dialog on REAPER's process working directory, which diff --git a/src/shell/package/package_io.cpp b/src/shell/package/package_io.cpp index 901dc3f..26cd7e6 100644 --- a/src/shell/package/package_io.cpp +++ b/src/shell/package/package_io.cpp @@ -1,11 +1,13 @@ // package_io.cpp — see package_io.h for the seam's contract. Non-throwing at the -// boundary: every filesystem call uses the error_code form, so no filesystem_error -// crosses into a REAPER action body. +// boundary about filesystem_error: every filesystem call uses the error_code form. +// Allocation can still throw bad_alloc — readRange's sanity ceiling exists to keep +// that surface small, not to remove it. #include "shell/package/package_io.h" #include #include +#include #include #include "shell/package/package_path.h" @@ -16,6 +18,7 @@ #include #include #else +#include #include #include #endif @@ -147,7 +150,15 @@ PackageFileReader::PackageFileReader(const std::string& srcAbsPath) { PayloadBuffer PackageFileReader::readRange(std::uint64_t offset, std::uint64_t length) { // Overflow-safe range check: length is capped by the real file size before any // allocation happens, so a hostile offset/length pair cannot demand the moon. - if (!ok_ || length == 0 || length > size_ || offset > size_ - length) { + // Also rejected here rather than truncated: a length that would not fit in + // size_t (possible on a 32-bit build, where streamsize below stays 64-bit and + // so would read past a truncated allocation) and a length past the sanity + // ceiling, which exists so a merely large-but-real file size can't still hand + // std::vector a multi-gigabyte demand. + constexpr std::uint64_t kMaxReadRangeBytes = std::uint64_t{4} << 30; // 4 GiB + if (!ok_ || length == 0 || length > size_ || offset > size_ - length || + length > kMaxReadRangeBytes || + length > static_cast(std::numeric_limits::max())) { return PayloadBuffer{}; } in_.clear(); // a prior failed read must not poison this one @@ -208,6 +219,7 @@ bool writeFileExclusive(const std::string& absPath, const PayloadBuffer& payload static_cast(chunk)); #else const ssize_t n = ::write(fd, payload.data() + written, chunk); + if (n < 0 && errno == EINTR) continue; // a signal on the UI thread isn't a failure #endif if (n <= 0) { ok = false; @@ -241,7 +253,7 @@ std::vector listFolderFileNames(const std::string& dirAbsPath) { const auto& entry = *it; std::error_code reg_ec; if (!entry.is_regular_file(reg_ec)) continue; // skip subdirs / specials - names.push_back(entry.path().filename().u8string()); // never .string(): ANSI + names.push_back(pathToUtf8(entry.path().filename())); // never .string(): ANSI } std::sort(names.begin(), names.end()); return names; diff --git a/src/shell/package/package_io.h b/src/shell/package/package_io.h index 332a5f1..a33f66e 100644 --- a/src/shell/package/package_io.h +++ b/src/shell/package/package_io.h @@ -89,7 +89,9 @@ public: bool ok() const { return ok_; } std::uint64_t fileSize() const { return size_; } // Bytes [offset, offset+length). Range-checked against the real file size, so a - // hostile layout can never demand an allocation past the file's end. + // hostile layout can never demand an allocation past the file's end, and capped + // against a 4 GiB sanity ceiling so a merely large-but-real file can't still + // force a multi-gigabyte allocation out of one call. PayloadBuffer readRange(std::uint64_t offset, std::uint64_t length); private: diff --git a/src/shell/package/package_path.h b/src/shell/package/package_path.h index 9982496..e70db66 100644 --- a/src/shell/package/package_path.h +++ b/src/shell/package/package_path.h @@ -1,9 +1,9 @@ -// shell/package/package_path — the ONE narrow-string -> fs::path conversion for this -// seam. std::filesystem decodes a narrow path through the RUNTIME ANSI code page on -// Windows (measured: GetACP() == 1252 here), never UTF-8, so a bare +// shell/package/package_path — the ONE narrow-string <-> fs::path conversion pair for +// this seam. std::filesystem decodes a narrow path through the RUNTIME ANSI code page +// on Windows (measured: GetACP() == 1252 here), never UTF-8, so a bare // fs::path(std::string) turns every non-ASCII path this repo's UTF-8 convention // produces into mojibake. u8path is the C++17 spelling; it is deprecated in C++20, so -// a standard bump replaces the body here rather than at every call site. +// a standard bump replaces both bodies here rather than at every call site. #pragma once @@ -16,4 +16,11 @@ inline std::filesystem::path utf8Path(const std::string& utf8) { return std::filesystem::u8path(utf8); } +// u8string() returns std::u8string in C++20 — this is the one place that narrows it +// back to std::string, so a standard bump only widens this one body. +inline std::string pathToUtf8(const std::filesystem::path& path) { + const auto u8 = path.u8string(); + return std::string(u8.begin(), u8.end()); +} + } // namespace reasampler diff --git a/src/shell/package/package_pickers.cpp b/src/shell/package/package_pickers.cpp index 75af2a0..16395a6 100644 --- a/src/shell/package/package_pickers.cpp +++ b/src/shell/package/package_pickers.cpp @@ -5,6 +5,9 @@ #include "shell/package/package_pickers.h" +#include +#include + #define REAPERAPI_MINIMAL #define REAPERAPI_WANT_GetUserFileName #include "reaper_plugin_functions.h" @@ -30,6 +33,14 @@ bool runPicker(int mode, const char* caption, const char* initial, return !outAbsPath.empty(); } +bool hasCaseInsensitiveSuffix(const std::string& path, const std::string& suffix) { + if (path.size() < suffix.size()) return false; + return std::equal(suffix.rbegin(), suffix.rend(), path.rbegin(), + [](unsigned char a, unsigned char b) { + return std::tolower(a) == std::tolower(b); + }); +} + } // namespace bool pickPackageForImport(std::string& outAbsPath) { @@ -39,7 +50,14 @@ bool pickPackageForImport(std::string& outAbsPath) { bool pickPackageSavePath(const std::string& suggestedPath, std::string& outAbsPath) { // [verify — DAW] GetUserFileName takes no owner window, so the dialog's parenting // is REAPER's to do; the previous Win32 path passed GetMainHwnd() explicitly. - return runPicker(0, "Export bank package", suggestedPath.c_str(), outAbsPath); + if (!runPicker(0, "Export bank package", suggestedPath.c_str(), outAbsPath)) { + return false; + } + // GetUserFileName has no lpstrDefExt equivalent (the old Win32 picker's + // ofn.lpstrDefExt = L"rsbank"); whether mode 0 appends one itself from + // kExtList is [verify — DAW], so append it ourselves whenever it's missing. + if (!hasCaseInsensitiveSuffix(outAbsPath, ".rsbank")) outAbsPath += ".rsbank"; + return true; } } // namespace reasampler diff --git a/src/shell/package/package_pickers.h b/src/shell/package/package_pickers.h index 2cd8cf7..9f60512 100644 --- a/src/shell/package/package_pickers.h +++ b/src/shell/package/package_pickers.h @@ -1,7 +1,10 @@ // shell/package/package_pickers — the two package file pickers, both on REAPER's own // GetUserFileName (mode 1 = existing file, mode 0 = new file). No native/platform // split: the SDK's save mode is not optional on any build that can load this -// extension. Paths in and out are UTF-8, per the REAPER API contract. +// extension. Paths in and out are UTF-8, matching this tree's established practice +// for narrow strings crossing the REAPER API (see instrument_drop_win.cpp's +// path.u8string() to TrackFX_SetPreset, or prune_fs.cpp's CP_UTF8 conversion) — the +// SDK header itself never says "UTF-8". #pragma once diff --git a/src/shell/package/package_rollback.cpp b/src/shell/package/package_rollback.cpp index 1219292..c3162d0 100644 --- a/src/shell/package/package_rollback.cpp +++ b/src/shell/package/package_rollback.cpp @@ -24,7 +24,7 @@ bool LandedFileJournal::writeLandedFile(const std::string& destPath, std::error_code ec; const fs::path resolved = fs::absolute(utf8Path(destPath), ec); if (ec) return false; - const std::string absPath = resolved.u8string(); + const std::string absPath = pathToUtf8(resolved); if (!writeFileExclusive(absPath, payload)) return false; paths_.push_back(absPath); diff --git a/src/shell/package/package_rollback.h b/src/shell/package/package_rollback.h index cfc663d..438f168 100644 --- a/src/shell/package/package_rollback.h +++ b/src/shell/package/package_rollback.h @@ -19,6 +19,10 @@ struct RollbackResult { bool refused = false; // markIndexCommitted() ran: nothing was deleted }; +// Destroying an armed (uncommitted, un-rolled-back) journal is NOT an implicit +// rollback — the caller must call rollback() itself on the failure path it wants +// to undo. That's the fail-safe direction: a journal dropped by an unrelated early +// return leaves the landed files in place rather than silently deleting them. class LandedFileJournal { public: // Lands one payload at destPath through the exclusive create (which refuses an @@ -33,7 +37,9 @@ public: // Disarms the journal: the index mutation these files back is committed, so they // are now referenced bytes and the carve-out no longer covers them. This is the // half of prune's discriminator the journal cannot make structural on its own — - // the import verb MUST call it at the moment the index is committed. + // the import verb MUST call this only AFTER the index write has returned success. + // Calling it before, then having that write fail, strands the landed files with + // no index entry and a journal that now refuses to roll them back. void markIndexCommitted() { indexCommitted_ = true; } bool indexCommitted() const { return indexCommitted_; } diff --git a/tests/test_package_io.cpp b/tests/test_package_io.cpp index a533c72..d6237f6 100644 --- a/tests/test_package_io.cpp +++ b/tests/test_package_io.cpp @@ -293,11 +293,22 @@ static void testWriteFileExclusiveRefusesAnOccupiedPath() { CHECK(writeFileExclusive(path, PayloadBuffer(mine))); CHECK(readAll(path) == mine); // The create IS the check: a second call cannot replace the first file's bytes. + // (this pins the refusal, not O_EXCL's atomicity — a genuine race isn't portable) CHECK(!writeFileExclusive(path, PayloadBuffer(patternBytes(9, 8)))); CHECK(readAll(path) == mine); CHECK(!writeFileExclusive("pkg_io_excl_empty.bin", PayloadBuffer{})); CHECK(!exists("pkg_io_excl_empty.bin")); removeQuietly(path); + + // writeFileExclusive is the one call site using the wstring()/c_str() Windows + // open form rather than the fstream(fs::path) overload every other test here + // exercises — the only conversion whose reversion this file would otherwise miss. + const std::string cafePath = "pkg_io_excl_caf\xC3\xA9.bin"; + const std::vector cafeBytes = patternBytes(6, 4); + CHECK(writeFileExclusive(cafePath, PayloadBuffer(cafeBytes))); + CHECK(readAll(cafePath) == cafeBytes); + CHECK(!writeFileExclusive(cafePath, PayloadBuffer(patternBytes(3, 9)))); + removeQuietly(cafePath); } static void testListFolderFileNames() { diff --git a/tests/test_package_rollback.cpp b/tests/test_package_rollback.cpp index 8ddeb32..6f68bfc 100644 --- a/tests/test_package_rollback.cpp +++ b/tests/test_package_rollback.cpp @@ -22,7 +22,7 @@ static int g_fail = 0; // The journal records absolute paths, so every expectation is built the same way. static std::string scratch(const std::string& name) { - return (fs::current_path() / utf8Path(name)).u8string(); + return pathToUtf8(fs::current_path() / utf8Path(name)); } static std::vector patternBytes(std::size_t n, std::uint8_t seed) { @@ -64,6 +64,18 @@ static void testLandRecordsOnSuccessOnly() { CHECK(!exists(path)); // the recorded path denoted the file we asked for } +static void testLandNonAsciiPathRoundTripsAsUtf8() { + // The fs::absolute -> u8string round trip at writeLandedFile is otherwise + // untested with a non-ASCII path. + LandedFileJournal journal; + const std::string path = scratch("rb_caf\xC3\xA9.bin"); + const std::vector bytes = patternBytes(16, 2); + CHECK(journal.writeLandedFile(path, PayloadBuffer(bytes))); + CHECK(readAll(path) == bytes); + journal.rollback(); + CHECK(!exists(path)); +} + static void testRelativeInputIsRecordedAbsolute() { // The hazard: a bare name recorded verbatim, then a CWD change, and rollback // unlinks whatever now sits at that name in the new directory. @@ -169,6 +181,7 @@ static void testIndexCommitDisarmsRollback() { int main() { testLandRecordsOnSuccessOnly(); + testLandNonAsciiPathRoundTripsAsUtf8(); testRelativeInputIsRecordedAbsolute(); testExistingDestinationRefusedUntouched(); testEmptyPayloadRefused();