diff mbox

[FFmpeg-devel,3/5] trace_headers: Fix memory leaks on syntax read failures

Message ID 20181004230947.9658-3-sw@jkqxz.net
State Accepted
Commit f6912cc3e7569a94159565f553159f6c1b7e0d2c
Headers show

Commit Message

Mark Thompson Oct. 4, 2018, 11:09 p.m. UTC 
---
 libavcodec/trace_headers_bsf.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

Comments

James Almer Oct. 4, 2018, 11:39 p.m. UTC | #1 
On 10/4/2018 8:09 PM, Mark Thompson wrote:
> ---
>  libavcodec/trace_headers_bsf.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/libavcodec/trace_headers_bsf.c b/libavcodec/trace_headers_bsf.c
> index 94a3ef72a2..8322229d4c 100644
> --- a/libavcodec/trace_headers_bsf.c
> +++ b/libavcodec/trace_headers_bsf.c
> @@ -49,15 +49,11 @@ static int trace_headers_init(AVBSFContext *bsf)
>          av_log(bsf, AV_LOG_INFO, "Extradata\n");
>  
>          err = ff_cbs_read_extradata(ctx->cbc, &ps, bsf->par_in);
> -        if (err < 0) {
> -            av_log(bsf, AV_LOG_ERROR, "Failed to read extradata.\n");

Why remove this error message?

> -            return err;
> -        }
>  
>          ff_cbs_fragment_uninit(ctx->cbc, &ps);
>      }
>  
> -    return 0;
> +    return err;
>  }
>  
>  static void trace_headers_close(AVBSFContext *bsf)
> @@ -97,14 +93,12 @@ static int trace_headers(AVBSFContext *bsf, AVPacket *pkt)
>      av_log(bsf, AV_LOG_INFO, "Packet: %d bytes%s.\n", pkt->size, tmp);
>  
>      err = ff_cbs_read_packet(ctx->cbc, &au, pkt);
> -    if (err < 0) {
> -        av_packet_unref(pkt);
> -        return err;
> -    }
>  
>      ff_cbs_fragment_uninit(ctx->cbc, &au);

Maybe the ff_cbs_read* functions should clean whatever they were able to
allocate before the failure by calling this function internally, instead
of leaving it to the caller. It would be more consistent with what other
APIs we offer do.

>  
> -    return 0;
> +    if (err < 0)
> +        av_packet_unref(pkt);
> +    return err;
>  }
>  
>  const AVBitStreamFilter ff_trace_headers_bsf = {

LGTM in any case.
Mark Thompson Oct. 6, 2018, 5:22 p.m. UTC | #2 
On 05/10/18 00:39, James Almer wrote:
> On 10/4/2018 8:09 PM, Mark Thompson wrote:
>> ---
>>  libavcodec/trace_headers_bsf.c | 14 ++++----------
>>  1 file changed, 4 insertions(+), 10 deletions(-)
>>
>> diff --git a/libavcodec/trace_headers_bsf.c b/libavcodec/trace_headers_bsf.c
>> index 94a3ef72a2..8322229d4c 100644
>> --- a/libavcodec/trace_headers_bsf.c
>> +++ b/libavcodec/trace_headers_bsf.c
>> @@ -49,15 +49,11 @@ static int trace_headers_init(AVBSFContext *bsf)
>>          av_log(bsf, AV_LOG_INFO, "Extradata\n");
>>  
>>          err = ff_cbs_read_extradata(ctx->cbc, &ps, bsf->par_in);
>> -        if (err < 0) {
>> -            av_log(bsf, AV_LOG_ERROR, "Failed to read extradata.\n");
> 
> Why remove this error message?

I was thinking it's not adding anything useful - there will already be an error from the reading describing the actual problem.  (Since it makes the BSF init fail there can't be any more from the filter after that point.)

>> -            return err;
>> -        }
>>  
>>          ff_cbs_fragment_uninit(ctx->cbc, &ps);
>>      }
>>  
>> -    return 0;
>> +    return err;
>>  }
>>  
>>  static void trace_headers_close(AVBSFContext *bsf)
>> @@ -97,14 +93,12 @@ static int trace_headers(AVBSFContext *bsf, AVPacket *pkt)
>>      av_log(bsf, AV_LOG_INFO, "Packet: %d bytes%s.\n", pkt->size, tmp);
>>  
>>      err = ff_cbs_read_packet(ctx->cbc, &au, pkt);
>> -    if (err < 0) {
>> -        av_packet_unref(pkt);
>> -        return err;
>> -    }
>>  
>>      ff_cbs_fragment_uninit(ctx->cbc, &au);
> 
> Maybe the ff_cbs_read* functions should clean whatever they were able to
> allocate before the failure by calling this function internally, instead
> of leaving it to the caller. It would be more consistent with what other
> APIs we offer do.

The reasoning for making the API this way was that partial reads are not useless - it might have successfully read a whole sequence of NAL units, then fail on some header error on the last one.

(I admit no current caller makes use of this.)

>>  
>> -    return 0;
>> +    if (err < 0)
>> +        av_packet_unref(pkt);
>> +    return err;
>>  }
>>  
>>  const AVBitStreamFilter ff_trace_headers_bsf = {
> 
> LGTM in any case.

Thanks,

- Mark
diff mbox

Patch

diff --git a/libavcodec/trace_headers_bsf.c b/libavcodec/trace_headers_bsf.c
index 94a3ef72a2..8322229d4c 100644
--- a/libavcodec/trace_headers_bsf.c
+++ b/libavcodec/trace_headers_bsf.c
@@ -49,15 +49,11 @@  static int trace_headers_init(AVBSFContext *bsf)
         av_log(bsf, AV_LOG_INFO, "Extradata\n");
 
         err = ff_cbs_read_extradata(ctx->cbc, &ps, bsf->par_in);
-        if (err < 0) {
-            av_log(bsf, AV_LOG_ERROR, "Failed to read extradata.\n");
-            return err;
-        }
 
         ff_cbs_fragment_uninit(ctx->cbc, &ps);
     }
 
-    return 0;
+    return err;
 }
 
 static void trace_headers_close(AVBSFContext *bsf)
@@ -97,14 +93,12 @@  static int trace_headers(AVBSFContext *bsf, AVPacket *pkt)
     av_log(bsf, AV_LOG_INFO, "Packet: %d bytes%s.\n", pkt->size, tmp);
 
     err = ff_cbs_read_packet(ctx->cbc, &au, pkt);
-    if (err < 0) {
-        av_packet_unref(pkt);
-        return err;
-    }
 
     ff_cbs_fragment_uninit(ctx->cbc, &au);
 
-    return 0;
+    if (err < 0)
+        av_packet_unref(pkt);
+    return err;
 }
 
 const AVBitStreamFilter ff_trace_headers_bsf = {