A trak may hold at most one tref. setQtChapters() appended a second one for
the chap reference instead of joining the atom already present, which happens
whenever the audio track references another track -- a timecode track, as in
camera recordings.
Parsers differ on the result: the lenient keep reading the track, the strict
discard it entirely. A file written this way therefore still reads correctly
in TagLib while presenting as having no audio elsewhere, and anything that
remuxes it from a stricter parser's track list writes an audioless copy back
to disk.
Removal follows the same rule inverted: only the chap box is taken out when
the tref is shared, and the tref itself goes only when chap was its sole
child. It also now looks for the tref that actually contains chap rather than
the first one on the track, so a file already carrying two is repaired rather
than stripped of the wrong reference.
Bound top-level ID3v2 parsing to 50000 frames. Crafted tags with
many small frames could otherwise consume excessive memory and crash
applications.
Stop parsing further frames after the limit while retaining the
successfully parsed tag data.
iTunes 12 writes ID3v2.2 three-character sort frames (TSA, TSP, TST,
TS2, TSC) inside ID3v2.3 tags, padded to four bytes with 0x20 (space)
rather than 0x00, which is tolerated by '#ifndef NO_ITUNES_HACKS'
code. Enhance that code to also tolerate padding with a space.
Reading the tags of a file materialised every attachment, so a fast read of
a file with cover art cost as much as the art is large, even though the
caller may never ask for it. Measured with a counting IOStream over
tests/data/no-tags.mka with an attachment behind its seek head, read with
AudioProperties::Fast:
attachment | read before | read now
----------------------------------
128 KiB | 131350 B | 278 B
512 KiB | 524544 B | 256 B
4 MiB | 4194563 B | 259 B
The number of read calls is unchanged (183, 164, 164 before; 182, 163, 163
now), so this is volume, not round trips, and what remains does not grow
with the payload.
MkAttachedFileData is now a DeferredBinaryElement, which registers the offset
of its data and skips over it. Matroska::File::attachments() reads the data
before handing the attachments out, so callers see no difference.
save() renders the attachments from the attached files, which would write an
empty attachment for data that was never requested. It therefore loads the
data before writing.
Co-authored-by: Claude Opus 5 <[email protected]>
Limit recursive CHAP and CTOC embedded frame parsing to 64 levels.
This prevents crafted ID3v2 tags from exhausting the parser stack.
Use one thread-local guard shared by both paths so mixed CHAP and
CTOC nesting is bounded as well.
Element::factory() rejects any element whose declared size runs past the bound it is given, and
read() passes the Fast scan limit as that bound. A Segment spans practically the whole file, so
under ReadStyle::Fast every Matroska over 512 KiB is rejected and no tags are read:
EBML: datasize too great: 1003369 > (524288 - 52)
Failed to find Matroska segment
The limit is readLimited()'s, which already applies it to skip Cues and to bound the walk over a
segment with no usable SeekHead. The lookup only needs the file length.
Correctly handle offsets and scan limits:
- maxOffset: Maximum offset from the beginning of the file; the end of
the element must be before this offset.
- scanLimit: Offset from the current file position until which scanning
for elements is allowed. Normally, elements are scanned up to the end
of the enclosing master element or the end of the file, but in Fast
reading mode, it is limited to FAST_SCAN_LIMIT, which is 512 kB.
- maxScanOffset: scanLimit from the current file position
---------
Co-authored-by: Claude Opus 5 <[email protected]>
Co-authored-by: Urs Fleisch <[email protected]>
Ogg::File::readPages() read pages from all logical bitstreams and
indexed their packets into one global list. In a multiplexed file the
Theora and Vorbis packets got interleaved, so packet 0 was a Theora
header rather than the expected Vorbis type-3 comment header.
Before reading packets, the Vorbis reader now selects the Vorbis
logical bitstream (the one whose first packet is the Vorbis
identification header), so it reads the correct stream regardless of
position. Packet parsing is scoped to the selected bitstream, so
packets from other codecs in the same file are ignored.
tests/data/multiplex.ogg is generated using
ffmpeg -hide_banner -y \
-f lavfi -i "color=c=navy:s=500x500:r=1:d=2" \
-f lavfi -i "sine=frequency=440:sample_rate=48000:duration=2" \
-map 0:v -map 1:a \
-c:v libtheora -q:v 3 \
-c:a libvorbis -ac 2 -q:a 2 \
-flags +bitexact -fflags +bitexact \
-metadata:s:a:0 TITLE="Paper Lights" \
-metadata:s:a:0 encoder= -metadata:s:v:0 encoder= \
-f ogg tests/data/multiplex.ogg
Ogg FLAC scans metadata blocks by repeatedly fetching Ogg packets.
Each packet lookup walks indexed pages from their beginning, so a file
with many small metadata blocks has quadratic parsing time and page
allocation.
Limit the metadata block count to 1024. This keeps the worst-case scan
bounded while allowing more than normal Ogg FLAC files require.
Xiph comment parsing retained an unbounded number of fields. A
crafted comment block with many small fields could consume
disproportionate memory.
Stop parsing comment fields when the parser limit is exceeded.
FLAC metadata scanning retained an unbounded number of blocks. A
crafted file with many small blocks could consume disproportionate
memory.
Reject files that exceed a maximum metadata block count.
ASF header parsing retained an unbounded number of objects. A crafted
file with many small objects could consume disproportionate memory.
Reject files whose header object count exceeds the parser limit.
RIFF files could contain an unbounded number of small chunks. The
parser retained a descriptor for each chunk, allowing a crafted file
to consume disproportionate memory.
Reject files that exceed a maximum parsed chunk count.
EBML containers could contain an unbounded number of small elements.
The parser retained each element, allowing a crafted Matroska file to
consume disproportionate memory.
Reject containers that exceed a per-level element count limit.
Nested MP4 containers did not enforce the atom count limit applied at
the root level. A small file with many child atoms could consume
disproportionate memory while building the atom tree.
Apply the per-level limit to container children and reject files that
exceed it.
SV8 packet parsing subtracted its header size from an unchecked
unsigned packet length. An undersized value could wrap and make the
parser allocate the remainder of a large file.
Reject packet lengths smaller than their header or beyond the remaining
file data before reading the payload.
MP4 chapter parsing subtracted table headers from atom lengths without
checking that the headers were present. A short atom could wrap the
read length and allocate the remainder of a large file.
Validate table sizes before reading their payloads and before updating
chunk offsets while saving.
A truncated ASF attribute object could declare a large count and make
the parser create empty attributes after reaching the end of its data.
Validate each attribute object's data extent and stop parsing when an
attribute would exceed it.
A crafted DSDIFF chunk size could wrap the parser boundary check,
seek backwards, and make the parser loop indefinitely.
Compare chunk sizes with remaining bytes before seeking at each DSDIFF
chunk nesting level.
Frame::fieldData() (taglib/mpeg/id3v2/id3v2frame.cpp) discarded any
ID3v2.4 frame whose declared size (from the header) no longer matched
its actual buffer size after per-frame unsynchronisation was decoded by
FrameFactory::prepareFrameHeader(), silently emptying frames like
TIT2/TPE1/TALB. Clamp the declared length to what's actually available
instead of discarding the frame, only bailing out if the frame's data
offset itself doesn't fit.
Adds testUnsynchDecodeID3v24Frame() to tests/test_id3v2.cpp, covering a
frame with its own per-frame Unsynchronisation flag (as opposed to the
tag-wide flag already covered by testUnsynchDecode()), using new fixture
tests/data/unsynch24.id3.
Express dependency on zlib using Requires.private in pkg-config .pc
file instead of adding -lz to the libs.
This will cause the following changes when using TagLib with
pkg-config:
- When using a dynamic library, -lz will no longer be present in
in linker command, but being linked transitively.
- When using a static library, pkg-config must be used with the
--static command line argument, then libs will still contain
-lz, which is needed when linking statically.
- The flags needed for zlib will be provided by pkg-config,
so if -lz is not appropriate (e.g. with MSVC), correct
linker flags will be provided.
A crafted compressed ID3v2 frame can declare an excessive output size
and cause zlib to allocate memory based on attacker-controlled data.
Bound decompression by an absolute 64 MiB limit and a 64:1 expansion
ratio, while retaining normal ID3v2.3 length handling.
A crafted embedded AIFF chunk size could wrap the parser offset and
make it repeatedly process the same chunk, causing a denial of service.
Validate AIFF chunk headers and padded sizes against the remaining
verbatim-header data, and advance every parsed chunk to its checked end.
The MAX_MP4_ATOM_COUNT_PER_LEVEL of 5000 seems to be too restrictive,
a legitimate file with 5390 atoms was reported to have been rejected.
The crafted file from #1344 had 653789 atoms at the top level, which
freezed the read process for 15s on my system. Enlarging the limit
to 50000 should be sufficient and will stop the crafted file after 2s.
In matroska an element data size of a VINT with all bits 1 means the
data size is unknown. Unknown data size can only apply to Master
Elements.
Unknown sized elements are described in
https://datatracker.ietf.org/doc/rfc8794/ section 6.2
It gives the following 5 conditions for detecting the end of an
unknown sized element:
* Any EBML Element that is a valid Parent Element of the Unknown-
Sized Element according to the EBML Schema, Global Elements
excluded.
* Any valid EBML Element according to the EBML Schema, Global
Elements excluded, that is not a Descendant Element of the
Unknown-Sized Element but shares a common direct parent, such as a
Top-Level Element.
* Any EBML Element that is a valid Root Element according to the
EBML Schema, Global Elements excluded.
* The end of the Parent Element with a known size has been reached.
* The end of the EBML Document, either when reaching the end of the
file or because a new EBML Header started.
In this patch we use the higher level maxOffset to determine
the maximum data size for the element, which matches the fourth
condition, but is incomplete without the other four methods.
As only Segment and Cluster elements of Matroska files are allowed
to use unknown size length and TagLib does not process Cluster
elements, this should be sufficient.
---------
Signed-off-by: Anthony Brandon <[email protected]>
Co-authored-by: Urs Fleisch <[email protected]>
Some files might have some invalid elements, but most elements might be
correct. In this case it would be useful to still parse the correct
elements and skip the invalid ones.
When an invalid element is encountered we can set the seek position in
the file to the (known) end of that Element. Then further elements can
be read starting from that position.
Signed-off-by: Anthony Brandon <[email protected]>
By checking the Element size against the maximum offset we can find
out of bounds elements early and not try to read them at all.
This is useful for the next patch.
Signed-off-by: Anthony Brandon <[email protected]>
Each time save is called registerSizeListener will add
new size listeners. If save is called multiple times this causes the
same delta being add/subtracted multiple times, resulting in incorrect
element sizes.
This can be observed in TestMatroska::testRepeatedSave.
Although the test passes, some sizes are incorrect resulting in the
following messages being printed during the test:
TagLib: Failed to read VINT size
TagLib: Failed to parse EMBL ElementID
Signed-off-by: Anthony Brandon <[email protected]>
XM saving failed for files with samples because the save logic only
advanced past sample headers and did not skip the sample data, causing
the next instrument to be written at the wrong position.
cnID must be longlong instead of int as Apple Music cnID values can now
exceed the range of a 32-bit integer and require 64-bit aka longlong.
---------
Co-authored-by: Urs Fleisch <[email protected]>
Fix: Don't silently drop ChapterAtom elements that omit MkChapterUID
parseChapterAtom() returned chapterUid = 0 when the source file omitted
the MkChapterUID element, and the existing call sites in
MkChapters::parse() (ebmlmkchapters.cpp lines 106 and 127) use a C++17
init-if `chapter.uid()` predicate that silently dropped such chapters.
The caller saw an empty ChapterEditionList for files that mkvinfo,
MediaInfo and FFmpeg all handle gracefully — produced by some audiobook
generators and older muxers that omit the per-chapter UID. The
companion orphan-EditionEntry fix in PR #1311 / commit e07b956f doesn't
cover this case because the ChapterAtoms there ARE wrapped in an
EditionEntry, just without a ChapterUID inside each atom.
This change:
- Synthesises a process-unique ChapterUID inside parseChapterAtom() when
the source file lacks MkChapterUID. The synthetic value sets the high
bit (1ULL << 63) and increments via a static std::atomic counter; real
ChapterUIDs are random 64-bit values from muxers, so collision with a
generated one is practically impossible while keeping the distinction
local to TagLib.
- The existing chapter.uid() filters at the call sites then always
evaluate truthy and the chapter is exposed through the public
ChapterEditionList API as if it were spec-compliant.
No existing behavior is changed — files that already conform to the spec
(ChapterAtoms with a ChapterUID element) parse identically; only
previously-dropped chapters are now surfaced.
Reported and verified against real-world chaptered Matroska audiobook
files where mkvinfo / MediaInfo see all chapters but TagLib 2.3 returned
an empty ChapterEditionList.
Per review feedback on #1364: moving the two new bool flags to the end
of FilePrivate, next to the existing `scanned` bool, lets the compiler
coalesce the three bytes into the same trailing padding slot. Saves one
machine word per FLAC::File instance versus placing the flags mid-struct
between bextData and the List<MetadataBlock*>.
Pure layout change, no behaviour difference. Test suite still green.
hasiXMLData() / hasBEXTData() were implemented as !data.isEmpty()
checks, which conflated in-memory payload with on-disk block presence.
That caused two wrong answers:
* setiXMLData("foo") on a file with no iXML block made hasiXMLData()
return true immediately, before save().
* A FLAC file carrying an iXML APPLICATION block with empty payload
round-tripped fine, but hasiXMLData() reported false.
Switch to the same model RIFF::WAV::File already uses: explicit
hasiXML / hasBEXT bool flags on FilePrivate, set during scan() when
the APPLICATION block is recognised, updated during save() after the
block is (re)written or omitted, and returned verbatim by the
accessors. New regression test pins down the before/after-save and
empty-block cases.
Refs: https://github.com/taglib/taglib/issues/1362
Ryan FrancesconiandGitHub
Acts1631andGitHub
Urs FleischandGitHub
Frederik SeiffertandGitHub
Thomas BergwinklandGitHub
EvilGivenandGitHub
dependabot[bot]andUrs Fleisch
HomerJauandUrs Fleisch
Ryan FrancesconiandUrs Fleisch
Stephen BoothandUrs Fleisch