On 10/02/14 14:03, Janne Grunau wrote: > 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
Thanks for spotting. >> @@ -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? Probably we could zero it from start. > 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. I prefer be sure than sorry. > and while identical &f->slice_context[i] is probably easier to read I prefer not use &foo[i] when possible because it is less easier to read and & could be typoed away too easily. >> + 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? As above. >> 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. right. lu _______________________________________________ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel