diff mbox series

[FFmpeg-devel,18/40] avcodec/alacenc: Don't free unnecessarily

Message ID 20200914052747.124118-2-andreas.rheinhardt@gmail.com
State Accepted
Commit ce482266a641a2cbd2900c0c2e4afb24f26cc422
Headers show
Series [FFmpeg-devel,01/16] avcodec/snowdec: Use ff_snow_common_init() directly
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 Sept. 14, 2020, 5:27 a.m. UTC
The init function of the ALAC encoder calls its own close function
if a call to ff_lpc_init() fails; yet nothing has been allocated before
that point (except extradata which is freed generically) and ff_lpc_init()
can be expected to clean up after itself on error (the documentation does
not say anything to the contrary and the current implementation can only
fail if the only allocation fails, so there is nothing to clean up on
error anyway), so this is unnecessary.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavcodec/alacenc.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

Comments

Paul B Mahol Sept. 14, 2020, 4:32 p.m. UTC | #1
On Mon, Sep 14, 2020 at 07:27:25AM +0200, Andreas Rheinhardt wrote:
> The init function of the ALAC encoder calls its own close function
> if a call to ff_lpc_init() fails; yet nothing has been allocated before
> that point (except extradata which is freed generically) and ff_lpc_init()
> can be expected to clean up after itself on error (the documentation does
> not say anything to the contrary and the current implementation can only
> fail if the only allocation fails, so there is nothing to clean up on
> error anyway), so this is unnecessary.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavcodec/alacenc.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
> 

what about adding cleanup flag so if new stuff is added(unlikely) new bugs
wont pop up?

> diff --git a/libavcodec/alacenc.c b/libavcodec/alacenc.c
> index fc5fa270e6..9d135d1350 100644
> --- a/libavcodec/alacenc.c
> +++ b/libavcodec/alacenc.c
> @@ -535,10 +535,8 @@ static av_cold int alac_encode_init(AVCodecContext *avctx)
>                                                   avctx->bits_per_raw_sample);
>  
>      avctx->extradata = av_mallocz(ALAC_EXTRADATA_SIZE + AV_INPUT_BUFFER_PADDING_SIZE);
> -    if (!avctx->extradata) {
> -        ret = AVERROR(ENOMEM);
> -        goto error;
> -    }
> +    if (!avctx->extradata)
> +        return AVERROR(ENOMEM);
>      avctx->extradata_size = ALAC_EXTRADATA_SIZE;
>  
>      alac_extradata = avctx->extradata;
> @@ -566,8 +564,7 @@ FF_DISABLE_DEPRECATION_WARNINGS
>             avctx->min_prediction_order > ALAC_MAX_LPC_ORDER) {
>              av_log(avctx, AV_LOG_ERROR, "invalid min prediction order: %d\n",
>                     avctx->min_prediction_order);
> -            ret = AVERROR(EINVAL);
> -            goto error;
> +            return AVERROR(EINVAL);
>          }
>  
>          s->min_prediction_order = avctx->min_prediction_order;
> @@ -578,8 +575,7 @@ FF_DISABLE_DEPRECATION_WARNINGS
>              avctx->max_prediction_order > ALAC_MAX_LPC_ORDER) {
>              av_log(avctx, AV_LOG_ERROR, "invalid max prediction order: %d\n",
>                     avctx->max_prediction_order);
> -            ret = AVERROR(EINVAL);
> -            goto error;
> +            return AVERROR(EINVAL);
>          }
>  
>          s->max_prediction_order = avctx->max_prediction_order;
> @@ -591,8 +587,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
>          av_log(avctx, AV_LOG_ERROR,
>                 "invalid prediction orders: min=%d max=%d\n",
>                 s->min_prediction_order, s->max_prediction_order);
> -        ret = AVERROR(EINVAL);
> -        goto error;
> +        return AVERROR(EINVAL);
>      }
>  
>      s->avctx = avctx;
> @@ -600,13 +595,10 @@ FF_ENABLE_DEPRECATION_WARNINGS
>      if ((ret = ff_lpc_init(&s->lpc_ctx, avctx->frame_size,
>                             s->max_prediction_order,
>                             FF_LPC_TYPE_LEVINSON)) < 0) {
> -        goto error;
> +        return ret;
>      }
>  
>      return 0;
> -error:
> -    alac_encode_close(avctx);
> -    return ret;
>  }
>  
>  static int alac_encode_frame(AVCodecContext *avctx, AVPacket *avpkt,
> -- 
> 2.25.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 Sept. 14, 2020, 4:43 p.m. UTC | #2
Paul B Mahol:
> On Mon, Sep 14, 2020 at 07:27:25AM +0200, Andreas Rheinhardt wrote:
>> The init function of the ALAC encoder calls its own close function
>> if a call to ff_lpc_init() fails; yet nothing has been allocated before
>> that point (except extradata which is freed generically) and ff_lpc_init()
>> can be expected to clean up after itself on error (the documentation does
>> not say anything to the contrary and the current implementation can only
>> fail if the only allocation fails, so there is nothing to clean up on
>> error anyway), so this is unnecessary.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>>  libavcodec/alacenc.c | 20 ++++++--------------
>>  1 file changed, 6 insertions(+), 14 deletions(-)
>>
> 
> what about adding cleanup flag so if new stuff is added(unlikely) new bugs
> wont pop up?
> 

That is possible, but as you said yourself: It is unlikely that new
stuff will be added. Furthermore, I think that whoever adds new stuff
would be responsible to make sure that it is free of leaks.

- Andreas
diff mbox series

Patch

diff --git a/libavcodec/alacenc.c b/libavcodec/alacenc.c
index fc5fa270e6..9d135d1350 100644
--- a/libavcodec/alacenc.c
+++ b/libavcodec/alacenc.c
@@ -535,10 +535,8 @@  static av_cold int alac_encode_init(AVCodecContext *avctx)
                                                  avctx->bits_per_raw_sample);
 
     avctx->extradata = av_mallocz(ALAC_EXTRADATA_SIZE + AV_INPUT_BUFFER_PADDING_SIZE);
