[FFmpeg-devel] lavu: make av_get_media_type_string() never return NULL

printf %s with a null pointer is undefined behavior
---
 libavutil/avutil.h | 3 +--
 libavutil/utils.c  | 3 ++-
 2 files changed, 3 insertions(+), 3 deletions(-)

On 2/1/2022 7:30 PM, Scott Theisen wrote:
> printf %s with a null pointer is undefined behavior
> ---
>   libavutil/avutil.h | 3 +--
>   libavutil/utils.c  | 3 ++-
>   2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/libavutil/avutil.h b/libavutil/avutil.h
> index 4d633156d1..4bd468d72f 100644
> --- a/libavutil/avutil.h
> +++ b/libavutil/avutil.h
> @@ -207,8 +207,7 @@ enum AVMediaType {
>   };
>   
>   /**
> - * Return a string describing the media_type enum, NULL if media_type
> - * is unknown.
> + * Return a string describing the media_type enum, never NULL.

This is an API breakage, so it's not ok.

The doxy states it returns NULL if media_type is unknown, so you're 
expected to check the returned pointer before trying to use it.

>    */
>   const char *av_get_media_type_string(enum AVMediaType media_type);
>   
> diff --git a/libavutil/utils.c b/libavutil/utils.c
> index ea9b5097b8..c85d7abace 100644
> --- a/libavutil/utils.c
> +++ b/libavutil/utils.c
> @@ -71,12 +71,13 @@ const char *avutil_license(void)
>   const char *av_get_media_type_string(enum AVMediaType media_type)
>   {
>       switch (media_type) {
> +    case AVMEDIA_TYPE_UNKNOWN:    return "unknown";
>       case AVMEDIA_TYPE_VIDEO:      return "video";
>       case AVMEDIA_TYPE_AUDIO:      return "audio";
>       case AVMEDIA_TYPE_DATA:       return "data";
>       case AVMEDIA_TYPE_SUBTITLE:   return "subtitle";
>       case AVMEDIA_TYPE_ATTACHMENT: return "attachment";
> -    default:                      return NULL;
> +    default:                      return "invalid";
>       }
>   }
>

On 2/1/22 17:34, James Almer wrote:
> This is an API breakage, so it's not ok. 
I'm new, so would this require a bump to the MINOR or MICRO version number?

> The doxy states it returns NULL if media_type is unknown, so you're 
> expected to check the returned pointer before trying to use it. 

git grep -E --count "av_get_media_type_string"

doc/APIchanges:1
doc/examples/demuxing_decoding.c:5
doc/examples/extract_mvs.c:2
fftools/cmdutils.c:1
fftools/cmdutils.h:1
fftools/ffmpeg.c:5
fftools/ffmpeg_opt.c:1
fftools/ffplay.c:2
fftools/ffprobe.c:3
libavcodec/avcodec.c:1
libavcodec/tests/avcodec.c:1
libavdevice/dshow.c:2
libavfilter/avfilter.c:2
libavfilter/avfiltergraph.c:2
libavfilter/src_movie.c:3
libavformat/avienc.c:1
libavformat/flvenc.c:1
libavformat/framehash.c:1
libavformat/mov.c:1
libavformat/mpegenc.c:1
libavformat/mpegts.c:1
libavformat/mxfdec.c:1
libavformat/segment.c:1
libavformat/tee.c:1
libavformat/uncodedframecrcenc.c:1
libavutil/avutil.h:1
libavutil/utils.c:1

44 total - 1 non-code - 1 prototype - 1 definition + 4 via macro - 1 
macro = 44 uses

The following uses of av_get_media_type_string() do not check for null:
doc/examples/demuxing_decoding.c: 5
doc/examples/extract_mvs.c: 2
fftools/cmdutils.c: 2 via the define in cmdutils.h
fftools/ffmpeg.c: 7, 2 via the define in cmdutils.h
fftools/ffmpeg_opt.c:1
fftools/ffplay.c:2
libavfilter/avfiltergraph.c:2
libavfilter/src_movie.c:3
libavformat/flvenc.c:1
libavformat/framehash.c:1
libavformat/mov.c:1
libavformat/mpegenc.c:1
libavformat/mpegts.c:1
libavformat/mxfdec.c:1
libavformat/segment.c:1
libavformat/tee.c:1

32/44 uses do not check for null.


libavcodec/tests/avcodec.c: this use is a workaround to 
av_get_media_type_string() returning null; checked 3 times


Let me know which version number to bump and I will submit a new version 
that also removes the now redundant null checks from the rest of the code.

Thanks,
Scott

On 2/1/2022 10:58 PM, Scott Theisen wrote:
> On 2/1/22 17:34, James Almer wrote:
>> This is an API breakage, so it's not ok. 
> I'm new, so would this require a bump to the MINOR or MICRO version number?

