From 373948c18ad71d1efe157adac1039af2ad79624c Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 26 Jul 2026 21:56:09 -0400 Subject: [PATCH] fix(ingest): convert-on-import to 32f WAV, undo-group bank+assign, overflow guards Non-WAV sources decode via PCM_source::GetSamples and land as canonical 32f RIFF/WAVE; hash taken post-conversion so re-imports dedup. Undo block covers bank mutation + assign_request atomically. Overflow guards + adversarial tests. --- src/assignment_request.cpp | 37 +++- src/assignment_request.h | 9 + src/ingest.cpp | 349 ++++++++++++++++++++++++------ src/main.cpp | 21 +- tests/test_assignment_request.cpp | 40 ++++ 5 files changed, 378 insertions(+), 78 deletions(-) diff --git a/src/assignment_request.cpp b/src/assignment_request.cpp index a880e0c..077aeee 100644 --- a/src/assignment_request.cpp +++ b/src/assignment_request.cpp @@ -3,6 +3,7 @@ #include "assignment_request.h" #include +#include namespace reasampler { @@ -29,27 +30,44 @@ public: bool atEnd() const { return pos_ >= s_.size(); } // Reads one length-prefixed field into `out`. Fails on a missing ':', an empty or - // non-numeric length, or a length that runs past the end. + // non-numeric length, a length that overflows SIZE_MAX, or a length that runs past + // the end. The digit count is capped at 20 (the decimal width of SIZE_MAX on a + // 64-bit host) so a crafted 200-digit length cannot accumulate past SIZE_MAX via + // repeated multiply. "never UB" promise from the header is upheld here. bool field(std::string& out) { if (!ok_) return false; const std::size_t colon = s_.find(':', pos_); if (colon == std::string::npos) return fail(); if (colon == pos_) return fail(); // empty length token + // Cap: SIZE_MAX fits in at most 20 decimal digits; a longer run is bogus. + if (colon - pos_ > 20u) return fail(); std::size_t len = 0; for (std::size_t i = pos_; i < colon; ++i) { const char c = s_[i]; if (c < '0' || c > '9') return fail(); - len = len * 10 + static_cast(c - '0'); + const std::size_t digit = static_cast(c - '0'); + // Overflow guard: if len would exceed SIZE_MAX after multiply+add, fail. + if (len > (std::numeric_limits::max() - digit) / 10u) + return fail(); + len = len * 10u + digit; } const std::size_t start = colon + 1; - if (start + len > s_.size()) return fail(); + // Guard: start may equal s_.size() (empty remainder), in which case only len==0 + // is valid; start > s_.size() cannot happen (colon < s_.size() by find()). + // Use subtraction-first form to avoid start+len wrapping on a huge len. + if (start > s_.size() || len > s_.size() - start) return fail(); out.assign(s_, start, len); pos_ = start + len; return true; } // Reads a length-prefixed field and parses it as a signed 64-bit decimal (an - // optional leading '-'). Fails on empty, non-digit, or trailing bytes. + // optional leading '-'). Fails on empty, non-digit, trailing bytes, or a value + // that would overflow INT64_MAX / underflow INT64_MIN. The digit count is capped + // at 19 (the decimal width of INT64_MAX, plus 1 for the optional sign = 20 + // characters maximum) so a crafted 21-digit field cannot accumulate UB. "never UB" + // promise from the header is upheld: all arithmetic is done on positive digits + // and capped before applying the sign. bool fieldInt64(std::int64_t& out) { std::string f; if (!field(f)) return false; @@ -61,11 +79,20 @@ public: i = 1; if (f.size() == 1) return fail(); // bare "-" } + // Cap at 19 digits (INT64_MAX = 9223372036854775807 — 19 digits). A 20-digit + // positive value would overflow INT64_MAX; a 20-digit negative might be valid + // (INT64_MIN = -9223372036854775808) but we conservatively reject it too: the + // generation field is a unix timestamp, never near INT64 limits in practice. + if (f.size() - i > 19u) return fail(); std::int64_t v = 0; for (; i < f.size(); ++i) { const char c = f[i]; if (c < '0' || c > '9') return fail(); - v = v * 10 + static_cast(c - '0'); + const std::int64_t digit = static_cast(c - '0'); + // Overflow guard: v * 10 + digit must not exceed INT64_MAX. + if (v > (std::numeric_limits::max() - digit) / 10) + return fail(); + v = v * 10 + digit; } out = neg ? -v : v; return true; diff --git a/src/assignment_request.h b/src/assignment_request.h index 4992242..46c1e03 100644 --- a/src/assignment_request.h +++ b/src/assignment_request.h @@ -75,6 +75,15 @@ std::string encodeAssignmentRequest(const AssignmentRequest& req); // malformed / truncated / trailing-garbage input (never UB, never a partial value) — // the reader shell treats absence/malformed as "no pending request." Round-trips: // decodeAssignmentRequest(encodeAssignmentRequest(x)) == x. +// +// READER REQUIREMENT (instrument-side, S8 follow-up dispatch): after successfully +// decoding a request, the reader MUST verify that (bankId, sampleId) resolves to an +// existing sample before acting on it. An undo on the extension side rolls back the +// `banks` ext-state key (removing the sample) but cannot atomically clear the +// `assign_request` key if the write happened outside the undo block. Even with the +// undo-grouping fix (Major 2), the reader must guard against this: treat an +// unresolvable (bankId, sampleId) pair as a stale/no-op request and discard it +// silently, never crashing or selecting a nonexistent entry. std::optional decodeAssignmentRequest(const std::string& wire); } // namespace reasampler diff --git a/src/ingest.cpp b/src/ingest.cpp index f411bc6..6211bb3 100644 --- a/src/ingest.cpp +++ b/src/ingest.cpp @@ -8,6 +8,7 @@ #include "ingest.h" #include +#include #include #include #include @@ -23,6 +24,8 @@ #include "capture_paths.h" // deriveBankPaths / projectDirOfRpp / hashWavContent #include "persist.h" // ReaSamplerSession +#include "wav_trim.h" // parseWavLayout — 32f-float WAV validator for the fast path + #include "reaper_plugin.h" // reaper_plugin_info_t, gaccel_register_t (full defs) #define REAPERAPI_MINIMAL @@ -34,6 +37,8 @@ #define REAPERAPI_WANT_GetMediaSourceNumChannels #define REAPERAPI_WANT_GetMediaSourceSampleRate #define REAPERAPI_WANT_GetMediaSourceLength +#define REAPERAPI_WANT_Undo_BeginBlock2 +#define REAPERAPI_WANT_Undo_EndBlock2 #include "reaper_plugin_functions.h" namespace reasampler { @@ -54,10 +59,10 @@ constexpr const char* kIdImportMediaExplorer = "INGEST_IMPORT_MEDIA_EXPLORER"; int g_cmdImportMediaExplorer = 0; gaccel_register_t g_accelImportMediaExplorer{}; -// Durable store of the composed, channel-qualified command-id + label strings. A vector of -// two entries suffices here (one action); pointers into it are handed to REAPER at register -// and re-presented at unregister, so it must not be resized after registration. Reserved up -// front so the register-time c_str() stays valid for the unload path. +// Durable store of the composed, channel-qualified command-id + label strings. Two scalar +// std::string globals (one action); their c_str() pointers are handed to REAPER at register +// and re-presented at unregister, so these strings must not be mutated after registration. +// Populated once by ingestRegisterActions; stable for the extension lifetime. std::string g_idImportStr; std::string g_labelImportStr; @@ -74,8 +79,7 @@ std::string currentProjectDir() { } // Reads a whole file's bytes. Empty vector on any failure (missing / unreadable). Mirror -// of capture.cpp's readFileBytes — used to hash an import source for dedup + a copied file -// for the Sample's content hash. +// of capture.cpp's readFileBytes — used to read the source and validate/hash the bank copy. std::vector readFileBytes(const std::string& path) { std::ifstream f(path, std::ios::binary | std::ios::ate); if (!f) return {}; @@ -88,27 +92,155 @@ std::vector readFileBytes(const std::string& path) { return bytes; } +// Writes a byte buffer to a file. Returns true on success. The caller is responsible for +// ensuring the directory exists before calling. +bool writeFileBytes(const std::string& path, const std::vector& bytes) { + std::ofstream f(path, std::ios::binary | std::ios::trunc); + if (!f) return false; + f.write(reinterpret_cast(bytes.data()), + static_cast(bytes.size())); + return f.good(); +} + +// Builds a minimal 32-bit-float RIFF/WAVE byte buffer from interleaved double samples. +// The output is a canonical WAV the bank and wav_trim can read: +// RIFF chunk, WAVE form, fmt chunk (tag 3 = WAVE_FORMAT_IEEE_FLOAT, 16-byte body), +// data chunk (interleaved little-endian float32, one float per sample per channel). +// `nch` channels, `rate` Hz sample rate, `frameCount` frames (total samples = frameCount*nch). +// Each ReaSample (double) is narrowed to float by assignment — the instrument expects +// 32-bit float; the reduction is intentional and matches how the bank contract is defined +// (capture.cpp kRenderFormatWavFloat32; wav_trim.h FORMAT ASSUMPTION). +std::vector buildFloat32Wav(int nch, std::uint32_t rate, + std::size_t frameCount, + const std::vector& interleaved) { + const std::size_t sampleCount = frameCount * static_cast(nch); + const std::size_t dataBytesCount = sampleCount * 4u; // 4 bytes per float32 + + // The WAV is: RIFF(4)+size(4)+WAVE(4) = 12, fmt (4)+size(4)+16 body = 24, data (4)+size(4)+payload. + // Total = 12 + 24 + 8 + dataBytesCount = 44 + dataBytesCount. + const std::uint32_t riffSize = + static_cast(36u + dataBytesCount); // 4("WAVE")+24(fmt chunk)+8(data hdr)+data + + std::vector out; + out.reserve(44u + dataBytesCount); + + auto putU16 = [&](std::uint16_t v) { + out.push_back(static_cast(v & 0xFF)); + out.push_back(static_cast((v >> 8) & 0xFF)); + }; + auto putU32 = [&](std::uint32_t v) { + out.push_back(static_cast(v & 0xFF)); + out.push_back(static_cast((v >> 8) & 0xFF)); + out.push_back(static_cast((v >> 16) & 0xFF)); + out.push_back(static_cast((v >> 24) & 0xFF)); + }; + auto putTag = [&](const char* t) { + for (int i = 0; i < 4; ++i) + out.push_back(static_cast(t[i])); + }; + auto putF32 = [&](float f) { + std::uint8_t tmp[4]; + std::memcpy(tmp, &f, 4); + for (int i = 0; i < 4; ++i) out.push_back(tmp[i]); + }; + + // RIFF header + putTag("RIFF"); + putU32(riffSize); + putTag("WAVE"); + + // fmt chunk (16-byte body, WAVE_FORMAT_IEEE_FLOAT = 0x0003) + putTag("fmt "); + putU32(16u); // chunk body size + putU16(0x0003u); // WAVE_FORMAT_IEEE_FLOAT + putU16(static_cast(nch)); + putU32(rate); + putU32(rate * static_cast(nch) * 4u); // avgBytesPerSec + putU16(static_cast(nch * 4)); // blockAlign + putU16(32u); // bitsPerSample + + // data chunk + putTag("data"); + putU32(static_cast(dataBytesCount)); + for (std::size_t i = 0; i < sampleCount && i < interleaved.size(); ++i) + putF32(static_cast(interleaved[i])); + + return out; +} + +// Decodes ALL samples from `src` into interleaved double-precision frames. +// Returns empty on a zero-length or silent source (sampleRate < 1, channelCount == 0). +// Uses GetSamples in blocks; advances time_s monotonically. The caller has already +// queried channelCount and sampleRate from the same source; those values are passed in +// to avoid re-querying after GetSamples mutates decoder state. +std::vector decodePcmSource(PCM_source* src, int nch, double sampleRate, + double lengthSeconds) { + if (!src || nch <= 0 || sampleRate < 1.0 || lengthSeconds <= 0.0) return {}; + + const std::size_t totalFrames = + static_cast(lengthSeconds * sampleRate + 0.5); + if (totalFrames == 0) return {}; + + std::vector out; + out.reserve(totalFrames * static_cast(nch)); + + // Pull samples in blocks of ~4096 frames; loop until source is exhausted. + constexpr int kBlockFrames = 4096; + std::vector block(static_cast(kBlockFrames * nch)); + + PCM_source_transfer_t t{}; + t.samplerate = sampleRate; + t.nch = nch; + t.time_s = 0.0; + t.midi_events = nullptr; + + while (true) { + t.samples = block.data(); + t.length = kBlockFrames; + t.samples_out = 0; + src->GetSamples(&t); + if (t.samples_out <= 0) break; + const std::size_t got = static_cast(t.samples_out) * + static_cast(nch); + out.insert(out.end(), block.data(), block.data() + got); + t.time_s += static_cast(t.samples_out) / sampleRate; + } + return out; +} + // The result of an import-into-bank: the sample id to assign (the existing id on a // hash-dedup collapse, the new id otherwise) and whether anything was added to the index // (so the caller opens an undo point only for a real mutation). struct ImportResult { - std::string sampleId; // "" on failure (nothing to assign) - bool added = false; // true iff a NEW index entry was created (not a collapse) - std::string message; // human-readable outcome for the console + std::string sampleId; // "" on failure (nothing to assign) + bool added = false; // true iff a NEW index entry was created (not a collapse) + std::string message; // human-readable outcome for the console }; -// Imports one OS-native source file into the ACTIVE bank: copy into the project-relative -// bank folder + index add, hash-dedup applied. Mirrors capture's landing semantics -// (relative-paths-only, content-hash dedup) for a file the user brought in rather than -// captured. Populates the Sample's metadata by probing the file via PCM_Source. +// Imports one OS-native source file into the ACTIVE bank: convert-if-needed to 32-bit- +// float WAV, write to the project-relative bank folder, index-add, hash-dedup applied. // -// DEDUP-BEFORE-COPY: hash the SOURCE first and consult the active bank; if the content is -// already held, assign the existing sample's id and DO NOT copy a redundant file to disk. -// Only a genuinely new file is copied + added. This is stricter than capture (which renders -// then adds, leaving a collapsed render orphaned) because an import can cheaply check first. +// BANK CONTRACT: the instrument (wav_trim) expects every bank file to be a canonical +// 32-bit-float WAV (WAVE_FORMAT_IEEE_FLOAT, 32 bits). A verbatim copy of a non-WAV (or +// an integer-PCM or double-float WAV) would be unplayable. This function therefore: +// 1. Checks whether the source IS already a valid 32f WAV (parseWavLayout fast path). +// 2. If yes: copies it verbatim — one I/O, content unchanged. +// 3. If no: decodes via PCM_source::GetSamples and writes a fresh 32f WAV, preserving +// the source's channel count and sample rate. // -// NON-DESTRUCTIVE: the source file is never modified or moved — only read + copied. -// Records the copied file in the owned-file manifest (Phase B B-cap) so Phase R prune can +// DEDUP ORDERING: the content hash is taken from the CONVERTED (bank-format) bytes AFTER +// building the file buffer but BEFORE writing to disk. This means: +// * Re-importing the same source file yields the same converted bytes → same hash → +// dedup fires → no redundant disk write (matching the DEDUP-BEFORE-DISK design). +// * An imported WAV whose audio-content hash matches a captured WAV also deduplicates +// correctly (hashWavContent is chunk-aware for both). +// * The pre-conversion hash shortcut (hash the raw source bytes) is not used: a non-WAV +// source's bytes would produce a different hash from the converted WAV bytes, so two +// imports of the same mp3 would NOT dedup — which is wrong. Hashing post-conversion +// is correct. +// +// NON-DESTRUCTIVE: the source file is never modified or moved — only read. +// Records the written file in the owned-file manifest (Phase B B-cap) so Phase R prune can // attribute it. Does NOT persist or open an undo point — the caller batches that (a // multi-file drop is one undo point, one persist). ImportResult importFileIntoActiveBank(const std::string& absoluteSourcePath) { @@ -132,23 +264,83 @@ ImportResult importFileIntoActiveBank(const std::string& absoluteSourcePath) { return out; } - // Hash the SOURCE content up front (WAV-aware, same hasher captures use so an imported - // WAV dedups against a captured one byte-for-byte on audio content). An unreadable - // source yields an empty hash — treated as "not dedupable" (the safe direction: it - // will be copied + added rather than silently collapsed onto an unrelated entry). + // Read source bytes; needed to check whether it is already a 32f WAV. const std::vector srcBytes = readFileBytes(absoluteSourcePath); if (srcBytes.empty()) { out.message = "file is empty or unreadable: " + absoluteSourcePath; return out; } - const std::string contentHash = hashWavContent(srcBytes); + + // Probe the source's audio geometry via PCM_source. Needed for conversion AND for + // populating the Sample's metadata. A file REAPER cannot open leaves geometry at + // zero — the sample still imports if the WAV-fast-path succeeds; the geometry + // is simply unknown, the honest default. + int channelCount = 0; + int sampleRate = 0; + double lengthSeconds = 0.0; + PCM_source* srcHandle = PCM_Source_CreateFromFile(absoluteSourcePath.c_str()); + if (srcHandle) { + channelCount = GetMediaSourceNumChannels(srcHandle); + sampleRate = GetMediaSourceSampleRate(srcHandle); + bool isQN = false; + lengthSeconds = GetMediaSourceLength(srcHandle, &isQN); + if (isQN) lengthSeconds = 0.0; // QN-length source has no seconds length to store + } + + // Determine whether a verbatim copy suffices (fast path) or a conversion is needed. + // parseWavLayout validates that the source is a canonical 32-bit-float RIFF/WAVE; any + // other format (mp3, aiff, integer PCM, 16-bit WAV, etc.) takes the decode+rewrite path. + const WavLayout layout = parseWavLayout(srcBytes); + const bool isFloat32Wav = layout.valid; + + // Build the bank-format bytes in memory (the "converted" bytes), which we hash for dedup + // BEFORE writing to disk so a re-import of the same source skips the disk write. + std::vector bankBytes; + if (isFloat32Wav) { + // Fast path: already canonical — bank bytes ARE the source bytes. + bankBytes = srcBytes; + if (srcHandle) PCM_Source_Destroy(srcHandle); + } else { + // Conversion path: decode all samples then write a fresh 32f WAV. + // PCM_source is opened on the source path (not a copy); we already have srcHandle. + std::vector decoded; + if (srcHandle && channelCount > 0 && sampleRate > 0 && lengthSeconds > 0.0) { + decoded = decodePcmSource(srcHandle, channelCount, + static_cast(sampleRate), lengthSeconds); + } + if (srcHandle) PCM_Source_Destroy(srcHandle); + + if (decoded.empty()) { + // No decodable audio. The source is on disk (valid path, REAPER could open it) + // but yielded no samples — e.g. a MIDI file, a zero-length audio file, or a + // format REAPER does not support. Fail loudly: we must not write a silent WAV + // and pretend the import succeeded. + out.message = "could not decode audio samples from: " + + fs::path(absoluteSourcePath).filename().string() + + " (unsupported format or no audio data)"; + return out; + } + + const std::size_t frameCount = + decoded.size() / static_cast(channelCount > 0 ? channelCount : 1); + bankBytes = buildFloat32Wav(channelCount, + static_cast(sampleRate), + frameCount, decoded); + } + // srcHandle is destroyed above in both branches. + + // Hash the converted (bank-format) bytes for dedup. WAV-aware hash (hashWavContent) + // so a re-import of the same source deduplicates against a previously-captured or + // previously-imported sample with identical audio content, even if non-audio RIFF + // chunks differ. Empty hash (unhashable) is treated as "not dedupable" (safe direction: + // copies + adds rather than silently collapsing onto an unrelated entry). + const std::string contentHash = hashWavContent(bankBytes); BankBook& book = g_session->book(); - const std::string activeBankId = book.activeBankId(); - // Dedup-before-copy: if the active bank already holds this content, assign the existing - // id and skip the copy (no redundant on-disk duplicate). Empty hashes never match - // (findByHash treats "" as non-participating), so an unhashable file always imports fresh. + // Dedup-before-disk: if the active bank already holds this audio content, assign the + // existing sample's id and skip the disk write (no redundant on-disk duplicate). + // Empty hashes never match (findByHash treats "" as non-participating). if (!contentHash.empty()) { if (const Sample* existing = book.activeIndex().findByHash(contentHash)) { out.sampleId = existing->id; @@ -158,46 +350,25 @@ ImportResult importFileIntoActiveBank(const std::string& absoluteSourcePath) { } } - // Derive the destination path (project-relative index value + absolute copy target). The - // stem comes from the source file name; a timestamp uniqueTag avoids collision with a - // prior import of a same-named file. (fs::path::stem strips the extension and any dir.) + // Derive the destination path. The stem comes from the source file name; a timestamp + // uniqueTag avoids collision with a prior import of a same-named file. const std::string sourceStem = fs::path(absoluteSourcePath).stem().string(); const std::int64_t nowSec = static_cast(std::time(nullptr)); const std::string uniqueTag = std::to_string(nowSec); const BankPaths paths = deriveBankPaths(projectDir, sourceStem, uniqueTag); - // Ensure the bank folder exists, then copy the source in (never move — the source is the - // user's file and must be left intact; non-destructive). copy_file with overwrite off: - // the uniqueTag makes a collision astronomically unlikely, and if one occurs we fail - // loudly rather than clobber. - fs::create_directories(paths.absoluteDir, ec); // idempotent; ec ignored (copy reports) + // Ensure the bank folder exists, then write the (converted) bank bytes. + fs::create_directories(paths.absoluteDir, ec); // idempotent; ec ignored (write reports) const std::string destPath = paths.absoluteDir + "/" + paths.fileName; - fs::copy_file(absoluteSourcePath, destPath, fs::copy_options::none, ec); - if (ec) { - out.message = "could not copy into the bank folder (" + ec.message() + ")"; + if (!writeFileBytes(destPath, bankBytes)) { + out.message = "could not write converted file to the bank folder"; return out; } - // Probe the copied file's audio geometry via PCM_Source (channels / rate / length) so - // the Sample carries real metadata the panel + instrument read. A file REAPER cannot - // open leaves these at their zero-values — the sample still imports (the file is on - // disk, the hash is set); the geometry is simply unknown, the honest default. - int channelCount = 0; - int sampleRate = 0; - double lengthSeconds = 0.0; - if (PCM_source* src = PCM_Source_CreateFromFile(destPath.c_str())) { - channelCount = GetMediaSourceNumChannels(src); - sampleRate = GetMediaSourceSampleRate(src); - bool isQN = false; - lengthSeconds = GetMediaSourceLength(src, &isQN); - if (isQN) lengthSeconds = 0.0; // a QN-length source has no seconds length to store - PCM_Source_Destroy(src); - } - // Build the Sample. Import is NOT a capture — sourceMode/range/tail do not apply; we // record what we know (path, hash, geometry, name) and leave capture-only fields at - // their defaults. rootNote/loop stay empty (D-B): an imported file is not a single - // played note, so we do not guess a root note. + // their defaults. rootNote/loop stay empty: an imported file is not a single played + // note, so we do not guess a root note. Sample s; s.id = "imp-" + uniqueTag + "-" + paths.fileName; s.displayName = sourceStem.empty() ? std::string("import") : sourceStem; @@ -210,21 +381,22 @@ ImportResult importFileIntoActiveBank(const std::string& absoluteSourcePath) { s.createdTimestamp = nowSec; const AddResult r = book.activeIndex().add(s); - // Record the copied file as owned regardless of the add outcome — the tool WROTE it, so + // Record the written file as owned regardless of the add outcome — the tool WROTE it, so // Phase R prune must attribute it. (A Collapsed result here would mean another sample in - // the active bank matched the hash; we pre-checked findByHash above, so in practice - // Added is the outcome. But record + handle both honestly.) + // the active bank matched the hash after we passed the pre-write dedup check — a narrow + // race window. Record + handle both honestly.) g_session->owned().add(paths.relativePath); switch (r) { case AddResult::Added: out.sampleId = s.id; out.added = true; - out.message = "imported -> " + paths.relativePath; + out.message = (isFloat32Wav ? "imported -> " : "converted + imported -> ") + + paths.relativePath; break; case AddResult::Collapsed: { - // The hash matched an existing entry (a race against our pre-check, or an empty- - // hash edge). Assign the existing entry's id, not the rejected new one. + // The hash matched an existing entry (a race against our pre-write dedup check, + // or an empty-hash edge). Assign the existing entry's id. const Sample* existing = contentHash.empty() ? nullptr : book.activeIndex().findByHash(contentHash); out.sampleId = existing ? existing->id : std::string{}; @@ -278,10 +450,27 @@ void doImportFromMediaExplorer() { return; } - // Persist the bank add as one undo point ONLY when the index actually mutated (a - // dedup collapse changed nothing on the index). Then assign + refresh the panel. - if (r.added) persistBankOp("ReaSampler: import from Media Explorer"); - ingestAssignActiveInstance(g_session->book().activeBankId(), r.sampleId); + // Persist the bank add AND the assign request inside ONE undo block so Ctrl-Z rolls + // back both keys atomically: undo restores `banks` (removing the new sample) AND + // clears the `assign_request` that named it, so no stale request can survive. + // The block is opened only when the index mutated (a dedup collapse changed nothing). + // If saveToActiveProject() no-ops (unsaved project), we close with an empty label + + // zero flag so REAPER discards the undo entry (the house pattern from actions.cpp). + if (r.added) { + Undo_BeginBlock2(nullptr); + const bool persisted = g_session->saveToActiveProject(); + // Assign request inside the same block: undo rolls back both keys together. + ingestAssignActiveInstance(g_session->book().activeBankId(), r.sampleId); + if (persisted) + Undo_EndBlock2(nullptr, "ReaSampler: import from Media Explorer", + UNDO_STATE_MISCCFG); + else + Undo_EndBlock2(nullptr, "", 0); + } else { + // Dedup collapse: index unchanged, no undo point. Assign request still written + // (the user explicitly re-imported; they want the instance updated). + ingestAssignActiveInstance(g_session->book().activeBankId(), r.sampleId); + } bankPanelRefresh(); ShowConsoleMsg(("ReaSampler ingest: " + r.message + " (assigned to the active " "instance).\n").c_str()); @@ -333,12 +522,28 @@ void ingestDroppedFiles(const std::vector& absolutePaths) { } // One undo point for the whole drop, opened only if a NEW index entry was created (a - // drop that only re-hit existing content mutated nothing on the index). persistBankOp - // itself no-ops the block on an unsaved project. - if (importedNew > 0) persistBankOp("ReaSampler: import dropped file(s)"); - + // drop that only re-hit existing content mutated nothing on the index). The assign + // request is written INSIDE the same block so Ctrl-Z rolls back both keys together: + // undo restores `banks` (removing the new samples) AND clears the `assign_request` that + // named one of them, so no stale request survives pointing to a removed sample. + // If saveToActiveProject() no-ops (unsaved project), we close with an empty label + zero + // flag so REAPER discards the undo entry (house pattern from actions.cpp). if (!firstAssignId.empty()) { - ingestAssignActiveInstance(firstAssignBank, firstAssignId); + if (importedNew > 0) { + Undo_BeginBlock2(nullptr); + const bool persisted = g_session->saveToActiveProject(); + // Assign inside the block: undo restores both keys atomically. + ingestAssignActiveInstance(firstAssignBank, firstAssignId); + if (persisted) + Undo_EndBlock2(nullptr, "ReaSampler: import dropped file(s)", + UNDO_STATE_MISCCFG); + else + Undo_EndBlock2(nullptr, "", 0); + } else { + // All dropped files deduplicated: index unchanged, no undo point needed. Still + // assign so the user sees the sample is already in the bank. + ingestAssignActiveInstance(firstAssignBank, firstAssignId); + } bankPanelRefresh(); const std::string msg = "ReaSampler ingest: imported " + std::to_string(importedTotal) + diff --git a/src/main.cpp b/src/main.cpp index 07c9d26..eeecdb3 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -867,15 +867,34 @@ static std::string RunCapture(const reasampler::CaptureActionDef& def) // inserts a timeline item (load-bearing principle); the only addition here is the // bank-index-id -> assignment-request write after the sample lands. If the capture // failed / no-op'd (empty id), no assignment is written (nothing to assign). +// +// UNDO GROUPING: both the bank mutation (RunCapture -> saveToActiveProject) AND the +// assignment-request write (ingestAssignActiveInstance -> writeAssignmentRequest) are +// wrapped in a single undo block so Ctrl-Z rolls back both ext-state keys atomically. +// An undo that removes the captured sample also clears the assign_request that named it, +// preventing a stale request from pointing at a removed sample. The block uses the house +// pattern (UNDO_STATE_MISCCFG, discarded on an unsaved project with empty label + zero +// flag) matching the bank-op family in actions.cpp. static void RunCaptureItemAssign() { // Reuse the Item-scope def from the capture table (index 0) — same range logic, same // FX-scope neutralize, same bank/persist landing as the plain "capture item" action. + Undo_BeginBlock2(nullptr); + const std::string sampleId = RunCapture(reasampler::captureActionTable()[0]); - if (sampleId.empty()) return; // capture failed / no-op — RunCapture already reported + if (sampleId.empty()) + { + // Capture failed or no-op'd — RunCapture already reported. Discard the empty point. + Undo_EndBlock2(nullptr, "", 0); + return; + } + // Assign inside the same block so undo clears both keys together. reasampler::ingestAssignActiveInstance(g_session.book().activeBankId(), sampleId); + Undo_EndBlock2(nullptr, "ReaSampler: capture + assign to active instance", + UNDO_STATE_MISCCFG); + reasampler::bankPanelRefresh(); ShowConsoleMsg("ReaSampler ingest: captured into the bank and assigned to the active " "instance.\n"); diff --git a/tests/test_assignment_request.cpp b/tests/test_assignment_request.cpp index 33b2982..9dfe12c 100644 --- a/tests/test_assignment_request.cpp +++ b/tests/test_assignment_request.cpp @@ -131,6 +131,43 @@ static void testTrailingGarbageRejected() { CHECK(!decodeAssignmentRequest(wire + "0:").has_value()); } +// --- overflow / adversarial integer inputs ------------------------------------ + +// A 21-digit length field overflows SIZE_MAX and must be rejected safely (no UB, +// no wrap-around that could make a huge length appear small and pass the bounds check). +static void testOverflowFieldLength() { + // Craft a wire where the bankId length token is 21 digits that exceed SIZE_MAX. + // The decoder must fail cleanly, not access memory out of bounds. + // "rsassign1" + "999999999999999999999:" (21 nines) + junk: rejects before OOB. + const std::string wire = std::string("rsassign1") + "999999999999999999999:junk"; + CHECK(!decodeAssignmentRequest(wire).has_value()); +} + +// A 20-digit generation (exceeds the 19-digit cap) must be rejected safely. +static void testOverflowFieldInt64() { + // Encode a valid record then manually substitute the generation with a 20-digit value. + // We cannot use encode (it would produce a correct 19-digit generation), so we + // build the wire manually. Generation "99999999999999999999" (20 nines) exceeds cap. + // bankId = "pool" (4 bytes), sampleId = "s1" (2 bytes). + const std::string wire = std::string("rsassign1") + + "4:pool" + + "2:s1" + + "20:99999999999999999999"; + CHECK(!decodeAssignmentRequest(wire).has_value()); +} + +// SIZE_MAX as a length field (20 digits, within the digit-count cap) must not UB or +// wrap. The overflow-guard in field() caps the multiplication; even if the value itself +// does not trigger the multiply guard (SIZE_MAX accumulates cleanly digit by digit), +// the subsequent "len > s_.size() - start" bounds check catches it because the actual +// string is tiny — no OOB access, no wraparound, clean rejection. +static void testOverflowExactSizeMax() { + // 18446744073709551615 = SIZE_MAX on 64-bit. 20 digits: within the digit cap, but the + // trailing bounds check rejects it because the wire string is far smaller than SIZE_MAX. + const std::string wire = std::string("rsassign1") + "18446744073709551615:X"; + CHECK(!decodeAssignmentRequest(wire).has_value()); +} + int main() { testRoundTrip(); testRoundTripPoolAndZeroGeneration(); @@ -139,6 +176,9 @@ int main() { testLargeGeneration(); testMalformedParse(); testTrailingGarbageRejected(); + testOverflowFieldLength(); + testOverflowFieldInt64(); + testOverflowExactSizeMax(); if (g_fail == 0) std::printf("All tests passed.\n"); return g_fail ? 1 : 0;