| Message ID | 20191016164739.15583-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | b288a7eb3d963a175e177b6219c8271076ee8590 |
| Headers | show |
LGTM On 10/16/19, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote: > If there is an error in mpeg_mux_init() (the write_header function of > the various MPEG-PS muxers), two things might happen: > > 1. Several fifos might leak. Instead of freeing them, the goto fail part > of the functions freed the private data of the AVStreams instead, > although this will be freed later in free_stream() anyway. > 2. And if the function is exited via goto fail, it automatically > returned AVERROR(ENOMEM), although this is also used when the error is > not a memory allocation failure. > > Both of these issues happened in ticket #8284 and have been fixed. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/mpegenc.c | 31 +++++++++++++++++++------------ > 1 file changed, 19 insertions(+), 12 deletions(-) > > diff --git a/libavformat/mpegenc.c b/libavformat/mpegenc.c > index 43ebc46e0e..93f40b202c 100644 > --- a/libavformat/mpegenc.c > +++ b/libavformat/mpegenc.c > @@ -315,7 +315,7 @@ static av_cold int mpeg_mux_init(AVFormatContext *ctx) > if (ctx->packet_size < 20 || ctx->packet_size > (1 << 23) + 10) { > av_log(ctx, AV_LOG_ERROR, "Invalid packet size %d\n", > ctx->packet_size); > - goto fail; > + return AVERROR(EINVAL); > } > s->packet_size = ctx->packet_size; > } else > @@ -343,7 +343,7 @@ static av_cold int mpeg_mux_init(AVFormatContext *ctx) > st = ctx->streams[i]; > stream = av_mallocz(sizeof(StreamInfo)); > if (!stream) > - goto fail; > + return AVERROR(ENOMEM); > st->priv_data = stream; > > avpriv_set_pts_info(st, 64, 1, 90000); > @@ -377,11 +377,11 @@ static av_cold int mpeg_mux_init(AVFormatContext *ctx) > for (sr = 0; sr < 4; sr++) > av_log(ctx, AV_LOG_INFO, " %d", > lpcm_freq_tab[sr]); > av_log(ctx, AV_LOG_INFO, "\n"); > - goto fail; > + return AVERROR(EINVAL); > } > if (st->codecpar->channels > 8) { > av_log(ctx, AV_LOG_ERROR, "At most 8 channels allowed > for LPCM streams.\n"); > - goto fail; > + return AVERROR(EINVAL); > } > stream->lpcm_header[0] = 0x0c; > stream->lpcm_header[1] = (st->codecpar->channels - 1) | (j > << 4); > @@ -416,7 +416,7 @@ static av_cold int mpeg_mux_init(AVFormatContext *ctx) > st->codecpar->codec_id != AV_CODEC_ID_MP2 && > st->codecpar->codec_id != AV_CODEC_ID_MP3) { > av_log(ctx, AV_LOG_ERROR, "Unsupported audio codec. > Must be one of mp1, mp2, mp3, 16-bit pcm_dvd, pcm_s16be, ac3 or dts.\n"); > - goto fail; > + return AVERROR(EINVAL); > } else { > stream->id = mpa_id++; > } > @@ -460,7 +460,7 @@ static av_cold int mpeg_mux_init(AVFormatContext *ctx) > } > stream->fifo = av_fifo_alloc(16); > if (!stream->fifo) > - goto fail; > + return AVERROR(ENOMEM); > } > bitrate = 0; > audio_bitrate = 0; > @@ -560,11 +560,6 @@ static av_cold int mpeg_mux_init(AVFormatContext *ctx) > s->system_header_size = get_system_header_size(ctx); > s->last_scr = AV_NOPTS_VALUE; > return 0; > - > -fail: > - for (i = 0; i < ctx->nb_streams; i++) > - av_freep(&ctx->streams[i]->priv_data); > - return AVERROR(ENOMEM); > } > > static inline void put_timestamp(AVIOContext *pb, int id, int64_t > timestamp) > @@ -1255,11 +1250,18 @@ static int mpeg_mux_end(AVFormatContext *ctx) > stream = ctx->streams[i]->priv_data; > > av_assert0(av_fifo_size(stream->fifo) == 0); > - av_fifo_freep(&stream->fifo); > } > return 0; > } > > +static void mpeg_mux_deinit(AVFormatContext *ctx) > +{ > + for (int i = 0; i < ctx->nb_streams; i++) { > + StreamInfo *stream = ctx->streams[i]->priv_data; > + av_fifo_freep(&stream->fifo); > + } > +} > + > #define OFFSET(x) offsetof(MpegMuxContext, x) > #define E AV_OPT_FLAG_ENCODING_PARAM > static const AVOption options[] = { > @@ -1289,6 +1291,7 @@ AVOutputFormat ff_mpeg1system_muxer = { > .write_header = mpeg_mux_init, > .write_packet = mpeg_mux_write_packet, > .write_trailer = mpeg_mux_end, > + .deinit = mpeg_mux_deinit, > .priv_class = &mpeg_class, > }; > #endif > @@ -1305,6 +1308,7 @@ AVOutputFormat ff_mpeg1vcd_muxer = { > .write_header = mpeg_mux_init, > .write_packet = mpeg_mux_write_packet, > .write_trailer = mpeg_mux_end, > + .deinit = mpeg_mux_deinit, > .priv_class = &vcd_class, > }; > #endif > @@ -1322,6 +1326,7 @@ AVOutputFormat ff_mpeg2vob_muxer = { > .write_header = mpeg_mux_init, > .write_packet = mpeg_mux_write_packet, > .write_trailer = mpeg_mux_end, > + .deinit = mpeg_mux_deinit, > .priv_class = &vob_class, > }; > #endif > @@ -1340,6 +1345,7 @@ AVOutputFormat ff_mpeg2svcd_muxer = { > .write_header = mpeg_mux_init, > .write_packet = mpeg_mux_write_packet, > .write_trailer = mpeg_mux_end, > + .deinit = mpeg_mux_deinit, > .priv_class = &svcd_class, > }; > #endif > @@ -1358,6 +1364,7 @@ AVOutputFormat ff_mpeg2dvd_muxer = { > .write_header = mpeg_mux_init, > .write_packet = mpeg_mux_write_packet, > .write_trailer = mpeg_mux_end, > + .deinit = mpeg_mux_deinit, > .priv_class = &dvd_class, > }; > #endif > -- > 2.20.1 > > _______________________________________________ > 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".
On Wed, Oct 16, 2019 at 06:53:44PM +0200, Paul B Mahol wrote:
> LGTM
will apply
[...]
diff --git a/libavformat/mpegenc.c b/libavformat/mpegenc.c index 43ebc46e0e..93f40b202c 100644 --- a/libavformat/mpegenc.c +++ b/libavformat/mpegenc.c @@ -315,7 +315,7 @@ static av_cold int mpeg_mux_init(AVFormatContext *ctx) if (ctx->packet_size < 20 || ctx->packet_size > (1 << 23) + 10) { av_log(ctx, AV_LOG_ERROR, "Invalid packet size %d\n", ctx->packet_size); - goto fail; + return AVERROR(EINVAL); } s->packet_size = ctx->packet_size; } else @@ -343,7 +343,7 @@ static av_cold int mpeg_mux_init(AVFormatContext *ctx) st = ctx->streams[i]; stream = av_mallocz(sizeof(StreamInfo)); if (!stream) - goto fail; + return AVERROR(ENOMEM); st->priv_data = stream; avpriv_set_pts_info(st, 64, 1, 90000); @@ -377,11 +377,11 @@ static av_cold int mpeg_mux_init(AVFormatContext *ctx) for (sr = 0; sr < 4; sr++) av_log(ctx, AV_LOG_INFO, " %d", lpcm_freq_tab[sr]); av_log(ctx, AV_LOG_INFO, "\n"); - goto fail; + return AVERROR(EINVAL); } if (st->codecpar->channels > 8) { av_log(ctx, AV_LOG_ERROR, "At most 8 channels allowed for LPCM streams.\n"); - goto fail; + return AVERROR(EINVAL); } stream->lpcm_header[0] = 0x0c; stream->lpcm_header[1] = (st->codecpar->channels - 1) | (j << 4); @@ -416,7 +416,7 @@ static av_cold int mpeg_mux_init(AVFormatContext *ctx) st->codecpar->codec_id != AV_CODEC_ID_MP2 && st->codecpar->codec_id != AV_CODEC_ID_MP3) { av_log(ctx, AV_LOG_ERROR, "Unsupported audio codec. Must be one of mp1, mp2, mp3, 16-bit pcm_dvd, pcm_s16be, ac3 or dts.\n"); - goto fail; + return AVERROR(EINVAL); } else { stream->id = mpa_id++; } @@ -460,7 +460,7 @@ static av_cold int mpeg_mux_init(AVFormatContext *ctx) } stream->fifo = av_fifo_alloc(16); if (!stream->fifo) - goto fail; + return AVERROR(ENOMEM); } bitrate = 0; audio_bitrate = 0; @@ -560,11 +560,6 @@ static av_cold int mpeg_mux_init(AVFormatContext *ctx) s->system_header_size = get_system_header_size(ctx); s->last_scr = AV_NOPTS_VALUE; return 0; - -fail: - for (i = 0; i < ctx->nb_streams; i++) - av_freep(&ctx->streams[i]->priv_data); - return AVERROR(ENOMEM); } static inline void put_timestamp(AVIOContext *pb, int id, int64_t timestamp) @@ -1255,11 +1250,18 @@ static int mpeg_mux_end(AVFormatContext *ctx) stream = ctx->streams[i]->priv_data; av_assert0(av_fifo_size(stream->fifo) == 0); - av_fifo_freep(&stream->fifo); } return 0; } +static void mpeg_mux_deinit(AVFormatContext *ctx) +{ + for (int i = 0; i < ctx->nb_streams; i++) { + StreamInfo *stream = ctx->streams[i]->priv_data; + av_fifo_freep(&stream->fifo); + } +} + #define OFFSET(x) offsetof(MpegMuxContext, x) #define E AV_OPT_FLAG_ENCODING_PARAM static const AVOption options[] = { @@ -1289,6 +1291,7 @@ AVOutputFormat ff_mpeg1system_muxer = { .write_header = mpeg_mux_init, .write_packet = mpeg_mux_write_packet, .write_trailer = mpeg_mux_end, + .deinit = mpeg_mux_deinit, .priv_class = &mpeg_class, }; #endif @@ -1305,6 +1308,7 @@ AVOutputFormat ff_mpeg1vcd_muxer = { .write_header = mpeg_mux_init, .write_packet = mpeg_mux_write_packet, .write_trailer = mpeg_mux_end, + .deinit = mpeg_mux_deinit, .priv_class = &vcd_class, }; #endif @@ -1322,6 +1326,7 @@ AVOutputFormat ff_mpeg2vob_muxer = { .write_header = mpeg_mux_init, .write_packet = mpeg_mux_write_packet, .write_trailer = mpeg_mux_end, + .deinit = mpeg_mux_deinit, .priv_class = &vob_class, }; #endif @@ -1340,6 +1345,7 @@ AVOutputFormat ff_mpeg2svcd_muxer = { .write_header = mpeg_mux_init, .write_packet = mpeg_mux_write_packet, .write_trailer = mpeg_mux_end, + .deinit = mpeg_mux_deinit, .priv_class = &svcd_class, }; #endif @@ -1358,6 +1364,7 @@ AVOutputFormat ff_mpeg2dvd_muxer = { .write_header = mpeg_mux_init, .write_packet = mpeg_mux_write_packet, .write_trailer = mpeg_mux_end, + .deinit = mpeg_mux_deinit, .priv_class = &dvd_class, }; #endif
If there is an error in mpeg_mux_init() (the write_header function of the various MPEG-PS muxers), two things might happen: 1. Several fifos might leak. Instead of freeing them, the goto fail part of the functions freed the private data of the AVStreams instead, although this will be freed later in free_stream() anyway. 2. And if the function is exited via goto fail, it automatically returned AVERROR(ENOMEM), although this is also used when the error is not a memory allocation failure. Both of these issues happened in ticket #8284 and have been fixed. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavformat/mpegenc.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-)