[FFmpeg-devel] [PATCH] avformat/mpegenc: Fix ever growing FIFO and infinite loop on error

[FFmpeg-devel] [PATCH] avformat/mpegenc: Fix ever growing FIFO and infinite loop on error

[Andreas Rheinhardt]

Since the switch to the new FIFO API in commit
ea511196a6c85eb433e10cdbecb0b2c722faf20d, the FIFO is always
grown by the amount of data intended to be written into it
even in case the FIFO has enough free space. Fix this by
only growing the FIFO if needed and then only by the amount that is
actually needed.

The allocation errors that resulted from this uncovered another bug:
The context is left in an inconsistent state in case the FIFO can't
be grown, because the FIFO does not contain as much data as the sizes
contained in the PacketDesc list claim. This led to an infinite loop
in output_packet() (called from mpeg_mux_end()).

Fix this by growing the FIFO before adding a new PacketDesc element,
thereby preventing the context from becoming inconsistent.

Reported-by: Nicolas Gaullier <nicolas.gaullier@cji.paris>
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavformat/mpegenc.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/libavformat/mpegenc.c b/libavformat/mpegenc.c
index cc47a43288..62692bfcd1 100644
--- a/libavformat/mpegenc.c
+++ b/libavformat/mpegenc.c
@@ -1152,6 +1152,7 @@ static int mpeg_mux_write_packet(AVFormatContext *ctx, AVPacket *pkt)
     int64_t pts, dts;
     PacketDesc *pkt_desc;
     int preload, ret;
+    size_t can_write;
     const int is_iframe = st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO &&
                           (pkt->flags & AV_PKT_FLAG_KEY);
 
@@ -1192,6 +1193,14 @@ static int mpeg_mux_write_packet(AVFormatContext *ctx, AVPacket *pkt)
         size -= 3;
     }
 
+    /* Enlarge the FIFO before adding a new PacketDesc
+     * in order to avoid inconsistencies on failure. */
+    can_write = av_fifo_can_write(stream->fifo);
+    if (can_write < size) {
+        ret = av_fifo_grow2(stream->fifo, size - can_write);
+        if (ret < 0)
+            return ret;
+    }
     pkt_desc                 = av_mallocz(sizeof(PacketDesc));
     if (!pkt_desc)
         return AVERROR(ENOMEM);
@@ -1207,10 +1216,6 @@ static int mpeg_mux_write_packet(AVFormatContext *ctx, AVPacket *pkt)
     pkt_desc->unwritten_size =
     pkt_desc->size           = size;
 
-    ret = av_fifo_grow2(stream->fifo, size);
-    if (ret < 0)
-        return ret;
-
     if (s->is_dvd) {
         // min VOBU length 0.4 seconds (mpucoder)
         if (is_iframe &&
-- 
2.32.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avformat/mpegenc: Fix ever growing FIFO and infinite loop on error

[Andreas Rheinhardt]

Andreas Rheinhardt:
> Since the switch to the new FIFO API in commit
> ea511196a6c85eb433e10cdbecb0b2c722faf20d, the FIFO is always
> grown by the amount of data intended to be written into it
> even in case the FIFO has enough free space. Fix this by
> only growing the FIFO if needed and then only by the amount that is
> actually needed.
> 
> The allocation errors that resulted from this uncovered another bug:
> The context is left in an inconsistent state in case the FIFO can't
> be grown, because the FIFO does not contain as much data as the sizes
> contained in the PacketDesc list claim. This led to an infinite loop
> in output_packet() (called from mpeg_mux_end()).
> 
> Fix this by growing the FIFO before adding a new PacketDesc element,
> thereby preventing the context from becoming inconsistent.
> 
> Reported-by: Nicolas Gaullier <nicolas.gaullier@cji.paris>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavformat/mpegenc.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/libavformat/mpegenc.c b/libavformat/mpegenc.c
> index cc47a43288..62692bfcd1 100644
> --- a/libavformat/mpegenc.c
> +++ b/libavformat/mpegenc.c
> @@ -1152,6 +1152,7 @@ static int mpeg_mux_write_packet(AVFormatContext *ctx, AVPacket *pkt)
>      int64_t pts, dts;
>      PacketDesc *pkt_desc;
>      int preload, ret;
> +    size_t can_write;
>      const int is_iframe = st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO &&
>                            (pkt->flags & AV_PKT_FLAG_KEY);
>  
> @@ -1192,6 +1193,14 @@ static int mpeg_mux_write_packet(AVFormatContext *ctx, AVPacket *pkt)
>          size -= 3;
>      }
>  
> +    /* Enlarge the FIFO before adding a new PacketDesc
> +     * in order to avoid inconsistencies on failure. */
> +    can_write = av_fifo_can_write(stream->fifo);
> +    if (can_write < size) {
> +        ret = av_fifo_grow2(stream->fifo, size - can_write);
> +        if (ret < 0)
> +            return ret;
> +    }
>      pkt_desc                 = av_mallocz(sizeof(PacketDesc));
>      if (!pkt_desc)
>          return AVERROR(ENOMEM);
> @@ -1207,10 +1216,6 @@ static int mpeg_mux_write_packet(AVFormatContext *ctx, AVPacket *pkt)
>      pkt_desc->unwritten_size =
>      pkt_desc->size           = size;
>  
> -    ret = av_fifo_grow2(stream->fifo, size);
> -    if (ret < 0)
> -        return ret;
> -
>      if (s->is_dvd) {
>          // min VOBU length 0.4 seconds (mpucoder)
>          if (is_iframe &&

Will apply this tonight unless there are objections.

- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".