On 30/12/13 17:02, Anton Khirnov wrote: > > 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?
In one case you are duplicating the memory since you have a copy of the context if you fail in the middle you have to free all you did alloc and NULL all you hadn't yet since it might be free later on close. lu _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
