diff mbox series

[FFmpeg-devel,v2,3/7] avformat/hlsenc: Check some unchecked allocations

Message ID 20200228095321.7222-3-andreas.rheinhardt@gmail.com
State New
Headers show
Series [FFmpeg-devel,v2,1/7] avformat/hlsenc: Avoid setting unused variables
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

Andreas Rheinhardt Feb. 28, 2020, 9:53 a.m. UTC
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavformat/hlsenc.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Comments

Steven Liu April 8, 2020, 11:58 p.m. UTC | #1
> 2020年2月28日 下午5:53,Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道:
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> libavformat/hlsenc.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> index a281c379f0..18f40ff3ed 100644
> --- a/libavformat/hlsenc.c
> +++ b/libavformat/hlsenc.c
> @@ -1610,6 +1610,8 @@ static int hls_start(AVFormatContext *s, VariantStream *vs)
>             if (c->use_localtime_mkdir) {
>                 const char *dir;
>                 char *fn_copy = av_strdup(oc->url);
> +                if (!fn_copy)
> +                    return AVERROR(ENOMEM);
>                 dir = av_dirname(fn_copy);
>                 if (ff_mkdir_p(dir) == -1 && errno != EEXIST) {
>                     av_log(oc, AV_LOG_ERROR, "Could not create directory %s with use_localtime_mkdir\n", dir);
> @@ -1770,6 +1772,8 @@ static int validate_name(int nb_vs, const char *fn)
>     }
> 
>     fn_dup = av_strdup(fn);
> +    if (!fn_dup)
> +        return AVERROR(ENOMEM);
>     filename = av_basename(fn);
>     subdir_name = av_dirname(fn_dup);
> 
> @@ -2139,6 +2143,8 @@ static int update_master_pl_info(AVFormatContext *s)
>     int ret = 0;
> 
>     fn1 = av_strdup(s->url);
> +    if (!fn1)
> +        return AVERROR(ENOMEM);
It’s unnecessary here,
I have checked all strdup return checker in hlsenc some month ago, and double check the workflow in update_master_pl_info,
It's the safe whether you check the strdup or not.
Reference commit id: 61aa77272a25d83e5ce5c63d93c64bb9a3e15557
>     dir = av_dirname(fn1);
> 
>     /**
> @@ -2147,6 +2153,10 @@ static int update_master_pl_info(AVFormatContext *s)
>      */
>     if (dir && av_stristr(av_basename(dir), "%v")) {
>         fn2 = av_strdup(dir);
> +        if (!fn2) {
> +            ret = AVERROR(ENOMEM);
> +            goto fail;
> +        }
Ditto.
>         dir = av_dirname(fn2);
>     }
> 
> @@ -2854,7 +2864,8 @@ static int hls_init(AVFormatContext *s)
>                 if (hls->nb_varstreams > 1) {
>                     if (av_stristr(vs->fmp4_init_filename, "%v")) {
>                         av_freep(&vs->fmp4_init_filename);
> -                        format_name(hls->fmp4_init_filename, &vs->fmp4_init_filename, i, vs->varname);
> +                        ret = format_name(hls->fmp4_init_filename,
> +                                          &vs->fmp4_init_filename, i, vs->varname);
>                     } else {
>                         ret = append_postfix(vs->fmp4_init_filename, fmp4_init_filename_len, i);
>                     }
> -- 
> 2.20.1
> 
> _______________________________________________
> 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”.


Thanks

Steven Liu
Nicolas George April 9, 2020, 8:49 a.m. UTC | #2
Steven Liu (12020-04-09):
> It’s unnecessary here,
> I have checked all strdup return checker in hlsenc some month ago, and double check the workflow in update_master_pl_info,
> It's the safe whether you check the strdup or not.
> Reference commit id: 61aa77272a25d83e5ce5c63d93c64bb9a3e15557

Honestly, I think that relying on string functions to all check for NULL
to guard against a memory allocation failure is very terrible design.
More importantly, it is fragile: it takes only one string function that
does not support NULL to make it crash. And since it is neither obvious
nor documented in the code (I am not talking about the doxy of the
string functions, I am talking about the fact that it relies on it), you
will endlessly get patches like that to restore them. And if somebody
decides to copy the code but is not careful about the string function it
uses, again, crash.

I vote for properly checking memory allocations as they happen.

Regards,
Steven Liu April 9, 2020, 9:07 a.m. UTC | #3
> 2020年4月9日 下午4:49,Nicolas George <george@nsup.org> 写道:
> 
> Steven Liu (12020-04-09):
>> It’s unnecessary here,
>> I have checked all strdup return checker in hlsenc some month ago, and double check the workflow in update_master_pl_info,
>> It's the safe whether you check the strdup or not.
>> Reference commit id: 61aa77272a25d83e5ce5c63d93c64bb9a3e15557
> 
> Honestly, I think that relying on string functions to all check for NULL
> to guard against a memory allocation failure is very terrible design.
> More importantly, it is fragile: it takes only one string function that
> does not support NULL to make it crash. And since it is neither obvious
> nor documented in the code (I am not talking about the doxy of the
> string functions, I am talking about the fact that it relies on it), you
> will endlessly get patches like that to restore them. And if somebody
> decides to copy the code but is not careful about the string function it
> uses, again, crash.
> 
> I vote for properly checking memory allocations as they happen.
I think every body have themselves reason to checking memory and remove the checking at here.
Whatever for me you can add them here for somebody copy, other body remove them 
some days(maybe long time later) some body remove remove the for unnecessary processing.

Thanks

Steven Liu
Nicolas George April 9, 2020, 9:33 a.m. UTC | #4
Steven Liu (12020-04-09):
> I think every body have themselves reason to checking memory and remove the checking at here.
> Whatever for me you can add them here for somebody copy, other body remove them 
> some days(maybe long time later) some body remove remove the for unnecessary processing.

I don't understand what you are trying to say.

The removal of the checks was done at the same time the string functions
were twisted to make them accept NULL. Nobody would propose to re-remove
them unless they are very familiar with the code.

As it is, the code path for the check has been made to start in
libavformat, make a detour through libavutil and come back to
libavformat. This is terrible.

Let us re-add proper, clear checks where they belong.

Regards,
Steven Liu April 9, 2020, 9:47 a.m. UTC | #5
> 2020年4月9日 下午5:33,Nicolas George <george@nsup.org> 写道:
> 
> Steven Liu (12020-04-09):
>> I think every body have themselves reason to checking memory and remove the checking at here.
>> Whatever for me you can add them here for somebody copy, other body remove them 
>> some days(maybe long time later) some body remove remove the for unnecessary processing.
> 
> I don't understand what you are trying to say.
I said I don’t care about it here, because it don’t  is not affected the workflow, you can merge it if it can make you happy.
I have no reason objection it, and no reason agreed it.:D
> 
> The removal of the checks was done at the same time the string functions
> were twisted to make them accept NULL. Nobody would propose to re-remove
> them unless they are very familiar with the code.
> 
> As it is, the code path for the check has been made to start in
> libavformat, make a detour through libavutil and come back to
> libavformat. This is terrible.
> 
> Let us re-add proper, clear checks where they belong.
> 
> Regards,
> 
> -- 
>  Nicolas George

Thanks

Steven Liu
Andreas Rheinhardt April 9, 2020, 9:48 a.m. UTC | #6
Steven Liu:
> 
> 
>> 2020年2月28日 下午5:53,Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道:
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>> libavformat/hlsenc.c | 13 ++++++++++++-
>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
>> index a281c379f0..18f40ff3ed 100644
>> --- a/libavformat/hlsenc.c
>> +++ b/libavformat/hlsenc.c
>> @@ -1610,6 +1610,8 @@ static int hls_start(AVFormatContext *s, VariantStream *vs)
>>             if (c->use_localtime_mkdir) {
>>                 const char *dir;
>>                 char *fn_copy = av_strdup(oc->url);
>> +                if (!fn_copy)
>> +                    return AVERROR(ENOMEM);
>>                 dir = av_dirname(fn_copy);
>>                 if (ff_mkdir_p(dir) == -1 && errno != EEXIST) {
>>                     av_log(oc, AV_LOG_ERROR, "Could not create directory %s with use_localtime_mkdir\n", dir);
>> @@ -1770,6 +1772,8 @@ static int validate_name(int nb_vs, const char *fn)
>>     }
>>
>>     fn_dup = av_strdup(fn);
>> +    if (!fn_dup)
>> +        return AVERROR(ENOMEM);
>>     filename = av_basename(fn);
>>     subdir_name = av_dirname(fn_dup);
>>
>> @@ -2139,6 +2143,8 @@ static int update_master_pl_info(AVFormatContext *s)
>>     int ret = 0;
>>
>>     fn1 = av_strdup(s->url);
>> +    if (!fn1)
>> +        return AVERROR(ENOMEM);
> It’s unnecessary here,
> I have checked all strdup return checker in hlsenc some month ago, and double check the workflow in update_master_pl_info,
> It's the safe whether you check the strdup or not.
> Reference commit id: 61aa77272a25d83e5ce5c63d93c64bb9a3e15557

If these strdups fail, the relevant dirnames will be wrong. While you
don't get segfaults, you will not create the files at the right
destinations. We should rather error out instead.

- Andreas
Steven Liu April 9, 2020, 9:55 a.m. UTC | #7
> 2020年4月9日 下午5:48,Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道:
> 
> Steven Liu:
>> 
>> 
>>> 2020年2月28日 下午5:53,Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道:
>>> 
>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>>> ---
>>> libavformat/hlsenc.c | 13 ++++++++++++-
>>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
>>> index a281c379f0..18f40ff3ed 100644
>>> --- a/libavformat/hlsenc.c
>>> +++ b/libavformat/hlsenc.c
>>> @@ -1610,6 +1610,8 @@ static int hls_start(AVFormatContext *s, VariantStream *vs)
>>>            if (c->use_localtime_mkdir) {
>>>                const char *dir;
>>>                char *fn_copy = av_strdup(oc->url);
>>> +                if (!fn_copy)
>>> +                    return AVERROR(ENOMEM);
>>>                dir = av_dirname(fn_copy);
>>>                if (ff_mkdir_p(dir) == -1 && errno != EEXIST) {
>>>                    av_log(oc, AV_LOG_ERROR, "Could not create directory %s with use_localtime_mkdir\n", dir);
>>> @@ -1770,6 +1772,8 @@ static int validate_name(int nb_vs, const char *fn)
>>>    }
>>> 
>>>    fn_dup = av_strdup(fn);
>>> +    if (!fn_dup)
>>> +        return AVERROR(ENOMEM);
>>>    filename = av_basename(fn);
>>>    subdir_name = av_dirname(fn_dup);
>>> 
>>> @@ -2139,6 +2143,8 @@ static int update_master_pl_info(AVFormatContext *s)
>>>    int ret = 0;
>>> 
>>>    fn1 = av_strdup(s->url);
>>> +    if (!fn1)
>>> +        return AVERROR(ENOMEM);
>> It’s unnecessary here,
>> I have checked all strdup return checker in hlsenc some month ago, and double check the workflow in update_master_pl_info,
>> It's the safe whether you check the strdup or not.
>> Reference commit id: 61aa77272a25d83e5ce5c63d93c64bb9a3e15557
> 
> If these strdups fail, the relevant dirnames will be wrong. While you
> don't get segfaults, you will not create the files at the right
> destinations. We should rather error out instead.
> 
Maybe need reproduce the problem it let me understand the exception.
I need one example, command line is ok.


Thanks

Steven Liu
Steven Liu April 9, 2020, 9:58 a.m. UTC | #8
> 2020年4月9日 下午5:55,Steven Liu <lq@chinaffmpeg.org> 写道:
> 
> 
> 
>> 2020年4月9日 下午5:48,Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道:
>> 
>> Steven Liu:
>>> 
>>> 
>>>> 2020年2月28日 下午5:53,Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道:
>>>> 
>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>>>> ---
>>>> libavformat/hlsenc.c | 13 ++++++++++++-
>>>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
>>>> index a281c379f0..18f40ff3ed 100644
>>>> --- a/libavformat/hlsenc.c
>>>> +++ b/libavformat/hlsenc.c
>>>> @@ -1610,6 +1610,8 @@ static int hls_start(AVFormatContext *s, VariantStream *vs)
>>>>           if (c->use_localtime_mkdir) {
>>>>               const char *dir;
>>>>               char *fn_copy = av_strdup(oc->url);
>>>> +                if (!fn_copy)
>>>> +                    return AVERROR(ENOMEM);
>>>>               dir = av_dirname(fn_copy);
>>>>               if (ff_mkdir_p(dir) == -1 && errno != EEXIST) {
>>>>                   av_log(oc, AV_LOG_ERROR, "Could not create directory %s with use_localtime_mkdir\n", dir);
>>>> @@ -1770,6 +1772,8 @@ static int validate_name(int nb_vs, const char *fn)
>>>>   }
>>>> 
>>>>   fn_dup = av_strdup(fn);
>>>> +    if (!fn_dup)
>>>> +        return AVERROR(ENOMEM);
>>>>   filename = av_basename(fn);
>>>>   subdir_name = av_dirname(fn_dup);
>>>> 
>>>> @@ -2139,6 +2143,8 @@ static int update_master_pl_info(AVFormatContext *s)
>>>>   int ret = 0;
>>>> 
>>>>   fn1 = av_strdup(s->url);
>>>> +    if (!fn1)
>>>> +        return AVERROR(ENOMEM);
>>> It’s unnecessary here,
>>> I have checked all strdup return checker in hlsenc some month ago, and double check the workflow in update_master_pl_info,
>>> It's the safe whether you check the strdup or not.
>>> Reference commit id: 61aa77272a25d83e5ce5c63d93c64bb9a3e15557
>> 
>> If these strdups fail, the relevant dirnames will be wrong. While you
>> don't get segfaults, you will not create the files at the right
>> destinations. We should rather error out instead.
>> 
> Maybe need reproduce the problem it let me understand the exception.
> I need one example, command line is ok.
> 
> 
I accept the 61aa77272a2 because i don’t know how to reproduce the exception to objection it. I have no example to objection it.


Thanks

Steven Liu
Nicolas George April 9, 2020, 10:25 a.m. UTC | #9
Steven Liu (12020-04-09):
> I said I don’t care about it here, because it don’t  is not affected
> the workflow, you can merge it if it can make you happy. I have no
> reason objection it, and no reason agreed it.:D

Ok.

Also, please heed reply-to headers instead of Cc-ing anybody randomly.

Regards,
Limin Wang April 9, 2020, 10:42 a.m. UTC | #10
On Thu, Apr 09, 2020 at 11:48:10AM +0200, Andreas Rheinhardt wrote:
> Steven Liu:
> > 
> > 
> >> 2020年2月28日 下午5:53,Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道:
> >>
> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> >> ---
> >> libavformat/hlsenc.c | 13 ++++++++++++-
> >> 1 file changed, 12 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> >> index a281c379f0..18f40ff3ed 100644
> >> --- a/libavformat/hlsenc.c
> >> +++ b/libavformat/hlsenc.c
> >> @@ -1610,6 +1610,8 @@ static int hls_start(AVFormatContext *s, VariantStream *vs)
> >>             if (c->use_localtime_mkdir) {
> >>                 const char *dir;
> >>                 char *fn_copy = av_strdup(oc->url);
> >> +                if (!fn_copy)
> >> +                    return AVERROR(ENOMEM);
> >>                 dir = av_dirname(fn_copy);
> >>                 if (ff_mkdir_p(dir) == -1 && errno != EEXIST) {
> >>                     av_log(oc, AV_LOG_ERROR, "Could not create directory %s with use_localtime_mkdir\n", dir);
> >> @@ -1770,6 +1772,8 @@ static int validate_name(int nb_vs, const char *fn)
> >>     }
> >>
> >>     fn_dup = av_strdup(fn);
> >> +    if (!fn_dup)
> >> +        return AVERROR(ENOMEM);
> >>     filename = av_basename(fn);
> >>     subdir_name = av_dirname(fn_dup);
> >>
> >> @@ -2139,6 +2143,8 @@ static int update_master_pl_info(AVFormatContext *s)
> >>     int ret = 0;
> >>
> >>     fn1 = av_strdup(s->url);
> >> +    if (!fn1)
> >> +        return AVERROR(ENOMEM);
> > It’s unnecessary here,
> > I have checked all strdup return checker in hlsenc some month ago, and double check the workflow in update_master_pl_info,
> > It's the safe whether you check the strdup or not.
> > Reference commit id: 61aa77272a25d83e5ce5c63d93c64bb9a3e15557
> 
> If these strdups fail, the relevant dirnames will be wrong. While you
> don't get segfaults, you will not create the files at the right
> destinations. We should rather error out instead.

Sorry, it seems that I remove these checking when av_dirname claims to support
NULL for path in the API comments like glib dirname function. So I think it's
duplicate check if the function claims to support NULL. If have different
opinions, please add the checking anyway. I don't know why dirname support NULL? 

> 
> - 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".
Nicolas George April 9, 2020, 11:12 a.m. UTC | #11
Limin Wang (12020-04-09):
> Sorry, it seems that I remove these checking when av_dirname claims to support
> NULL for path in the API comments like glib dirname function. So I think it's
> duplicate check if the function claims to support NULL.

"Supporting NULL" can mean anything, and therefore means nothing. You
should have tested that your new code produced the exact same results as
the old code. Apparently, you neglected to do that.

In the future, remember: if you change something non trivial, test it.

> I don't know why dirname support NULL? 

It was a terrible idea: NULL is not a valid file name, and therefore it
makes no sense to take its dir name.

This is "defensive programming": returning random results for invalid
values instead of errorring properly. It is a very bad habit, it leads
to corrupted files and security issues.

Regards,
diff mbox series

Patch

diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
index a281c379f0..18f40ff3ed 100644
--- a/libavformat/hlsenc.c
+++ b/libavformat/hlsenc.c
@@ -1610,6 +1610,8 @@  static int hls_start(AVFormatContext *s, VariantStream *vs)
             if (c->use_localtime_mkdir) {
                 const char *dir;
                 char *fn_copy = av_strdup(oc->url);
+                if (!fn_copy)
+                    return AVERROR(ENOMEM);
                 dir = av_dirname(fn_copy);
                 if (ff_mkdir_p(dir) == -1 && errno != EEXIST) {
                     av_log(oc, AV_LOG_ERROR, "Could not create directory %s with use_localtime_mkdir\n", dir);
@@ -1770,6 +1772,8 @@  static int validate_name(int nb_vs, const char *fn)
     }
 
     fn_dup = av_strdup(fn);
+    if (!fn_dup)
+        return AVERROR(ENOMEM);
     filename = av_basename(fn);
     subdir_name = av_dirname(fn_dup);
 
@@ -2139,6 +2143,8 @@  static int update_master_pl_info(AVFormatContext *s)
     int ret = 0;
 
     fn1 = av_strdup(s->url);
+    if (!fn1)
+        return AVERROR(ENOMEM);
     dir = av_dirname(fn1);
 
     /**
@@ -2147,6 +2153,10 @@  static int update_master_pl_info(AVFormatContext *s)
      */
     if (dir && av_stristr(av_basename(dir), "%v")) {
         fn2 = av_strdup(dir);
+        if (!fn2) {
+            ret = AVERROR(ENOMEM);
+            goto fail;
+        }
         dir = av_dirname(fn2);
     }
 
@@ -2854,7 +2864,8 @@  static int hls_init(AVFormatContext *s)
                 if (hls->nb_varstreams > 1) {
                     if (av_stristr(vs->fmp4_init_filename, "%v")) {
                         av_freep(&vs->fmp4_init_filename);
-                        format_name(hls->fmp4_init_filename, &vs->fmp4_init_filename, i, vs->varname);
+                        ret = format_name(hls->fmp4_init_filename,
+                                          &vs->fmp4_init_filename, i, vs->varname);
                     } else {
                         ret = append_postfix(vs->fmp4_init_filename, fmp4_init_filename_len, i);
                     }