diff mbox series

[FFmpeg-devel] avformat/protocols: check protocol name before foreach

Message ID 20200131023040.31793-1-lq@chinaffmpeg.org
State New
Headers show
Series [FFmpeg-devel] avformat/protocols: check protocol name before foreach
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Steven Liu Jan. 31, 2020, 2:30 a.m. UTC
Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
---
 libavformat/protocols.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Nicolas George Jan. 31, 2020, 11:13 a.m. UTC | #1
Steven Liu (12020-01-31):
> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> ---
>  libavformat/protocols.c | 2 ++
>  1 file changed, 2 insertions(+)

In what situation is it useful for the caller?

Regards,
Steven Liu Jan. 31, 2020, 12:02 p.m. UTC | #2
> 在 2020年1月31日,下午7:13,Nicolas George <george@nsup.org> 写道:
> 
> Steven Liu (12020-01-31):
>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
>> ---
>> libavformat/protocols.c | 2 ++
>> 1 file changed, 2 insertions(+)
> 
> In what situation is it useful for the caller?
for example: https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=250
> 
> Regards,
> 
> -- 
>  Nicolas George
> _______________________________________________
> 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".

Steven
Thanks
Nicolas George Jan. 31, 2020, 12:07 p.m. UTC | #3
Liu Steven (12020-01-31):
> for example: https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=250

Yes, I saw it shortly afterwards. Seems misguided: same result for a
protocol not found (normal behavior, must be handled) and for a clumsy
programmer. I like it better when the core functions are strict, unless
when there is a significant benefit.

Also, it is abusing an undocumented behavior.

Regards,
Steven Liu Jan. 31, 2020, 12:14 p.m. UTC | #4
> 在 2020年1月31日,下午8:07,Nicolas George <george@nsup.org> 写道:
> 
> Liu Steven (12020-01-31):
>> for example: https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=250
> 
> Yes, I saw it shortly afterwards. Seems misguided: same result for a
> protocol not found (normal behavior, must be handled) and for a clumsy
> programmer. I like it better when the core functions are strict, unless
> when there is a significant benefit.
> 
> Also, it is abusing an undocumented behavior.
Just more safe than without check.
I think if it return -EINVAL maybe better than NULL, is it?
> 
> Regards,
> 
> -- 
>  Nicolas George
> _______________________________________________
> 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".

Steven
Thanks
Steven Liu Jan. 31, 2020, 12:15 p.m. UTC | #5
> 在 2020年1月31日,下午8:14,Liu Steven <lq@chinaffmpeg.org> 写道:
> 
> 
> 
>> 在 2020年1月31日,下午8:07,Nicolas George <george@nsup.org> 写道:
>> 
>> Liu Steven (12020-01-31):
>>> for example: https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=250
>> 
>> Yes, I saw it shortly afterwards. Seems misguided: same result for a
>> protocol not found (normal behavior, must be handled) and for a clumsy
>> programmer. I like it better when the core functions are strict, unless
>> when there is a significant benefit.
>> 
>> Also, it is abusing an undocumented behavior.
> Just more safe than without check.
> I think if it return -EINVAL maybe better than NULL, is it?
Ah, no, it should return AVClass *
>> 
>> Regards,
>> 
>> -- 
>> Nicolas George
>> _______________________________________________
>> 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".
> 
> Steven
> Thanks
> 

Steven
Thanks
Nicolas George Jan. 31, 2020, 3:24 p.m. UTC | #6
Liu Steven (12020-01-31):
> Just more safe than without check.

This is a mistake, a common one: this is not safer, it is less: the
caller has the incorrect assumption that their pointer is not NULL, and
you are letting them keep it, and even in some extents validating it.

> I think if it return -EINVAL maybe better than NULL, is it?

If it was possible (you accurately noticed that not), it would be
actually worse because it introduce a extra case for something that is
not supposed to happen in the first place.

Remember, FFmpeg is programmed in C, not Java or Python: when the
programmers do something stupid, like dividing by 0 or dereferencing
NULL, the program crashes: this is the correct behavior.

Unless there is a useful semantic to give to the NULL case, crashing
immediately is the right behavior. And saying the user "the protocol you
specified does not exist" when the issue is they did not specify a
protocol is not a useful semantic.

Regards,
Steven Liu Jan. 31, 2020, 11:38 p.m. UTC | #7
> 在 2020年1月31日,下午11:24,Nicolas George <george@nsup.org> 写道:
> 
> Liu Steven (12020-01-31):
>> Just more safe than without check.
> 
> This is a mistake, a common one: this is not safer, it is less: the
> caller has the incorrect assumption that their pointer is not NULL, and
> you are letting them keep it, and even in some extents validating it.
> 
>> I think if it return -EINVAL maybe better than NULL, is it?
> 
> If it was possible (you accurately noticed that not), it would be
> actually worse because it introduce a extra case for something that is
> not supposed to happen in the first place.
> 
> Remember, FFmpeg is programmed in C, not Java or Python: when the
> programmers do something stupid, like dividing by 0 or dereferencing
> NULL, the program crashes: this is the correct behavior.
> 
> Unless there is a useful semantic to give to the NULL case, crashing
> immediately is the right behavior. And saying the user "the protocol you
> specified does not exist" when the issue is they did not specify a
> protocol is not a useful semantic.

There have two way for this:
1. make same with other help message, for example:
StevenLiu:dash StevenLiu$ ./ffmpeg -hide_banner -h demuxer
Unknown format '(null)'.
StevenLiu:dash StevenLiu$
StevenLiu:dash StevenLiu$ ./ffmpeg -hide_banner -h muxer
Unknown format '(null)’.

2. i submmit patch to make demuxer mixer help message same as for unified message.
"the demuxer you specified does not exist”
"the muxer you specified does not exist”

two patch to choose, 1st is this patch, 2nd it the new patch https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200131233704.38591-1-lq@chinaffmpeg.org/

> 
> Regards,
> 
> -- 
>  Nicolas George
> _______________________________________________
> 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".

Steven
Thanks
diff mbox series

Patch

diff --git a/libavformat/protocols.c b/libavformat/protocols.c
index 29fb99e7fa..c692342132 100644
--- a/libavformat/protocols.c
+++ b/libavformat/protocols.c
@@ -110,6 +110,8 @@  const char *avio_enum_protocols(void **opaque, int output)
 const AVClass *avio_protocol_get_class(const char *name)
 {
     int i = 0;
+    if (!name)
+        return NULL;
     for (i = 0; url_protocols[i]; i++) {
         if (!strcmp(url_protocols[i]->name, name))
             return url_protocols[i]->priv_data_class;