On Sun, 29 Dec 2013 09:29:50 +0100, Luca Barbato <[email protected]> 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) {
> @@ -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;
nit: a bit strange to see the check after many other assignments
would be nice to reorder the operations
> +
> 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
This looks weird. Wouldn't the following be simpler:
for (i = 0; i < f->slice_count; i++) {
if (f->slice_context[i])
av_freep(&f->slice_context[i]->sample_buffer);
av_freep(&f->slice_context[i];
}
> + 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;
A weird loop again. Is there any reason you cannot just av_freep() them all?
--
Anton Khirnov
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