-    if (!avctx->extradata) {
-        ret = AVERROR(ENOMEM);
-        goto error;
-    }
+    if (!avctx->extradata)
+        return AVERROR(ENOMEM);
     avctx->extradata_size = ALAC_EXTRADATA_SIZE;
 
     alac_extradata = avctx->extradata;
@@ -566,8 +564,7 @@  FF_DISABLE_DEPRECATION_WARNINGS
            avctx->min_prediction_order > ALAC_MAX_LPC_ORDER) {
             av_log(avctx, AV_LOG_ERROR, "invalid min prediction order: %d\n",
                    avctx->min_prediction_order);
-            ret = AVERROR(EINVAL);
-            goto error;
+            return AVERROR(EINVAL);
         }
 
         s->min_prediction_order = avctx->min_prediction_order;
@@ -578,8 +575,7 @@  FF_DISABLE_DEPRECATION_WARNINGS
             avctx->max_prediction_order > ALAC_MAX_LPC_ORDER) {
             av_log(avctx, AV_LOG_ERROR, "invalid max prediction order: %d\n",
                    avctx->max_prediction_order);
-            ret = AVERROR(EINVAL);
-            goto error;
+            return AVERROR(EINVAL);
         }
 
         s->max_prediction_order = avctx->max_prediction_order;
@@ -591,8 +587,7 @@  FF_ENABLE_DEPRECATION_WARNINGS
         av_log(avctx, AV_LOG_ERROR,
                "invalid prediction orders: min=%d max=%d\n",
                s->min_prediction_order, s->max_prediction_order);
-        ret = AVERROR(EINVAL);
-        goto error;
+        return AVERROR(EINVAL);
     }
 
     s->avctx = avctx;
@@ -600,13 +595,10 @@  FF_ENABLE_DEPRECATION_WARNINGS
     if ((ret = ff_lpc_init(&s->lpc_ctx, avctx->frame_size,
                            s->max_prediction_order,
                            FF_LPC_TYPE_LEVINSON)) < 0) {
-        goto error;
+        return ret;
     }
 
     return 0;
-error:
-    alac_encode_close(avctx);
-    return ret;
 }
 
 static int alac_encode_frame(AVCodecContext *avctx, AVPacket *avpkt,