diff mbox series

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

Message ID 20200331123745.6461-5-andreas.rheinhardt@gmail.com
State Superseded
Headers show
Series libavformat/mux patches
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Andreas Rheinhardt March 31, 2020, 12:37 p.m. UTC
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.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavformat/mux.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

Comments

Marton Balint March 31, 2020, 8:08 p.m. UTC | #1
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...

>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@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.

Marton
Andreas Rheinhardt April 10, 2020, 4:19 p.m. UTC | #2
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@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.
Marton Balint April 10, 2020, 5:25 p.m. UTC | #3
On Fri, 10 Apr 2020, Andreas Rheinhardt wrote:

> 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.

I tried to find a project on the internet which is using this API, failed 
to find one. Therefore I doubt that it is reasonable to keep it after the 
API bump (if we decide to drop it), it only makes sense to keep API 
deprectated longer which is indeed used in the wild.

>
>>>
>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@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.

The third option is to keep it broken, and apply the rest of the patch 
series. Yeah, a) seems like the best way, if you have the capacity to do 
it, but I'd simply ignore that it is broken, because IMHO it is simply a 
bad design which does not worth my time.

>
> 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?

Padding is required because we don't want av_buffer_realloc() in 
avcodec_encode_video/audio to realloc the packet buf, because it has 
internal pointers (extended_data) which points to the AVFrame itself, so 
a simple memcpy on the data to another place breaks it.

> **: 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.

I'd say keep signature, but change documentation. And also assert() if 
AVFrame is reset to NULL. Technically a break, but IMHO acceptable.

Regards,
Marton
diff mbox series

Patch

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:
+        av_frame_free(&frame);
+    }
+    return ret;
 }
 
 int av_write_uncoded_frame(AVFormatContext *s, int stream_index,