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

[Andreas Rheinhardt]

Steven Liu:
> 
> 
>> 2020年4月9日 下午8:58，Andreas Rheinhardt <andreas.rheinhardt at gmail.com> 写道：
>>
>> Steven Liu:
>>>
>>>
>>>> 2020年4月9日 下午5:55，Steven Liu <lq at chinaffmpeg.org> 写道：
>>>>
>>>>
>>>>
>>>>> 2020年4月9日 下午5:48，Andreas Rheinhardt <andreas.rheinhardt at gmail.com> 写道：
>>>>>
>>>>> Steven Liu:
>>>>>>
>>>>>>
>>>>>>> 2020年2月28日 下午5:53，Andreas Rheinhardt <andreas.rheinhardt at gmail.com> 写道：
>>>>>>>
>>>>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at 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.
>>>
>> Command line:
>> ffmpeg -i <Input file> -c copy -t 1:30 -f hls -hls_time 2
>> -hls_playlist_type event -master_pl_name index.m3u8
>> -hls_segment_filename stream_%v/data%06d.ts   -use_localtime_mkdir 1
>> -var_stream_map 'v:0,a:0' test/test%v/stream.m3u8
>> (My input file simply had H.264 video and AC-3 audio, but I don't think
>> that is important.)
>>
>> If I run this normally, I get two subfolders in the current directory:
>> stream_0 and test. stream_0 contains various transport streams, test
>> contains index.m3u8 and a folder test0 which contains stream.m3u8.
>>
>> If I simulate memory allocation failure by replacing the first strdup in
>> update_master_pl_info() with 'fn1 = NULL;' (instead of 'fn1 =
>> av_strdup(s->url);'), then the index.m3u8 file is not created in the
>> test subfolder, but in the current directory.
> Yes, you are right, I have not reproduced by this way, I double checked this way, it really have the problem,
> 
>> (If you believed that av_strdup() failures don't affect the outcome,
>> then you could have replaced all 'temp = av_strdup(str); dir =
>> av_dirname(temp);' with 'dir = ".";'.)
>>
>> - Andreas
>>
>> PS: What about the rest of this patchset?
> I have no option for other patch, only this one, because i remember Limin Wang submit one patch about this patch and I cannot not reproduced the problem which use the same way above that time, maybe I forget something i not attention. So just asked you at this patch.
> I merge that patch because:
> 1. I cannot reproduce the bug, so have no reason to object it.
> 2. The other part maybe exit if strdup failed. (Real no memory for other part.)

And what about the other patches of this patchset? Do you object to me
applying them? I'll apply them tomorrow if you don't object.

- Andreas