Making a behavior change like this requires a warning and a period to 
let library users adapt their code. You can't break it just like that 
with no warning at all.

> 
>> The doxy states it returns NULL if media_type is unknown, so you're 
>> expected to check the returned pointer before trying to use it. 
> 
> git grep -E --count "av_get_media_type_string"
> 
> doc/APIchanges:1
> doc/examples/demuxing_decoding.c:5
> doc/examples/extract_mvs.c:2
> fftools/cmdutils.c:1
> fftools/cmdutils.h:1
> fftools/ffmpeg.c:5
> fftools/ffmpeg_opt.c:1
> fftools/ffplay.c:2
> fftools/ffprobe.c:3
> libavcodec/avcodec.c:1
> libavcodec/tests/avcodec.c:1
> libavdevice/dshow.c:2
> libavfilter/avfilter.c:2
> libavfilter/avfiltergraph.c:2
> libavfilter/src_movie.c:3
> libavformat/avienc.c:1
> libavformat/flvenc.c:1
> libavformat/framehash.c:1
> libavformat/mov.c:1
> libavformat/mpegenc.c:1
> libavformat/mpegts.c:1
> libavformat/mxfdec.c:1
> libavformat/segment.c:1
> libavformat/tee.c:1
> libavformat/uncodedframecrcenc.c:1
> libavutil/avutil.h:1
> libavutil/utils.c:1
> 
> 44 total - 1 non-code - 1 prototype - 1 definition + 4 via macro - 1 
> macro = 44 uses
> 
> The following uses of av_get_media_type_string() do not check for null:
> doc/examples/demuxing_decoding.c: 5
> doc/examples/extract_mvs.c: 2
> fftools/cmdutils.c: 2 via the define in cmdutils.h
> fftools/ffmpeg.c: 7, 2 via the define in cmdutils.h
> fftools/ffmpeg_opt.c:1
> fftools/ffplay.c:2
> libavfilter/avfiltergraph.c:2
> libavfilter/src_movie.c:3
> libavformat/flvenc.c:1
> libavformat/framehash.c:1
> libavformat/mov.c:1
> libavformat/mpegenc.c:1
> libavformat/mpegts.c:1
> libavformat/mxfdec.c:1
> libavformat/segment.c:1
> libavformat/tee.c:1
> 
> 32/44 uses do not check for null.

But do they need to? Those modules have probably ensured media type is 
one of the known and supported ones by the time they call this function.

Why do you need this function to never return NULL, anyway? If you know 
any of these calls where media type can be UNKNOWN, then it's easier to 
just fix them by checking for NULL as per the doxy.

> 
> 
> libavcodec/tests/avcodec.c: this use is a workaround to 
> av_get_media_type_string() returning null; checked 3 times
> 
> 
> Let me know which version number to bump and I will submit a new version 
> that also removes the now redundant null checks from the rest of the code.
> 
> Thanks,
> Scott
> _______________________________________________
> 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".

Scott Theisen:
> On 2/1/22 17:34, James Almer wrote:
>> This is an API breakage, so it's not ok. 
> I'm new, so would this require a bump to the MINOR or MICRO version number?
> 

That would need an announcement in doc/APIchanges; it can then be
changed at the next major version bump after two years have passed.

>> The doxy states it returns NULL if media_type is unknown, so you're
>> expected to check the returned pointer before trying to use it. 
> 
> git grep -E --count "av_get_media_type_string"
> 
> doc/APIchanges:1
> doc/examples/demuxing_decoding.c:5
> doc/examples/extract_mvs.c:2
> fftools/cmdutils.c:1
> fftools/cmdutils.h:1
> fftools/ffmpeg.c:5
> fftools/ffmpeg_opt.c:1
> fftools/ffplay.c:2
> fftools/ffprobe.c:3
> libavcodec/avcodec.c:1
> libavcodec/tests/avcodec.c:1
> libavdevice/dshow.c:2
> libavfilter/avfilter.c:2
> libavfilter/avfiltergraph.c:2
> libavfilter/src_movie.c:3
> libavformat/avienc.c:1
> libavformat/flvenc.c:1
> libavformat/framehash.c:1
> libavformat/mov.c:1
> libavformat/mpegenc.c:1
> libavformat/mpegts.c:1
> libavformat/mxfdec.c:1
> libavformat/segment.c:1
> libavformat/tee.c:1
> libavformat/uncodedframecrcenc.c:1
> libavutil/avutil.h:1
> libavutil/utils.c:1
> 
> 44 total - 1 non-code - 1 prototype - 1 definition + 4 via macro - 1
> macro = 44 uses
> 
> The following uses of av_get_media_type_string() do not check for null:
> doc/examples/demuxing_decoding.c: 5
> doc/examples/extract_mvs.c: 2
> fftools/cmdutils.c: 2 via the define in cmdutils.h
> fftools/ffmpeg.c: 7, 2 via the define in cmdutils.h
> fftools/ffmpeg_opt.c:1
> fftools/ffplay.c:2
> libavfilter/avfiltergraph.c:2
> libavfilter/src_movie.c:3
> libavformat/flvenc.c:1
> libavformat/framehash.c:1
> libavformat/mov.c:1
> libavformat/mpegenc.c:1
> libavformat/mpegts.c:1
> libavformat/mxfdec.c:1
> libavformat/segment.c:1
> libavformat/tee.c:1
> 
> 32/44 uses do not check for null.

