[Scummvm-git-logs] scummvm master -> ce76f5b8cf9ac52b13825d446661cc980f8cd086

[sev-]

This automated email contains information about 2 new commits which have been
pushed to the 'scummvm' repo located at https://github.com/scummvm/scummvm .

Summary:
e3f4117535 COMMON: Avoid Quicktime parsing crash on early EOF
ce76f5b8cf COMMON: Add basic tests for quicktime parser


Commit: e3f4117535e0bd232a50cc2f284efdb8baf2c9db
    https://github.com/scummvm/scummvm/commit/e3f4117535e0bd232a50cc2f284efdb8baf2c9db
Author: Matthew Duggan (mgithub at guarana.org)
Date: 2023-11-18T21:03:02+01:00

Commit Message:
COMMON: Avoid Quicktime parsing crash on early EOF

In parseStream, atom.size is initailized to uint32 max.  If the loop in
readDefault then never executes because the stream has hit EOF, that size is
passed as-is to seek.  This can cause a crash eg in SeekableSubReadStream
because of an int overflow.

Change readDefault to return an error in a few places if we have hit eof before
a valid atom has been read.  It's ok not to do the seek in this case.

Changed paths:
    common/formats/quicktime.cpp


diff --git a/common/formats/quicktime.cpp b/common/formats/quicktime.cpp
index 4153f517caf..abd9b39f56f 100644
--- a/common/formats/quicktime.cpp
+++ b/common/formats/quicktime.cpp
@@ -181,6 +181,9 @@ int QuickTimeParser::readDefault(Atom atom) {
 
 	a.offset = atom.offset;
 
+	if (_fd->eos() || _fd->err() || (_fd->pos() == _fd->size()))
+		return -1;
+
 	while(((total_size + 8) < atom.size) && !_fd->eos() && _fd->pos() < _fd->size() && !err) {
 		a.size = atom.size;
 		a.type = 0;
@@ -232,6 +235,9 @@ int QuickTimeParser::readDefault(Atom atom) {
 			uint32 start_pos = _fd->pos();
 			err = (this->*_parseTable[i].func)(a);
 
+			if (!err && (_fd->eos() || _fd->err()))
+				err = -1;
+
 			uint32 left = a.size - _fd->pos() + start_pos;
 
 			if (left > 0) // skip garbage at atom end


Commit: ce76f5b8cf9ac52b13825d446661cc980f8cd086
    https://github.com/scummvm/scummvm/commit/ce76f5b8cf9ac52b13825d446661cc980f8cd086
Author: Matthew Duggan (mgithub at guarana.org)
Date: 2023-11-18T21:03:02+01:00

Commit Message:
COMMON: Add basic tests for quicktime parser

Changed paths:
  A test/common/formats/quicktime.h
    test/module.mk


diff --git a/test/common/formats/quicktime.h b/test/common/formats/quicktime.h
new file mode 100644
index 00000000000..93f44d7ec51
--- /dev/null
+++ b/test/common/formats/quicktime.h
@@ -0,0 +1,83 @@
+#include <cxxtest/TestSuite.h>
+#include "common/util.h"
+#include "common/formats/quicktime.h"
+
+static const byte VALID_MOOV_DATA[] = { // a minimally 'correct' quicktime file.
+	// size				'moov'					size				'mdat'
+	0x0, 0x0, 0x0, 0x8, 0x6d, 0x6f, 0x6f, 0x76, 0x0, 0x0, 0x0, 0x8, 0x6d, 0x64, 0x61, 0x74
+};
+
+static const byte VALID_MHDR_DATA[] = { // a 'correct' quicktime file with a header
+	// size	(incl mvhd)	'moov'
+	0x0, 0x0, 0x0, 0x74, 0x6d, 0x6f, 0x6f, 0x76,
+	//size (27*4)		'mvhd'					vers  3bytes flags
+	0x0, 0x0, 0x0, 0x6c, 0x6d, 0x76, 0x68, 0x64, 0x00, 0xff, 0xff, 0xff,
+	// creation				modification			timescale (60?)		length (999 * 60)+ 1
+	0x65, 0x52, 0xef, 0x5b, 0x65, 0x52, 0xef, 0x5b, 0x0, 0x0, 0x0, 0x3c, 0x0, 0x0, 0xea, 0x25,
+	// preferred scale, vol, 		[10 bytes reserved]
+	0x0, 0x0, 0x0, 0x1, 0x0, 0x10, 0,0,0,0,0,0,0,0,0,0,
+	// display matrix, mostly ignored by parser except xMod (0x8000) and yMod (0xa000)
+	0x0, 0x0, 0x80, 0x0, 0,0,0,0,0,0,0,0,0,0,0,0, 0x0, 0x0, 0xa0, 0x0, 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+	// 7 more 32-bit values
+	0,0,0,0, 0,0,0,0, 0,0,0,0, 0,0,0,0, 0,0,0,0, 0,0,0,0, 0,0,0,0,
+	// size				'mdat'
+	0x0, 0x0, 0x0, 0x8, 0x6d, 0x64, 0x61, 0x74
+};
+
+
+class QuickTimeTestParser : public Common::QuickTimeParser {
+public:
+	uint32 getDuration() const { return _duration; }
+	const Common::Rational &getScaleFactorX() const { return _scaleFactorX; }
+	const Common::Rational &getScaleFactorY() const { return _scaleFactorY; }
+	const Common::Array<Track *> &getTracks() const { return _tracks; }
+
+	SampleDesc *readSampleDesc(Track *track, uint32 format, uint32 descSize) override {
+		return nullptr;
+	}
+};
+
+class QuicktimeParserTestSuite : public CxxTest::TestSuite {
+public:
+	void test_streamAtEOS() {
+		QuickTimeTestParser parser;
+		const byte data[] = "";
+		Common::MemoryReadStream stream(data, sizeof(data));
+		stream.readByte(); // read the null char
+		bool result = parser.parseStream(&stream, DisposeAfterUse::NO);
+		TS_ASSERT(!result);
+	}
+
+	void test_streamInvalid() {
+		QuickTimeTestParser parser;
+		const byte data[] = "not a moov";
+		Common::MemoryReadStream stream(data, sizeof(data));
+		bool result = parser.parseStream(&stream, DisposeAfterUse::NO);
+		TS_ASSERT(!result);
+	}
+
+	void test_moov() {
+		QuickTimeTestParser parser;
+		Common::MemoryReadStream stream(VALID_MOOV_DATA, sizeof(VALID_MOOV_DATA));
+		bool result = parser.parseStream(&stream, DisposeAfterUse::NO);
+		TS_ASSERT(result);
+	}
+
+	void test_mhdr() {
+		QuickTimeTestParser parser;
+		Common::MemoryReadStream stream(VALID_MHDR_DATA, sizeof(VALID_MHDR_DATA));
+		bool result = parser.parseStream(&stream, DisposeAfterUse::NO);
+		TS_ASSERT(result);
+		TS_ASSERT_EQUALS(parser.getDuration(), 999*60 + 1);
+		TS_ASSERT_EQUALS(parser.getScaleFactorX(), Common::Rational(0x10000, 0x8000));
+		TS_ASSERT_EQUALS(parser.getScaleFactorY(), Common::Rational(0x10000, 0xa000));
+	}
+
+	void test_mhdrEarlyEOF() {
+		QuickTimeTestParser parser;
+		Common::MemoryReadStream stream(VALID_MHDR_DATA, sizeof(VALID_MHDR_DATA) - 10);
+		bool result = parser.parseStream(&stream, DisposeAfterUse::NO);
+		TS_ASSERT(!result);
+	}
+
+};
diff --git a/test/module.mk b/test/module.mk
index 6116d3ef6e9..185ea9a1a72 100644
--- a/test/module.mk
+++ b/test/module.mk
@@ -5,7 +5,7 @@
 #
 ######################################################################
 
-TESTS        := $(srcdir)/test/common/*.h $(srcdir)/test/audio/*.h $(srcdir)/test/math/*.h $(srcdir)/test/image/*.h
+TESTS        := $(srcdir)/test/common/*.h $(srcdir)/test/common/formats/*.h $(srcdir)/test/audio/*.h $(srcdir)/test/math/*.h $(srcdir)/test/image/*.h
 TEST_LIBS    :=
 
 ifdef POSIX