Closed Bug 1226707 Opened 9 years ago Closed 9 years ago

ASan: NULL deference in [@mozilla::FFmpegDataDecoder<54>::ProcessFlush()]

Categories

(Core :: Audio/Video: Playback, defect, P1)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: tsmith, Assigned: jya)

References

(Blocks 2 open bugs)

Details

(Keywords: csectype-nullptr, testcase)

Attachments

(5 files)

Attached file call_stack.txt
Found while fuzzing nightly.
Attached video test_case.mp4
What version of ffmpeg are you testing against?

54 is rather super old
Whatever comes with Ubuntu 14.04. I wasn't intending to target ffmpeg here I was feeding fuzzed files to the browser. Is there a pref I should be using to avoid this?
$ dpkg -l libavcodec54
Desired=Unknown/Install/Remove/Purge/Hold
| Status=Not/Inst/Conf-files/Unpacked/halF-conf/Half-inst/trig-aWait/Trig-pend
|/ Err?=(none)/Reinst-required (Status,Err: uppercase=bad)
||/ Name             Version       Architecture  Description
+++-================-=============-=============-======================================
rc  libavcodec54:amd 6:9.18-0ubunt amd64         Libav codec library
This stack trace doesn't make much sense. That would be mTaskQueue being null, but you can only reach ProcessFlush upon dispatch on mTaskQueue.
The MediaFormatReader will flush a decoder upon shutdown. So we ensure that the codec context is delete first. P1 and P2 fix the underlying issue ; but P3 is designed to be simple and risk-free so it can be uplifted to beta.
Attachment #8690398 - Flags: review?(gsquelart)
Assignee: nobody → jyavenard
Comment on attachment 8690398 [details] [diff] [review]
[ffmpeg] P3. Delete codec context upon initialization failure.

Review of attachment 8690398 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with proposed changes (if they make sense).

::: dom/media/platforms/ffmpeg/FFmpegDataDecoder.cpp
@@ +121,1 @@
>      return NS_ERROR_FAILURE;

According to the documentation, the codec context (and everything associated with it) allocated by avcodec_alloc_context3() should be freed with avcodec_free_context().
https://ffmpeg.org/doxygen/2.8/group__lavc__core.html#gae80afec6f26df6607eaacf39b561c315
If correct, you'll need to change the av_freep in ProcessShutdown() as well.

Also, there is another 'return NS_ERROR_FAILURE' in the following if-block, at line 130. Shouldn't you free the codec context there as well?
Attachment #8690398 - Flags: review?(gsquelart) → review+
This is not available in all version of libav plus here the codec context failed to be initialised so all it will do is free it. 

