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

RESOLVED FIXED in Firefox 45

Status

()

P1
critical
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: tsmith, Assigned: jya)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {csectype-nullptr, testcase})

unspecified
mozilla45
x86_64
Linux
csectype-nullptr, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(5 attachments)

(Reporter)

Description

3 years ago
Created attachment 8690196 [details]
call_stack.txt

Found while fuzzing nightly.
(Reporter)

Comment 1

3 years ago
Created attachment 8690197 [details]
test_case.mp4
(Assignee)

Comment 2

3 years ago
What version of ffmpeg are you testing against?

54 is rather super old
(Reporter)

Comment 3

3 years ago
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?
(Reporter)

Comment 4

3 years ago
$ 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
(Assignee)

Comment 5

3 years ago
This stack trace doesn't make much sense. That would be mTaskQueue being null, but you can only reach ProcessFlush upon dispatch on mTaskQueue.
(Assignee)

Comment 6

3 years ago
Created attachment 8690396 [details] [diff] [review]
P1 Ensure we won't operate on a decoder that failed to initialize.
Attachment #8690396 - Flags: review?(cpearce)
(Assignee)

Comment 7

3 years ago
Created attachment 8690397 [details] [diff] [review]
P2. Only create the type of decoder we will need.
Attachment #8690397 - Flags: review?(cpearce)
(Assignee)

Comment 8

3 years ago
Created attachment 8690398 [details] [diff] [review]
[ffmpeg] P3. Delete codec context upon initialization failure.

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+
(Assignee)

Comment 10

3 years ago
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.
(Assignee)

Comment 12

3 years ago
(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()
(Assignee)

Comment 13

3 years ago
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.
(Assignee)

Comment 14

3 years ago
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?
(Assignee)

Comment 16

3 years ago
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.
(Assignee)

Updated

3 years ago
Priority: -- → P1
Attachment #8690396 - Flags: review?(cpearce) → review+
Attachment #8690397 - Flags: review?(cpearce) → review+

Comment 18

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cc0de27135b5
https://hg.mozilla.org/mozilla-central/rev/9d36733ded4d
https://hg.mozilla.org/mozilla-central/rev/b94b0f9de8ba
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Reporter)

Updated

2 years ago
Blocks: 1289609
Can we land the testcase here as a crashtest?
Flags: needinfo?(jyavenard)
Flags: in-testsuite?
(Assignee)

Updated

2 years ago
Blocks: 1294581
(Assignee)

Updated

2 years ago
Flags: needinfo?(jyavenard)
You need to log in before you can comment on or make changes to this bug.