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

[Paul B Mahol]

On 8/19/20, Andreas Rheinhardt <andreas.rheinhardt at gmail.com> wrote:
> 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).

Add this explanation in commit log and patch is fine.

>
> - Andreas
>
>>> -
>>>  error:
>>>      for (int i = 0; i < 2; i++) {
>>>          if (vlc[i].table)
>>> --
>>> 2.20.1
>>>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".