BMediaFile crashes within AVFormatReader::Sniff() if it cannot handle the file

[ttcoder]

If a file is not audio, or is an mp3 with embedded artwork (JFIF/JPEG) in it, BMediaFile crashes. First seen in our own audio app, though the crash is easily reproduced with MediaPlayer as well.

Seems to have been there a long time: the crash occurs in PM builds but also in old hrev45824; though I didn't find a prior ticket that matches: #6595 is a crash that occurs in the dtor due to post-Open() init, whereas this crash occurs even before Open() is ever called.

We can workaround the non-audio file case easily; we can also probably workaround the embedded art case by loading the file in memory and scanning for 'JFIF' before passing it to BMediaFile but that's of course very hackish. And this will only fix it for us, not for MediaPlayer.

Attaching backtrace...

[ttcoder]

MediaPlayer crashes opening an mp3 with embedded art

[anevilyak]

Looks like a problem in the ffmpeg plugin's AVFormatReader implementation.

[ttcoder]

MediaPlayer masks most of the problem (but not the embedded art crash) since it filters files on their MIME type. I'm not anxious to upload an example file with embedded art as the one I have is 9 MB; but reproducing this bug with MediaPlayer is doable though it needs a bit of work:

cp myvalid_audio_file.mp3 foo.mp3
> foo.mp3  #this bash redirect will reset foo's size to 0 bytes, which is a valid case to trigger the crash
MediaPlayer foo.mp3

I would have done it with touch and addatr BEOS:TYPE audio/mpeg <file> except it does not work, as addattr adds a CSTR whereas it needs to be a MIMS... But the above works with the right preparation anyway.

[anevilyak]

Replying to ttcoder:

I would have done it with touch and addatr BEOS:TYPE audio/mpeg <file> except it does not work, as addattr adds a CSTR whereas it needs to be a MIMS... But the above works with the right preparation anyway.

That's what addattr -t mime is for.

[phoudoin]

While the stack trace may suggest it crashed in ctor, maybe it's as in #6595, in StreamBase'dtor. Since #6595 fix, fIOContext was changed to a pointer, and av_free(fIOContext->buffer) is called without checking fIOContext was actually allocated. May I suggest to check fIOContext is not null before doing that, as it's still a possible case...

[ttcoder]

Good catch, reading the constructor and destructor side by side it's clear that the dtor should test for pointer validity!

Otherwise just declaring the variable on the stack..

   {
      StreamBase stream;
   }

... and letting it go out of scope makes it crash!

And even without going to that extreme, leaving it in its natural ffmpeg ecosystem :-) it can crash just the same, if Open() fails at any of the last 2 points of failure. So regardless of whether or not this ticket is caused by this variable mishandling, there should be a fix for it indeed:

 StreamBase::~StreamBase()
 {
-       av_free(fIOContext->buffer);
+       if (fIOContext)
+               av_free(fIOContext->buffer);
        av_free(fIOContext);

Tried compiling ffmpeg here but it wants to build the whole libroot.so and kitchen sink before doing it so maybe it would take less time to hack a custom jamfile, which will still take time..

Anyway no need for somebody to check the validity of the "patch", it's painfully obvious it should be applied.

I'll take it on myself of downloading a nightly once the change is commited and check if it fixes this ticket and report here for sure.

[ttcoder]

Fixed by pulkomandy in hrev46441, yay! (and thanks!)

[anevilyak]

Thanks for the note!