[FFmpeg-devel] [PATCH 04/10] avformat/mux: Fix leaks of uncoded frames on errors

[Andreas Rheinhardt]

Marton Balint:
> 
> 
> On Tue, 31 Mar 2020, Andreas Rheinhardt wrote:
> 
>> If writing an uncoded frame fails at the preparatory steps of
>> av_[interleaved_]write_frame(), the frame would be either not freed
>> at all in case of av_write_frame() or would leak when the fake packet
>> would be unreferenced like an ordinary packet.
>> There is also a memleak when the output format is not suited for
>> writing uncoded frames as the documentation states that ownership of the
>> frame passes to av_[interleaved_]write_uncoded_frame(). Both of these
>> memleaks have been fixed.
> 
> Yuck, does using uncoded_frames muxing have any benefit over using
> AV_CODEC_ID_WRAPPED_AVFRAME with normal muxing? If not, then I'd very
> much like to see uncoded frames dropped with the next bump...
> 
I'd like to see them dropped, too, but the earliest possible time at
which they can be removed is the bump after the next major bump.

>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
>> ---
>> libavformat/mux.c | 23 ++++++++++++++++++-----
>> 1 file changed, 18 insertions(+), 5 deletions(-)
>>
>> diff --git a/libavformat/mux.c b/libavformat/mux.c
>> index 814d773b9d..a0ebcec119 100644
>> --- a/libavformat/mux.c
>> +++ b/libavformat/mux.c
>> @@ -1239,7 +1239,11 @@ int av_interleaved_write_frame(AVFormatContext
>> *s, AVPacket *pkt)
>>             return ret;
>>     }
>> fail:
>> -    av_packet_unref(pkt);
>> +    // This is a deviation from the usual behaviour
>> +    // of av_interleaved_write_frame: We leave cleaning
>> +    // up uncoded frames to write_uncoded_frame_internal.
>> +    if (!(pkt->flags & AV_PKT_FLAG_UNCODED_FRAME))
>> +        av_packet_unref(pkt);
>>     return ret;
>> }
>>
>> @@ -1324,10 +1328,13 @@ static int
>> write_uncoded_frame_internal(AVFormatContext *s, int stream_index,
>>                                         AVFrame *frame, int interleaved)
>> {
>>     AVPacket pkt, *pktp;
>> +    int ret;
>>
>>     av_assert0(s->oformat);
>> -    if (!s->oformat->write_uncoded_frame)
>> -        return AVERROR(ENOSYS);
>> +    if (!s->oformat->write_uncoded_frame) {
>> +        ret = AVERROR(ENOSYS);
>> +        goto free;
>> +    }
>>
>>     if (!frame) {
>>         pktp = NULL;
>> @@ -1343,8 +1350,14 @@ static int
>> write_uncoded_frame_internal(AVFormatContext *s, int stream_index,
>>         pkt.flags |= AV_PKT_FLAG_UNCODED_FRAME;
>>     }
>>
>> -    return interleaved ? av_interleaved_write_frame(s, pktp) :
>> -                         av_write_frame(s, pktp);
>> +    ret = interleaved ? av_interleaved_write_frame(s, pktp) :
>> +                        av_write_frame(s, pktp);
>> +    if (ret < 0 && pktp) {
>> +        frame = (AVFrame*)pktp->data;
>> +    free:
> 
> I think we always align labels to the start of the line.
> 
>> +        av_frame_free(&frame);
>> +    }
>> +    return ret;
>> }
>>
>> int av_write_uncoded_frame(AVFormatContext *s, int stream_index,
> 
> LGTM, thanks.

It is actually not good at all, but I only found this out later: As the
commit message says, this fixes leaks if one of the preparatory steps
fails. But if there is an error when actually writing the frame, then
this will lead to a double free in the non-interleaved codepath is used
because of:
        AVFrame *frame = (AVFrame *)pkt->data;
        av_assert0(pkt->size == UNCODED_FRAME_PACKET_SIZE);
        ret = s->oformat->write_uncoded_frame(s, pkt->stream_index,
&frame, 0);
        av_frame_free(&frame);

(Both times frame should be replaced by (AVFrame **)&pkt->data.)

And then the patch for no longer modifying packets we don't own needs to
be adapted, too: Using a spare packet for uncoded frames is not only
unnecessary, but harmful, because then the original packet would still
contain a dangling pointer to the frame even after it has been freed.

So far this stuff is easily fixable (and I have fixed it locally), but
there is more:
1. As I already said, there is a leak in case the functions for
interleaved output are used if the trailer is never written (e.g.
because an error occurs during an earlier write operation), because
ff_packet_list_free() is not aware of uncoded frames.
2. This is all very fragile. E.g. if one of these warnings in
write_packet() would be transformed into errors, then (all other things
equal) uncoded frames would leak if they have been written via the
interleaved codepath.
3. I do not even know whether the approach taken here (namely leaving
cleaning up to the callers in order to minimize the places where one
does cleanup) is compatible with Marton's patchset.

I see two ways going forward:
a) The way uncoded frames are handled is changed so that it respects the
ordinary AVBuffer-API: Ownership of the AVFrame is passed to an AVBuffer
with a custom free function (like it is for wrapped AVFrames). This will
automatically solve the first problem and it will simplify freeing more
generally (no constant checks for whether this is an uncoded frame).
The downsides of this are that it involves two allocations* per frame
and that the muxer must no longer have the ability to keep the AVFrame,
because the AVFrame is owned by an AVBuffer and the signature of
write_uncoded_frame does not allow to pass this reference to the
muxer.** But none of these muxers make use of this at all (and
av_frame_move_ref() is still fine).
b) Marton's patches get applied and I figure out later what the most
efficient way to fix these memleaks is. Fixing 1. would then imply that
the uncoded frame-hack can no longer be contained in mux.c.

My preferred approach is a).

- Andreas

*: In 436f00b10c062b75c7aab276c4a7d64524bd0444 Marton modified the
wrapped AVFrame encoder to create a new packet with padding. I don't
know if this would be needed here. I don't see any function (except the
muxer, of course) which actually looks at the packet's data in the
codepath here. Marton, do you still remember the exact function that
needed the padding?
**: Said signature is not part of the public API, but the muxers that
support writing uncoded frames reside in libavdevice and therefore we
can't simply change it.