diff mbox series

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

Message ID 20200731112241.8948-6-andreas.rheinhardt@gmail.com
State Accepted
Commit 4656c1771b4ac12d9df9812390551f32fda181bc
Headers show
Series [FFmpeg-devel,1/8] avcodec/smacker: Remove write-only and unused variables
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Andreas Rheinhardt July 31, 2020, 11:22 a.m. UTC
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavcodec/smacker.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Paul B Mahol Aug. 19, 2020, 7:24 p.m. UTC | #1
On 7/31/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@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.

> -
>  error:
>      for (int i = 0; i < 2; i++) {
>          if (vlc[i].table)
> --
> 2.20.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Andreas Rheinhardt Aug. 19, 2020, 7:36 p.m. UTC | #2
Paul B Mahol:
> On 7/31/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@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
>>
Paul B Mahol Aug. 19, 2020, 7:39 p.m. UTC | #3
On 8/19/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
> Paul B Mahol:
>> On 7/31/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

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;
-
 error:
     for (int i = 0; i < 2; i++) {
         if (vlc[i].table)