Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit a7847b3f authored by Marco Nelissen's avatar Marco Nelissen
Browse files

Fix handling of ID3v2.4 extended header 

If an optional ID3v2.4 extended header was present, it would be
treated as a frame, causing parse errors. Fix by doing ID3v2.4
per-frame unsynchronization after skipping over the extended
header.

Bug: 154357105
Bug: 151448144
Test: CTS, manual w/ temporary additional logging.
Change-Id: Iaeb8cab866b98dcf97f81d3ade4a1c8d26add3a5
parent 141095dc

media/libstagefright/id3/ID3.cpp

+48 −49
Original line number Diff line number Diff line
@@ -107,20 +107,6 @@ ID3::ID3(DataSourceHelper *sourcehelper, bool ignoreV1, off64_t offset) 
    }

}



ID3::ID3(DataSourceBase *source, bool ignoreV1, off64_t offset)

    : mIsValid(false),

      mData(NULL),

      mSize(0),

      mFirstFrameOffset(0),

      mVersion(ID3_UNKNOWN),

      mRawSize(0) {

    mIsValid = parseV2(source, offset);



    if (!mIsValid && !ignoreV1) {

        mIsValid = parseV1(source);

    }

}



ID3::ID3(const uint8_t *data, size_t size, bool ignoreV1)

    : mIsValid(false),

      mData(NULL),
@@ -247,44 +233,14 @@ struct id3_header { 
        return false;

    }



    if (header.version_major == 4) {

        void *copy = malloc(size);

        if (copy == NULL) {

            free(mData);

            mData = NULL;

            ALOGE("b/24623447, no more memory");

            return false;

        }



        memcpy(copy, mData, size);



        bool success = removeUnsynchronizationV2_4(false /* iTunesHack */);

        if (!success) {

            memcpy(mData, copy, size);

            mSize = size;



            success = removeUnsynchronizationV2_4(true /* iTunesHack */);



            if (success) {

                ALOGV("Had to apply the iTunes hack to parse this ID3 tag");

            }

        }



        free(copy);

        copy = NULL;



        if (!success) {

            free(mData);

            mData = NULL;



            return false;

        }

    } else if (header.flags & 0x80) {

    // first handle global unsynchronization

    if (header.flags & 0x80) {

        ALOGV("removing unsynchronization");



        removeUnsynchronization();

    }



    // handle extended header, if present

    mFirstFrameOffset = 0;

    if (header.version_major == 3 && (header.flags & 0x40)) {

        // Version 2.3 has an optional extended header.
@@ -296,6 +252,7 @@ struct id3_header { 
            return false;

        }



        // v2.3 does not have syncsafe integers

        size_t extendedHeaderSize = U32_AT(&mData[0]);

        if (extendedHeaderSize > SIZE_MAX - 4) {

            free(mData);
@@ -367,6 +324,48 @@ struct id3_header { 
        mFirstFrameOffset = ext_size;

    }



    // Handle any v2.4 per-frame unsynchronization

    // The id3 spec isn't clear about what should happen if the global

    // unsynchronization flag is combined with per-frame unsynchronization,

    // or whether that's even allowed, so this code assumes id3 writing

    // tools do the right thing and not apply double-unsynchronization,

    // but will honor the flags if they are set.

    if (header.version_major == 4) {

        void *copy = malloc(size);

        if (copy == NULL) {

            free(mData);

            mData = NULL;

            ALOGE("b/24623447, no more memory");

            return false;

        }



        memcpy(copy, mData, size);



        bool success = removeUnsynchronizationV2_4(false /* iTunesHack */);

        if (!success) {

            memcpy(mData, copy, size);

            mSize = size;



            success = removeUnsynchronizationV2_4(true /* iTunesHack */);



            if (success) {

                ALOGV("Had to apply the iTunes hack to parse this ID3 tag");

            }

        }



        free(copy);

        copy = NULL;



        if (!success) {

            free(mData);

            mData = NULL;



            return false;

        }

    }







    if (header.version_major == 2) {

        mVersion = ID3_V2_2;

    } else if (header.version_major == 3) {
@@ -411,7 +410,7 @@ static void WriteSyncsafeInteger(uint8_t *dst, size_t x) { 
bool ID3::removeUnsynchronizationV2_4(bool iTunesHack) {

    size_t oldSize = mSize;



    size_t offset = 0;

    size_t offset = mFirstFrameOffset;

    while (mSize >= 10 && offset <= mSize - 10) {

        if (!memcmp(&mData[offset], "\0\0\0\0", 4)) {

            break;
@@ -445,7 +444,7 @@ bool ID3::removeUnsynchronizationV2_4(bool iTunesHack) { 
        }



        if ((flags & 2) && (dataSize >= 2)) {

            // This file has "unsynchronization", so we have to replace occurrences

            // This frame has "unsynchronization", so we have to replace occurrences

            // of 0xff 0x00 with just 0xff in order to get the real data.



            size_t readOffset = offset + 11;

media/libstagefright/id3/test/ID3Test.cpp

+11 −5
Original line number Diff line number Diff line
@@ -24,6 +24,7 @@ 
#include <datasource/FileSource.h>



#include <media/stagefright/foundation/hexdump.h>

#include <media/MediaExtractorPluginHelper.h>

#include <ID3.h>



#include "ID3TestEnvironment.h"
@@ -42,7 +43,8 @@ TEST_P(ID3tagTest, TagTest) { 
    string path = gEnv->getRes() + GetParam();

    sp<FileSource> file = new FileSource(path.c_str());

    ASSERT_EQ(file->initCheck(), (status_t)OK) << "File initialization failed! \n";

    ID3 tag(file.get());

    DataSourceHelper helper(file->wrap());

    ID3 tag(&helper);

    ASSERT_TRUE(tag.isValid()) << "No valid ID3 tag found for " << path.c_str() << "\n";



    ID3::Iterator it(tag, nullptr);
@@ -61,7 +63,8 @@ TEST_P(ID3versionTest, VersionTest) { 
    sp<android::FileSource> file = new FileSource(path.c_str());

    ASSERT_EQ(file->initCheck(), (status_t)OK) << "File initialization failed! \n";



    ID3 tag(file.get());

    DataSourceHelper helper(file->wrap());

    ID3 tag(&helper);

    ASSERT_TRUE(tag.isValid()) << "No valid ID3 tag found for " << path.c_str() << "\n";

    ASSERT_TRUE(tag.version() >= versionNumber)

            << "Expected version: " << tag.version() << " Found version: " << versionNumber;
@@ -73,7 +76,8 @@ TEST_P(ID3textTagTest, TextTagTest) { 
    sp<android::FileSource> file = new FileSource(path.c_str());

    ASSERT_EQ(file->initCheck(), (status_t)OK) << "File initialization failed! \n";



    ID3 tag(file.get());

    DataSourceHelper helper(file->wrap());

    ID3 tag(&helper);

    ASSERT_TRUE(tag.isValid()) << "No valid ID3 tag found for " << path.c_str() << "\n";

    int countTextFrames = 0;

    ID3::Iterator it(tag, nullptr);
@@ -99,7 +103,8 @@ TEST_P(ID3albumArtTest, AlbumArtTest) { 
    sp<android::FileSource> file = new FileSource(path.c_str());

    ASSERT_EQ(file->initCheck(), (status_t)OK) << "File initialization failed! \n";



    ID3 tag(file.get());

    DataSourceHelper helper(file->wrap());

    ID3 tag(&helper);

    ASSERT_TRUE(tag.isValid()) << "No valid ID3 tag found for " << path.c_str() << "\n";

    size_t dataSize;

    String8 mime;
@@ -124,7 +129,8 @@ TEST_P(ID3multiAlbumArtTest, MultiAlbumArtTest) { 
    sp<android::FileSource> file = new FileSource(path.c_str());

    ASSERT_EQ(file->initCheck(), (status_t)OK) << "File initialization failed! \n";



    ID3 tag(file.get());

    DataSourceHelper helper(file->wrap());

    ID3 tag(&helper);

    ASSERT_TRUE(tag.isValid()) << "No valid ID3 tag found for " << path.c_str() << "\n";

    int count = 0;

    ID3::Iterator it(tag, nullptr);

media/libstagefright/id3/testid3.cpp

+3 −1
Original line number Diff line number Diff line
@@ -24,6 +24,7 @@ 
#include <binder/ProcessState.h>

#include <datasource/FileSource.h>

#include <media/stagefright/foundation/ADebug.h>

#include <media/MediaExtractorPluginHelper.h>



#define MAXPATHLEN 256
@@ -72,7 +73,8 @@ void scanFile(const char *path) { 
    sp<FileSource> file = new FileSource(path);

    CHECK_EQ(file->initCheck(), (status_t)OK);



    ID3 tag(file.get());

    DataSourceHelper helper(file->wrap());

    ID3 tag(&helper);

    if (!tag.isValid()) {

        printf("FAIL %s\n", path);

    } else {

media/libstagefright/include/ID3.h

+0 −1
Original line number Diff line number Diff line
@@ -37,7 +37,6 @@ struct ID3 { 
    };



    explicit ID3(DataSourceHelper *source, bool ignoreV1 = false, off64_t offset = 0);

    explicit ID3(DataSourceBase *source, bool ignoreV1 = false, off64_t offset = 0);

    ID3(const uint8_t *data, size_t size, bool ignoreV1 = false);

    ~ID3();