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

[Mark Thompson]

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