In the other place, so long that the codec context was properly initialised it's okay. Calling flush on it won't cause the crash (which only happen with libav 9 and earlier.
(In reply to Jean-Yves Avenard [:jya] from comment #10)
> This is not available in all version of libav

(Assuming 'this' means 'avcodec_free_context')
Can't you test for that, like you've done with other APIs?
I see it appeared in ffmpeg 2.3 as the suggested method to match avcodec_alloc_context3, instead of doing both avcodec_close then av_free.

> plus here the codec context
> failed to be initialised so all it will do is free it. 

I can see from current ffmpeg source that you're correct.
But you are taking the risk that in future versions avcodec_alloc_context3 will allocate more than one struct, in which case av_freep wouldn't be enough to clean up everything.

> In the other place, so long that the codec context was properly initialised
> it's okay. Calling flush on it won't cause the crash (which only happen with
> libav 9 and earlier.

Not sure we're talking about the same things (you: crash, me: leaks).
To be more explicit:

1. I think you should close&free the context at line 130, before the 2nd 'return NS_ERROR_FAILURE', so that the context does not leak if avcodec_open2 succeeded but InitDecoder failed (because we don't like the output format) -- I'm assuming InitDecoder failing means this FFmpegDataDecoder won't be used at all after that, not even shutdown.

2. I think you should use avcodec_free_context (if available) instead of |avcodec_close();av_freep(&mCodecContext);| everywhere, to avoid potential leaks. Agreed that it may not be necessary after a failed avcodec_open2 with current ffmpeg (though I still think it should be done for correctness and future-proofing), but after that (i.e., lines 130 and 200) using just av_freep means that some structs allocated by a successful avcodec_open2 will leak.

Of course my suggestions do not help with crashes, but I thought this bug could be an opportunity to handle nearby issues. Alternatively you may open another bug for these non-critical potential leaks.
(In reply to Gerald Squelart [:gerald] from comment #11)
> (Assuming 'this' means 'avcodec_free_context')
> Can't you test for that, like you've done with other APIs?
> I see it appeared in ffmpeg 2.3 as the suggested method to match
> avcodec_alloc_context3, instead of doing both avcodec_close then av_free.

I remember checking on that, and the avcodec version at which it appeared between libav and FFmpeg had already diverged too much; making the test of just avcodec major version insufficient to decide.

And I can't see a way to check if the libavcodec is from FFmpeg or libav fork; which is very annoying.

> 1. I think you should close&free the context at line 130, before the 2nd
> 'return NS_ERROR_FAILURE', so that the context does not leak if
> avcodec_open2 succeeded but InitDecoder failed (because we don't like the
> output format) -- I'm assuming InitDecoder failing means this
> FFmpegDataDecoder won't be used at all after that, not even shutdown.

MediaDataDecoder::Shutdown() is always called; regardless of Init failing or not. So it won't leak


> 
> 2. I think you should use avcodec_free_context (if available) instead of
> |avcodec_close();av_freep(&mCodecContext);| everywhere, to avoid potential
> leaks. Agreed that it may not be necessary after a failed avcodec_open2 with
> current ffmpeg (though I still think it should be done for correctness and
> future-proofing), but after that (i.e., lines 130 and 200) using just
> av_freep means that some structs allocated by a successful avcodec_open2
> will leak.

no they won't.
They are cleared in FFMpegDataDecoder::Shutdown()
One of the issue with our current FFMpegRuntimeLinking is that all symbols must be resolved; we can't attempt to resolve a symbol and if that symbol exists then use conditional code..

It would be good to have that.
Looking at the code for avcodec_free_context

void avcodec_free_context(AVCodecContext **pavctx)
{
    AVCodecContext *avctx = *pavctx;

    if (!avctx)
        return;

    avcodec_close(avctx);

    av_freep(&avctx->extradata);
    av_freep(&avctx->subtitle_header);
    av_freep(&avctx->intra_matrix);
    av_freep(&avctx->inter_matrix);
    av_freep(&avctx->rc_override);

    av_freep(pavctx);
}

That would cause our code to crash. It frees avctx->extradata and we certainly don't want to do that (it's a pointer to ouse MediaByteBuffer).
(In reply to Jean-Yves Avenard [:jya] from comment #14)
> Looking at the code for avcodec_free_context
> ...
> That would cause our code to crash. It frees avctx->extradata and we
> certainly don't want to do that (it's a pointer to our MediaByteBuffer).

Seems like it's a bug in avcodec_free_context! (avcodec_close does the right thing and only frees extradata for encoders, pfew)

In this case, you're right it cannot be used -- unless you manually reset the pointer before calling avcode_free_context.
But then you have the other issue of resolving symbols.

Fine, don't use it then!
Could you please just add a comment about that gotcha, so that later reviewers won't trip on this again?
Lodged https://trac.ffmpeg.org/ticket/5027

My guess is that it's too late for them to fix it properly. It is clearly stated in their documentation that the ownership of extradata is by the user, not avcodec.
Priority: -- → P1
Attachment #8690396 - Flags: review?(cpearce) → review+
Attachment #8690397 - Flags: review?(cpearce) → review+
Blocks: grizzly
Can we land the testcase here as a crashtest?
Flags: needinfo?(jyavenard)
Flags: in-testsuite?
Blocks: 1294581
Flags: needinfo?(jyavenard)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: