From 96c789333250ed0911f8d14c3216d1078a3519b2 Mon Sep 17 00:00:00 2001 From: Ryan Francesconi <2917795+ryanfrancesconi@users.noreply.github.com> Date: Sat, 31 Oct 2026 22:39:07 -0700 Subject: [PATCH] RIFF: Don't let a clamped chunk swallow chunks saved after it (#1482) Only the audio chunk (data, or SSND in AIFF) is clamped to the end of the file when it declares more than the file holds, and its size is rewritten before a chunk is appended after it. Any other chunk running past the end, such as a trailing APEv2 tag, ends the chunk walk again, so a save inserts new chunks ahead of it instead of losing them on the next read. --- taglib/riff/rifffile.cpp | 31 ++++++++++++++----- tests/test_wav.cpp | 65 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 88 insertions(+), 8 deletions(-) diff --git a/taglib/riff/rifffile.cpp b/taglib/riff/rifffile.cpp index 46ae35d9..fcb8a88c 100644 --- a/taglib/riff/rifffile.cpp +++ b/taglib/riff/rifffile.cpp @@ -46,6 +46,8 @@ struct Chunk //! May exceed 32 bits for the "data" chunk of an RF64/BW64 file. offset_t size; unsigned int padding; + //! The audio chunk, declared larger than the file and read to its end instead. + bool clamped { false }; }; class RIFF::File::FilePrivate @@ -243,10 +245,18 @@ void RIFF::File::appendChunk(const ByteVector &name, const ByteVector &data) return; } - // Adjust the padding of the last chunk to place the new chunk at even position. - Chunk &last = d->chunks.back(); + // A clamped chunk gets its real size before anything follows it, or the next read + // takes the new chunk for part of it. + if(last.clamped && !d->isLongForm) { + insert(ByteVector::fromUInt(static_cast(last.size), d->endianness == BigEndian), + last.offset - 4, 4); + last.clamped = false; + } + + // Adjust the padding of the last chunk to place the new chunk at even position. + offset_t offset = last.offset + last.size + last.padding; if(offset & 1) { if(last.padding == 1) { @@ -386,13 +396,19 @@ void RIFF::File::read() : static_cast(d->dataSize64); } + bool clamped = false; if(chunkSize > static_cast(available)) { - // Clamp to available bytes rather than rejecting the chunk outright. - // Some encoders write a correct data chunk but with a slightly too-large - // declared size, or place the data chunk outside the declared RIFF boundary. - // Lenient parsers (ffmpeg, QuickTime) handle this by clamping; we do the same. + // Some encoders write a correct audio chunk with a slightly too-large declared + // size, or place it outside the declared RIFF boundary. Lenient parsers (ffmpeg, + // QuickTime) clamp it to the available bytes; we do the same. Any other chunk + // running past the end is trailing data, such as an APE tag, and not a chunk. + if(chnkName != (bigEndian ? "SSND" : "data")) { + debug("RIFF::File::read() -- Chunk '" + chnkName + "' has invalid size (larger than the file size)"); + break; + } debug("RIFF::File::read() -- Chunk '" + chnkName + "' is truncated; clamping size to available bytes."); - chunkSize = length() - offset - 8; + chunkSize = available; + clamped = true; } Chunk chunk; @@ -400,6 +416,7 @@ void RIFF::File::read() chunk.size = static_cast(chunkSize); chunk.offset = offset + 8; chunk.padding = 0; + chunk.clamped = clamped; offset = chunk.offset + chunk.size; diff --git a/tests/test_wav.cpp b/tests/test_wav.cpp index 08365129..a587ccd4 100644 --- a/tests/test_wav.cpp +++ b/tests/test_wav.cpp @@ -29,6 +29,7 @@ #include #include "taglib_config.h" +#include "apetag.h" #include "tcharset.h" #include "id3v2tag.h" #include "infotag.h" @@ -356,6 +357,8 @@ class TestWAV : public CppUnit::TestFixture CPPUNIT_TEST(testFuzzedFile1); CPPUNIT_TEST(testFuzzedFile2); CPPUNIT_TEST(testFileWithGarbageAppended); + CPPUNIT_TEST(testTrailingAPETagIsNotAChunk); + CPPUNIT_TEST(testSaveAfterOversizedDataChunk); CPPUNIT_TEST(testStripAndProperties); CPPUNIT_TEST(testPCMWithFactChunk); CPPUNIT_TEST(testWaveFormatExtensible); @@ -1243,7 +1246,7 @@ public: { FileStream stream(copy.fileName().c_str()); stream.seek(0, IOStream::End); - constexpr char garbage[] = "\r2345678"; + constexpr char garbage[] = "12345678"; stream.writeBlock(ByteVector(garbage, sizeof(garbage) - 1)); stream.seek(0); contentsBeforeModification = stream.readBlock(stream.length()); @@ -1266,6 +1269,66 @@ public: } } + void testTrailingAPETagIsNotAChunk() + { + ScopedFileCopy copy("empty", ".wav"); + APE::Tag ape; + ape.setTitle("APE Title"); + const ByteVector apeData = ape.render(); + { + FileStream stream(copy.fileName().c_str()); + stream.seek(0, IOStream::End); + stream.writeBlock(apeData); + } + { + RIFF::WAV::File f(copy.fileName().c_str()); + CPPUNIT_ASSERT(f.isValid()); + f.ID3v2Tag()->setTitle("ID3v2 Title"); + f.InfoTag()->setTitle("INFO Title"); + CPPUNIT_ASSERT(f.save()); + } + { + RIFF::WAV::File f(copy.fileName().c_str()); + CPPUNIT_ASSERT(f.isValid()); + CPPUNIT_ASSERT_EQUAL(String("ID3v2 Title"), f.ID3v2Tag()->title()); + CPPUNIT_ASSERT_EQUAL(String("INFO Title"), f.InfoTag()->title()); + } + { + FileStream stream(copy.fileName().c_str()); + stream.seek(-static_cast(apeData.size()), IOStream::End); + CPPUNIT_ASSERT_EQUAL(apeData, stream.readBlock(apeData.size())); + } + } + + void testSaveAfterOversizedDataChunk() + { + ScopedFileCopy copy("empty", ".wav"); + { + // empty.wav ends with its "data" chunk; declare 100 bytes more than it holds. + FileStream stream(copy.fileName().c_str()); + stream.seek(40); + const unsigned int dataSize = stream.readBlock(4).toUInt(false); + stream.seek(40); + stream.writeBlock(ByteVector::fromUInt(dataSize + 100, false)); + } + { + RIFF::WAV::File f(copy.fileName().c_str()); + CPPUNIT_ASSERT(f.isValid()); + f.ID3v2Tag()->setTitle("ID3v2 Title"); + f.InfoTag()->setTitle("INFO Title"); + CPPUNIT_ASSERT(f.save()); + } + { + RIFF::WAV::File f(copy.fileName().c_str()); + CPPUNIT_ASSERT(f.isValid()); + CPPUNIT_ASSERT_EQUAL(String("ID3v2 Title"), f.ID3v2Tag()->title()); + CPPUNIT_ASSERT_EQUAL(String("INFO Title"), f.InfoTag()->title()); + RIFF::WAV::File original(TEST_FILE_PATH_C("empty.wav")); + CPPUNIT_ASSERT_EQUAL(original.audioProperties()->lengthInMilliseconds(), + f.audioProperties()->lengthInMilliseconds()); + } + } + void testStripAndProperties() { ScopedFileCopy copy("empty", ".wav");