Merge pull request #12464 from daschuer/mp3_fix

mp3 garbage detection fix
mixxxdj · Dec 25, 2023 · 869b95b · 869b95b
2 parents 8c66d6a + 1757037
commit 869b95b
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 4 deletions.
diff --git a/src/sources/soundsourcemp3.cpp b/src/sources/soundsourcemp3.cpp
@@ -121,7 +121,18 @@ bool decodeFrameHeader(
         mad_stream* pMadStream,
         bool skipId3Tag) {
     DEBUG_ASSERT(!hasUnrecoverableError(pMadStream));
-    if (mad_header_decode(pMadHeader, pMadStream)) {
+    int ret = mad_header_decode(pMadHeader, pMadStream);
+    if (pMadHeader->flags & MAD_FLAG_FREEFORMAT) {
+        // perform missing sanity check for Layer I and II
+        // See libmad frame.c free_bitrate()
+        if ((pMadHeader->layer == MAD_LAYER_I && pMadHeader->bitrate > 896000) ||
+                (pMadHeader->layer == MAD_LAYER_II && pMadHeader->bitrate > 768000)) {
+            pMadStream->error = MAD_ERROR_LOSTSYNC;
+            pMadStream->sync = 0;
+            ret = -1;
+        }
+    }
+    if (ret) {
         // Something went wrong when decoding the frame header...
         DEBUG_ASSERT(pMadStream->error != MAD_ERROR_NONE);
         if (MAD_ERROR_BUFLEN == pMadStream->error) {
@@ -130,7 +141,8 @@ bool decodeFrameHeader(
         }
         if (isUnrecoverableError(pMadStream->error)) {
             kLogger.warning() << "Unrecoverable MP3 header decoding error:"
-                              << mad_stream_errorstr(pMadStream);
+                              << mad_stream_errorstr(pMadStream)
+                              << pMadStream->this_frame - pMadStream->buffer;
             return false;
         }
         if ((pMadStream->error == MAD_ERROR_LOSTSYNC) && skipId3Tag) {
@@ -148,7 +160,8 @@ bool decodeFrameHeader(
         // worry users when logged as a warning. The issue will become
         // obsolete once we switched to FFmpeg for MP3 decoding.
         kLogger.info() << "Recoverable MP3 header decoding error:"
-                       << mad_stream_errorstr(pMadStream);
+                       << mad_stream_errorstr(pMadStream)
+                       << pMadStream->this_frame - pMadStream->buffer;
         logFrameHeader(kLogger.info(), *pMadHeader);
         return false;
     }
@@ -731,7 +744,7 @@ ReadableSampleFrames SoundSourceMp3::readSampleFramesClamped(
             if (madFrameChannelCount != getSignalInfo().getChannelCount()) {
                 kLogger.warning() << "MP3 frame header with mismatching number of channels"
                                   << madFrameChannelCount << "<>" << getSignalInfo().getChannelCount()
-                                  << " - aborting";
+                                  << " - aborting @" << m_madStream.this_frame - m_madStream.buffer;
                 abortReading = true;
             }
 #endif

diff --git a/src/test/id3-test-data/free_mode_garbage.mp3 b/src/test/id3-test-data/free_mode_garbage.mp3
diff --git a/src/test/soundproxy_test.cpp b/src/test/soundproxy_test.cpp
@@ -1069,3 +1069,20 @@ TEST_F(SoundSourceProxyTest, fileSuffixWithDifferingType) {
                 SoundSourceProxy::isFileSuffixSupported(fileSuffix));
     }
 }
+
+TEST_F(SoundSourceProxyTest, freeModeGarbage) {
+    // Try to load a file with an insane bitrate in the garbage before the real frame.
+    QString filePath = getTestDir().filePath(
+            QStringLiteral("id3-test-data/free_mode_garbage.mp3"));
+    ASSERT_TRUE(SoundSourceProxy::isFileNameSupported(filePath));
+    const auto fileUrl = QUrl::fromLocalFile(filePath);
+    const auto providerRegistrations =
+            SoundSourceProxy::allProviderRegistrationsForUrl(fileUrl);
+    for (const auto& providerRegistration : providerRegistrations) {
+        mixxx::AudioSourcePointer pContReadSource = openAudioSource(
+                filePath,
+                providerRegistration.getProvider());
+        ASSERT_TRUE(pContReadSource != nullptr);
+        break;
+    }
+}