[FFmpeg-devel] [PATCH 6/8] avcodec/smacker: Directly goto error in case of error

[Andreas Rheinhardt]

Paul B Mahol:
> On 7/31/20, Andreas Rheinhardt <andreas.rheinhardt at gmail.com> wrote:
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
>> ---
>>  libavcodec/smacker.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/libavcodec/smacker.c b/libavcodec/smacker.c
>> index 8a4d88cfed..4b1f0f1b7c 100644
>> --- a/libavcodec/smacker.c
>> +++ b/libavcodec/smacker.c
>> @@ -251,17 +251,18 @@ static int smacker_decode_header_tree(SmackVContext
>> *smk, GetBitContext *gb, int
>>          err = AVERROR(ENOMEM);
>>          goto error;
>>      }
>> +    *recodes = huff.values;
>>
>>      res = smacker_decode_bigtree(gb, &huff, &ctx, 0);
>> -    if (res < 0)
>> +    if (res < 0) {
>>          err = res;
>> +        goto error;
>> +    }
>>      skip_bits1(gb);
>>      if(ctx.last[0] == -1) ctx.last[0] = huff.current++;
>>      if(ctx.last[1] == -1) ctx.last[1] = huff.current++;
>>      if(ctx.last[2] == -1) ctx.last[2] = huff.current++;
>>
>> -    *recodes = huff.values;
> 
> Commit log does not explain this change at all.
> And it looks wrong at first look.
> 

huff.values is freshly allocated and if an error happens after its
allocation, it either needs to be stored permanently to not go out of
scope or it needs to be freed in this function. The latter option would
lead to redundant code (one can just reuse the decoder's close
function), so neither the old nor my proposed new code does this. The
old code solves this problem by not jumping to error in case
smacker_decode_bigtree() fails, my new code solves this problem by
storing the array immediately after its allocation (before
smacker_decode_bigtree()). (Another option would be to store the array
after the error: label, but I think storing something as soon as
possible is better practice).

- Andreas

>> -
>>  error:
>>      for (int i = 0; i < 2; i++) {
>>          if (vlc[i].table)
>> --
>> 2.20.1
>>