[FFmpeg-devel] [PATCH V2] avutil/tx: add check against (*ctx)

[James Almer]

On 5/16/2019 5:03 AM, Nicolas George wrote:
> Ruiling Song (12019-05-16):
>> ctx is a pointer to pointer here.
>>
>> Signed-off-by: Ruiling Song <ruiling.song at intel.com>
>> ---
>>  libavutil/tx.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavutil/tx.c b/libavutil/tx.c
>> index 934ef27c81..1690604040 100644
>> --- a/libavutil/tx.c
>> +++ b/libavutil/tx.c
>> @@ -697,7 +697,7 @@ static int gen_mdct_exptab(AVTXContext *s, int len4, double scale)
>>  
>>  av_cold void av_tx_uninit(AVTXContext **ctx)
>>  {
> 
>> -    if (!ctx)
>> +    if (!ctx || !(*ctx))
> 
> That would protect somebody stupid enough to call av_tx_uninit(NULL)
> instead of av_tx_uninit(&var). A hard crass is completely warranted in
> this case. An assert would be acceptable.

An assert is meant to detect developer errors, not user errors. Crashing
the user's whole application because they misused the API is not really
acceptable.

I can't find examples of such functions using asserts this way, but
there are several uninit/free/unref functions behave like the above
patch. See av_buffer_unref(), av_packet_free(), av_bsf_free().
Other functions instead just don't even consider the passed argument
could be NULL at all, like avcodec_free_context() and swr_free(), which
while not 100% safe, is a pretty realistic expectation.

I'd say either apply this patch as is, or apply the original one sent
last night. In both cases it will be following an existing precedent in
the codebase.

> 
>>          return;
>>  
>>      av_free((*ctx)->pfatab);
> 
> Regards,
> 
> 
> _______________________________________________
> 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".
>