[FFmpeg-devel] tools/coverity: override av_dict_set

Coverity thinks av_dict_set frees the key and value parameter, because
it has the (rarely used) option to do so, and it's not smart enough to
figure out it depends on the flags parameter.
So lets provide a custom implementation that does not free them.
---
 tools/coverity.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Timo Rothenpieler:
> Coverity thinks av_dict_set frees the key and value parameter, because
> it has the (rarely used) option to do so, and it's not smart enough to
> figure out it depends on the flags parameter.
> So lets provide a custom implementation that does not free them.
> ---
>  tools/coverity.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/tools/coverity.c b/tools/coverity.c
> index 19a132a976..5e4eb19f39 100644
> --- a/tools/coverity.c
> +++ b/tools/coverity.c
> @@ -77,3 +77,12 @@ void *av_free(void *ptr) {
>      __coverity_mark_as_afm_freed__(ptr, "av_free");
>  }
>  
> +int av_dict_set(void **pm, const char *key, const char *value, int flags) {
> +    int has_memory;
> +    if (has_memory) {
> +        return 0;
> +    } else {
> +        return -1;
> +    }
> +}
> +
> 
1. Won't this lead to a use-of-uninitialized-value warning?
2. This is a trade-off, isn't it? Coverity will then think that all the
cases where the AV_DICT_DONT_STRDUP_* flags are used lead to leaks,
won't it? If so, this shoul be mentioned in the commit message.
3. Furthermore, this model does not actually set anything: pm is
unchanged. Given that av_dict_set is the only function that actually
allocates an AVDictionary, Coverity might infer that lots of
AVDictionary * are in fact NULL; this might lead it to think that
certain code is dead although it isn't. Is there a way we can just use
the ordinary implementation of av_dict_set, but without the
AV_DICT_DONT_STRDUP_* flags?

- Andreas

On 11.07.2020 22:17, Andreas Rheinhardt wrote:
> Timo Rothenpieler:
>> Coverity thinks av_dict_set frees the key and value parameter, because
>> it has the (rarely used) option to do so, and it's not smart enough to
>> figure out it depends on the flags parameter.
>> So lets provide a custom implementation that does not free them.
>> ---
>>   tools/coverity.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/tools/coverity.c b/tools/coverity.c
>> index 19a132a976..5e4eb19f39 100644
>> --- a/tools/coverity.c
>> +++ b/tools/coverity.c
>> @@ -77,3 +77,12 @@ void *av_free(void *ptr) {
>>       __coverity_mark_as_afm_freed__(ptr, "av_free");
>>   }
>>   
>> +int av_dict_set(void **pm, const char *key, const char *value, int flags) {
>> +    int has_memory;
>> +    if (has_memory) {
>> +        return 0;
>> +    } else {
>> +        return -1;
>> +    }
>> +}
>> +
>>
> 1. Won't this lead to a use-of-uninitialized-value warning?

No, since this is never actually compiled, but given to coverity 
(manually) as hints file.
You just need to make branch points for it to analyze.

> 2. This is a trade-off, isn't it? Coverity will then think that all the
> cases where the AV_DICT_DONT_STRDUP_* flags are used lead to leaks,
> won't it? If so, this shoul be mentioned in the commit message.

Yeah, but those seem to be rarely used, while the case where it's 0 and 
causes errors is happening a whole lot.

> 3. Furthermore, this model does not actually set anything: pm is
> unchanged. Given that av_dict_set is the only function that actually
> allocates an AVDictionary, Coverity might infer that lots of
> AVDictionary * are in fact NULL; this might lead it to think that
> certain code is dead although it isn't. Is there a way we can just use
> the ordinary implementation of av_dict_set, but without the
> AV_DICT_DONT_STRDUP_* flags?

Not easily, since one cannot include other files here, and only use very 
basic features that coverity can understand.

Yeah, some more side effects will probably have to be built in, but 
still, given that coverity never actually runs the code, and only runs 
static analysis on it, a lot of normally nonsensical shortcuts can be made.


> - Andreas
> _______________________________________________
> 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".
>