On 2014-02-09 19:28:15 +0100, Luca Barbato wrote:
> Plug some leaks and free on non-allocated pointers.
> ---
> libavcodec/ffv1.c | 29 +++++++++++++++++++++++++----
> libavcodec/ffv1dec.c | 14 +++++++++-----
> 2 files changed, 34 insertions(+), 9 deletions(-)
>
> diff --git a/libavcodec/ffv1.c b/libavcodec/ffv1.c
> index 9e7ba2e..1bc4273 100644
> --- a/libavcodec/ffv1.c
> +++ b/libavcodec/ffv1.c
> @@ -189,7 +189,7 @@ int ffv1_init_slice_state(FFV1Context *f, FFV1Context *fs)
>
> av_cold int ffv1_init_slice_contexts(FFV1Context *f)
> {
> - int i;
> + int i, j;
>
> f->slice_count = f->num_h_slices * f->num_v_slices;
> if (f->slice_count <= 0) {
since I found it nowhere else this needs a check for f->slice_count >
MAX_SLICES
> @@ -205,6 +205,10 @@ av_cold int ffv1_init_slice_contexts(FFV1Context *f)
> int sxe = f->avctx->width * (sx + 1) / f->num_h_slices;
> int sys = f->avctx->height * sy / f->num_v_slices;
> int sye = f->avctx->height * (sy + 1) / f->num_v_slices;
> +
> + if (!fs)
> + goto fail;
> +
> f->slice_context[i] = fs;
> memcpy(fs, f, sizeof(*fs));
> memset(fs->rc_stat2, 0, sizeof(fs->rc_stat2));
> @@ -216,10 +220,21 @@ av_cold int ffv1_init_slice_contexts(FFV1Context *f)
>
> fs->sample_buffer = av_malloc(3 * MAX_PLANES * (fs->width + 6) *
> sizeof(*fs->sample_buffer));
> - if (!fs->sample_buffer)
> - return AVERROR(ENOMEM);
> + if (!fs->sample_buffer) {
> + av_freep(f->slice_context + i);
> + goto fail;
> + }
> }
> return 0;
> +
> +fail:
> + for (j = 0; j < i; j++) {
> + av_free(f->slice_context[i]->sample_buffer);
> + av_freep(f->slice_context + j);
> + }
> + for (; i < f->slice_count; i++)
> + f->slice_context[i] = NULL;
is there a reason to believe that these could be not null?
The error handling looks imho too complex while technically correct.
I would just go over all f->slice_count contexts free
f->slice_context[i]->sample_buffer if (f->slice_context[i]) and
av_freep() the slice context then.
and while identical &f->slice_context[i] is probably easier to read
> + return AVERROR(ENOMEM);
> }
>
> int ffv1_allocate_initial_states(FFV1Context *f)
> @@ -229,8 +244,14 @@ int ffv1_allocate_initial_states(FFV1Context *f)
> for (i = 0; i < f->quant_table_count; i++) {
> f->initial_states[i] = av_malloc(f->context_count[i] *
> sizeof(*f->initial_states[i]));
> - if (!f->initial_states[i])
> + if (!f->initial_states[i]) {
> + int j;
> + for (j = 0; j < i; j++)
> + av_freep(f->initial_states + j);
> + for (; i < f->quant_table_count; i++)
> + f->initial_states[i] = NULL;
same, is there any reason to believe one of those is not NULL?
> return AVERROR(ENOMEM);
> + }
> memset(f->initial_states[i], 128,
> f->context_count[i] * sizeof(*f->initial_states[i]));
> }
> diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c
> index 98fd9d8..dd31d89 100644
> --- a/libavcodec/ffv1dec.c
> +++ b/libavcodec/ffv1dec.c
> @@ -805,15 +805,19 @@ static av_cold int ffv1_decode_init(AVCodecContext
> *avctx)
>
> ffv1_common_init(avctx);
>
> - f->last_picture = av_frame_alloc();
> - if (!f->last_picture)
> - return AVERROR(ENOMEM);
> -
> if (avctx->extradata && (ret = read_extra_header(f)) < 0)
read_extra_header() might have already allocate memory so this needs
ffv1_close() too.
> return ret;
>
> - if ((ret = ffv1_init_slice_contexts(f)) < 0)
> + if ((ret = ffv1_init_slice_contexts(f)) < 0) {
> + ffv1_close(avctx);
> return ret;
> + }
> +
> + f->last_picture = av_frame_alloc();
> + if (!f->last_picture) {
> + ffv1_close(avctx);
> + return AVERROR(ENOMEM);
> + }
no need to change the allocation order
Janne
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
