FluidDecoder: fix out-of-bounds reads and unchecked file I/O - #68
Conversation
The MTrk length is a 32-bit big-endian field, but it was read into an
int32_t while the sibling fields around it (trackHeader just above, and
headerLength earlier in the function) are uint32_t.
A length with the top bit set therefore became negative, and in
while (blockPtr < trackHeaderLength)
the negative int was converted to unsigned int by the usual arithmetic
conversions, turning the loop bound into roughly 2^31 and letting the
body index far past the end of the heap buffer. The same value also fed
ptr += trackHeaderLength + 8;
which moved ptr backwards.
Reaching this needs a length field of 0x80000000 or more, that is a
claimed track of 2 GiB, so it is not something an encoder produces or a
user trips over -- it is hardening against a crafted file. The ordinary
out-of-bounds read in the same loop, where a perfectly normal positive
length simply runs off the end of a truncated file, is a separate matter
and is fixed by the later commit that bounds the parsing against the
file length.
This is also what the -Wsign-compare warning on the inner loop points at.
data is uint8_t, so each element promotes to int before the shift, and a byte of 0x80 or more shifted left by 24 overflows int. Since C++20 that overflow is well defined and wraps, so the assembled value is already correct where the addon is built as C++20. It pins no standard -- CMakeLists.txt sets no CMAKE_CXX_STANDARD -- so it takes the toolchain default, which is gnu++17 on gcc 14 and older, and there the shift is undefined. Cast the top byte to uint32_t so the shift happens in an unsigned type and the result does not depend on which standard the toolchain picks. The value produced is unchanged. The low three bytes cannot overflow and are left alone.
kodi::vfs::CFile::GetLength() returns int64_t and is documented to return -1 on error. It was assigned to an int and used unchecked: - on an error, new uint8_t[-1] throws std::bad_alloc out of ReadTag(), which handles nothing; - on a file shorter than 8 bytes, data[0] through data[7] are read past the end of the allocation -- an empty .mid is enough, and AddressSanitizer reports a read 2 bytes past a 1-byte region; - past 2 GiB the int64_t is truncated, though no real MIDI reaches that. Keep the int64_t and reject anything shorter than the 14-byte MThd header chunk, the smallest input the parser can read without running off the end: it reads data[0..7] and then starts at offset 14. The same test rejects the -1 error case. Widening len also silences a -Wsign-compare warning on the outer loop, whose unsigned counter was compared against the signed length.
Neither loop checked that the bytes it was about to read were inside the buffer: - the outer loop tested ptr < len but then read up to data[ptr + 7], so a truncated file over-read by as much as seven bytes; - the inner loop was bounded only by the track length taken from the file, and read data[blockPtr + ptr + 8] through [+ 11] plus a further blockLength bytes of event text, none of it checked against len. This is the case that needs no crafted input. A 30-byte file whose MTrk header claims 200 bytes of track data -- an interrupted copy -- walks off the end on the second pass of the inner loop, and AddressSanitizer reports a read 2 bytes past the 30-byte region at data[blockPtr + ptr + 10]. Bound both loops against len, and widen the two offsets to int64_t so the comparisons cannot wrap. The truncated file above now yields the title from the events that are actually present and stops.
The early return taken when the file does not start with a valid MThd chunk left the freshly allocated buffer behind, so every malformed or mis-named file handed to ReadTag() leaked its whole length. The addon registers .mid with tags="true", so Kodi calls this during a library scan. LeakSanitizer on a 1000-byte non-MIDI file reports "Direct leak of 1000 byte(s) in 1 object(s)".
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
heitbaum
changed the title
|
The extra 2 commits were planned as a second PR - including them here. b233f2e bounds the parse by the bytes valgrind on the parser with a stub VFS returning fewer bytes than requested, After: silent. With earlier data left in the heap, the unpatched build also puts 87a2395 applies the same two fixes to Retitled, as this now covers more than the tag scanner. |
|
AI use: Good fix overall. One remaining issue: an event can stay inside the file but extend past its declared MTrk chunk into the next chunk. Also add for std::nothrow. diff --git a/src/FluidDecoder.cpp b/src/FluidDecoder.cpp
index 1f51ae4..xxxxxxx 100644
--- a/src/FluidDecoder.cpp
+++ b/src/FluidDecoder.cpp
@@ -8,6 +8,7 @@
#include "FluidDecoder.h"
#include <kodi/Filesystem.h>
#include <kodi/General.h>
+#include <new>
@@ -143,6 +144,9 @@ bool CFluidCodec::ReadTag(const std::string& filename, kodi::addon::AudioDecoder
if (blockLength == 0 || blockIdentifier == MIDI_CHANNEL_PREFIX)
break;
+ if (blockPtr + 4 + blockLength > trackHeaderLength)
+ break;
+
if (ptr + blockPtr + 12 + blockLength > len)
break; |
len is taken straight from the file, so ReadTag() asks for an allocation whose size the scanned file controls. A plain new throws std::bad_alloc when that cannot be satisfied, and nothing here catches it, so the exception would unwind out of ReadTag() and across the addon interface. The function already tests the returned pointer, but that test could never fire against a throwing new -- it is a leftover from the pre-standard behaviour that new replaced with std::bad_alloc. Allocate with std::nothrow so the check it already has becomes the thing that handles the failure.
CFile::Read() returns the number of bytes it stored, or -1, and the result was discarded. A short read -- a network VFS, a truncated or failing read -- therefore left the tail of the allocation uninitialised, and the parser went on to treat it as MIDI data, bounded by the length the file advertised rather than by what arrived. The title could then be built out of uninitialised heap, which Kodi stores in the music database. Keep the advertised length as size, use it only to allocate and to ask for the read, and let len be what Read() actually returned. The existing bounds are all expressed in terms of len, so they now follow the bytes that are really there, and the same test rejects the -1 error return.
Init() loads the whole file to hand to fluid_player_add_mem(), and had the same two faults ReadTag() did. GetLength() is documented to return -1 on error, and here it was stored in a size_t, so that became SIZE_MAX and new uint8_t[] threw std::bad_alloc with nothing to catch it -- out of Init() and across the addon interface. Read()'s result was discarded and size was passed on to the player, so a short read gave fluidsynth the untouched tail of the allocation to parse as MIDI. Take the length as int64_t and reject it if it is not positive, allocate with std::nothrow so the null check means something, and hand the player the number of bytes Read() actually returned. Bailing out after the fluid context is created is safe: ~CFluidCodec() already frees player, synth and settings.
An event occupies four header bytes plus blockLength bytes of text, and blockPtr advances by that much, but the loop only tested blockPtr against trackHeaderLength on entry. An event starting just inside the chunk could therefore have its text run past the end of the chunk and into the one that follows. The file-length bound added earlier keeps that inside the buffer, so this is not a memory-safety problem, but the bytes it reads belong to the next chunk rather than to this track, and they end up in the title. Stop at the chunk boundary as well.
thanks for the review - new squashed in, and "event inside" added. |
garbear
left a comment
garbear
left a comment
There was a problem hiding this comment.
Reviewed against xbmc/xbmc#29361
No blocking findings on current head 15e1f5b.
The previously raised MTrk boundary issue is fixed, and <new> is now explicitly included. I found no new correctness, regression, build-code, or spelling blockers.
|
Thanks for the fix! |
|
@heitbaum Is this stlll shipped? Your PR was to the Nexus branch. I noticed problems and stopped maintaining this. Should we ship it with Piers? |
We are still shipping the Nexus branch in our Piers - ideally update the branch to Piers for the automation. It is the only one tagged with Nexus. (Additionally there are the 76 game.libretro. tagged as Omega) |
|
Apparently FluidDecoder was never ported to Omega or Piers. We don't build it on Jenkins, and translations don't work. You're free to ship it as unmaintained, and perform the maintenance yourself. I'll be here to merge any work you bring. |
Sounds a plan. If you don’t mind tagging it as 20.2.3-Nexus, it update the LE package.mk and lets flag it as unsupported in LE23, see if anyone pops up then. |
|
Tagged |
2 participants
Building the addon with gcc 16 gives two
-Wsign-comparewarnings insrc/FluidDecoder.cpp:Both are loop bounds in
ReadTag(), which parses MIDI chunk headers by hand to pull a title out of the file. Neither turned out to be cosmetic: in both cases the signed side of the comparison comes from the file being scanned, and the loops read past the end of the heap buffer.Two commits clear the warnings. The other four are what fixing them properly turned up in the same function.
int32_twhile the fields either side of it areuint32_t; a length with the top bit set went negative and the inner loop bound became ~2^31 — clears the:123warning<< 24; well defined since C++20, undefined when built as C++17, which is the default on gcc ≤ 14 and which the addon does not pinGetLength()returnsint64_tand-1on error; it was truncated tointand used without a check — clears the:112warningstd::nothrowlenis file-controlled, and a throwingnewwould unwind out ofReadTag()across the addon interface; the null check the function already had could never fire against itTesting
Built against Kodi 22 in a LibreELEC 13.0 x86_64 cross build, gcc 16.2.0: warnings go 2 → 0 and the addon links.
src/FluidDecoder.cpp.Checked under AddressSanitizer by extracting the parser into a harness (the loops verbatim, only the
kodi::vfscalls stubbed), before and after:.midtitle="Hello"title="Hello"The 30-byte case is an ordinary truncated copy — no crafted input needed.