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
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
