Crash trying to parse MP4 file

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Download the video clip from http://x264dev.multimedia.cx/?p=212 (actual
clip is at http://www.mediafire.com/?amt4ge2mkzj)
2. Try to open the file with mp4v2
3. MP4v2 crashes

Please provide any additional information below.

I'm not totally sure where MP4v2 is crashing; I think it may have something
to do with the hdlr atom though.  All of my testing was done with r355.

Original issue reported on code.google.com by kid...@gmail.com on 14 Jan 2010 at 12:52

[GoogleCodeExporter]

Sorry, let me clarify--mp4v2 doesn't "crash," but a call to MP4Read will fail 
due to
an internal exception (which, of course, MP4v2 catches).  The exception happens 
in
MP4HdlrAtom::Read():

// There is a spec incompatiblity between QT and MP4
// QT says name field is a counted string
// MP4 says name field is a null terminated string
// Here we attempt to make all things work
void MP4HdlrAtom::Read()
{
    // read all the properties but the "name" field
    ReadProperties(0, 5);
    uint64_t pos = m_pFile->GetPosition();
    uint64_t end = GetEnd();
    if (pos == end) return;

    // take a peek at the next byte
    uint8_t strLength;
    m_pFile->PeekBytes(&strLength, 1);
    // if the value matches the remaining atom length
    if (pos + strLength + 1 == end) {
        // read a counted string
        MP4StringProperty* pNameProp =
            (MP4StringProperty*)m_pProperties[5];
        pNameProp->SetCountedFormat(true);
        ReadProperties(5);
        pNameProp->SetCountedFormat(false);
    } else {
        // read a null terminated string
        ReadProperties(5);  <-- EXCEPTION
    }

    Skip(); // to end of atom
}

Specifically, the exception thrown is:

void MP4Atom::ReadProperties(uint32_t startIndex, uint32_t count)
{
    uint32_t numProperties = min(count, m_pProperties.Size() - startIndex);

    // read any properties of the atom
    for (uint32_t i = startIndex; i < startIndex + numProperties; i++) {

        m_pProperties[i]->Read(m_pFile);

        if (m_pFile->GetPosition() > m_end) {
            VERBOSE_READ(GetVerbosity(),
                         printf("ReadProperties: insufficient data for property: %s
pos 0x%" PRIx64 " atom end 0x%" PRIx64 "\n",
                                m_pProperties[i]->GetName(),
                                m_pFile->GetPosition(), m_end));

            ostringstream oss;
            oss << "atom '" << GetType() << "' is too small; overrun at property: "
<< m_pProperties[i]->GetName();
            throw new MP4Error( oss.str().c_str(), "Atom ReadProperties" );
        }

For this file, m_pFile->GetPosition() returns 766; m_end is 765--so it sounds 
like
it's .  VLC, WMP (on windows 7) and a few other players I have digest this 
file; I'm
not sure if it's because they're more permissive than mp4v2 and the source 
material
is illegitimate, or if this is a bug in mp4v2.  

Let me know if you need anymore info.

Original comment by kid...@gmail.com on 14 Jan 2010 at 1:58

[GoogleCodeExporter]

I can fix this by catching in MP4HdlrAtom, in the section to read a null 
terminated
string:

    // take a peek at the next byte
    uint8_t strLength;
    m_pFile->PeekBytes(&strLength, 1);
    // if the value matches the remaining atom length
    if (pos + strLength + 1 == end) {
        // read a counted string
        MP4StringProperty* pNameProp =
            (MP4StringProperty*)m_pProperties[5];
        pNameProp->SetCountedFormat(true);
        ReadProperties(5);
        pNameProp->SetCountedFormat(false);
    } else {
        // read a null terminated string
        try {
            ReadProperties(5);
        }
        catch( MP4Error* ) {
            //SetEnd(m_pFile->GetPosition());
        }
    }

    Skip(); // to end of atom

The particular issue with this file seems to be that the string isn't null
terminated.  The only saving grace is that the subsequent atom ("minf") has a 
size
value that's small enough to act as a null terminator for the string.  If I 
catch,
the subsequent Skip() command sets the position to 765 (which is in fact the 
correct
position), and everything works out fine.

So I'm not sure what to do about this.  Clearly, this file is invalid.  But it 
also
plays back in WMP on Windows 7, VLC, quicktime, etc.  Suggestions?

my best guess is this file was made with some version of mp4box, but I'm not 
totally
certain.

Original comment by kid...@gmail.com on 3 Apr 2010 at 2:36

[GoogleCodeExporter]

Changed catch to be more specific, and also clean up correctly after itself:

if (pos + strLength + 1 == end) {
        // read a counted string
        MP4StringProperty* pNameProp =
            (MP4StringProperty*)m_pProperties[5];
        pNameProp->SetCountedFormat(true);
        ReadProperties(5);
        pNameProp->SetCountedFormat(false);
    } else {
        // read a null terminated string
        try {
            // Unfortunately, there are some invalid mp4 writers that don't
            // null the hdlr name string.  Generally this will be "automatically"
            // terminated for them by the size field of the subsequent atom.  So if
            // our size is off by one...let it slide.  otherwise, rethrow.
            // The Skip() call will set our start to the correct location
            // for the next Atom. See issue #52
            ReadProperties(5);
        }
        catch(MP4Error*e) { 
            if( m_pFile->GetPosition() - GetEnd() == 1 )
                delete e;
            else
                throw e;
        }
    }

    Skip(); // to end of atom

Still not sure if we should be doing this (technically the file is invalid), but
considering every other player I could throw this file at would read 
successfully...I
think it's a reasonable change.  fixed in r379

Original comment by kid...@gmail.com on 4 Apr 2010 at 2:34

• Changed state: Fixed