Who sets invalid media types?
(In case of fftools/*: If they don't set it themselves, they (and all
our users) should be able to rely on our libraries to not set invalid
values. The same goes for all demuxers (as they set this value
themselves). Checks are only necessary where the media type comes from
the user; this means muxers (where the check should be done generically)
and possibly avfiltergraph.c (I am pretty sure that the type has already
been checked earlier for filters).)

- Andreas

On 2/1/22 21:06, James Almer wrote:
>> 32/44 uses do not check for null.
>
> But do they need to? Those modules have probably ensured media type is 
> one of the known and supported ones by the time they call this function.
>
> Why do you need this function to never return NULL, anyway? If you 
> know any of these calls where media type can be UNKNOWN, then it's 
> easier to just fix them by checking for NULL as per the doxy. 
The impetus was the desire to use this function in MythTV without 
invoking undefined behavior.  Currently MythTV uses a modification to 
ffmpeg: 
https://github.com/MythTV/mythtv/blob/7fa02d4dc3ab0d3f2cbfcbc514047fe2736fe9cf/mythtv/external/FFmpeg/libavcodec/utils-mythtv.c#L228
(For reference, I just tested and a NULL/nullptr const char* becomes an 
empty QString, but this is not documented.)

Also, another reason was to match behavior with avcodec_get_name() which 
never returns NULL, instead it returns "unknown_codec".

On 2/1/22 21:13, Andreas Rheinhardt wrote:
> That would need an announcement in doc/APIchanges; it can then be
> changed at the next major version bump after two years have passed.
So, would creating a new function instead and deprecating the old one be 
a better option?  (e.g. av_get_media_type_name)
> Who sets invalid media types?
> (In case of fftools/*: If they don't set it themselves, they (and all
> our users) should be able to rely on our libraries to not set invalid
> values. The same goes for all demuxers (as they set this value
> themselves). Checks are only necessary where the media type comes from
> the user; this means muxers (where the check should be done generically)
> and possibly avfiltergraph.c (I am pretty sure that the type has already
> been checked earlier for filters).)
I agree invalid media types are probably not set by anyone, but without 
checking for non-NULL it potentially invokes undefined behavior.

Returning "unknown" for AVMEDIA_TYPE_UNKNOWN and returning a different 
value for the default case is, in my opinion clearer. 
"invalid_media_type" may be a better default string to match the 
behavior of avcodec_get_name().

Regards,
Scott

Instead of making an API breaking change, create a new function instead,
and deprecate the old one.

Then replace all uses in FFmpeg of the old function with the new one.

Identical to v2, but I reworded the second commit's message to add a context.

Quoting Andreas Rheinhardt (2022-02-02 03:13:12)
> Who sets invalid media types?
> (In case of fftools/*: If they don't set it themselves, they (and all
> our users) should be able to rely on our libraries to not set invalid
> values. The same goes for all demuxers (as they set this value
> themselves). Checks are only necessary where the media type comes from
> the user; this means muxers (where the check should be done generically)
> and possibly avfiltergraph.c (I am pretty sure that the type has already
> been checked earlier for filters).)

Some demuxer code in lavf looks like it might result in a TYPE_UNKNOWN
AVStream. E.g. argo_brp, avi, both asfdecs, gxf.

On 2/23/22 02:52, Anton Khirnov wrote:
> Quoting Andreas Rheinhardt (2022-02-02 03:13:12)
>> Who sets invalid media types?
>> (In case of fftools/*: If they don't set it themselves, they (and all
>> our users) should be able to rely on our libraries to not set invalid
>> values. The same goes for all demuxers (as they set this value
>> themselves). Checks are only necessary where the media type comes from
>> the user; this means muxers (where the check should be done generically)
>> and possibly avfiltergraph.c (I am pretty sure that the type has already
>> been checked earlier for filters).)
> Some demuxer code in lavf looks like it might result in a TYPE_UNKNOWN
> AVStream. E.g. argo_brp, avi, both asfdecs, gxf.

Also, in libavcodec/codec_desc.c, avcodec_get_type(AV_CODEC_ID_NONE) 
will return AVMEDIA_TYPE_UNKNOWN.  The mpegts demuxer, for example, will 
set (or more likely leave) AV_CODEC_ID_NONE as the AVCodecID of a stream 
if it can't identify the codec.  This is probably true of all demuxers.

-Scott Theisen