| Message ID | 20200228095321.7222-3-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,v2,1/7] avformat/hlsenc: Avoid setting unused variables | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
> 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
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,
> 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
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,
> 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
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
> 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
> 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
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,
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".
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,
Steven Liu: > > >> 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. > 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. (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?
> 2020年4月9日 下午8:58，Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道： > > Steven Liu: >> >> >>> 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. >> > 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.) > _______________________________________________ > 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
Steven Liu: > > >> 2020年4月9日 下午8:58，Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道： >> >> Steven Liu: >>> >>> >>>> 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. >>> >> 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
> 2020年5月8日 下午7:07，Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道： > > Steven Liu: >> >> >>> 2020年4月9日 下午8:58，Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道： >>> >>> Steven Liu: >>>> >>>> >>>>> 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. >>>> >>> 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. of course, you can apply them ;) > > - 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". Thanks Steven Liu
Steven Liu: > > >> 2020年5月8日 下午7:07，Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道： >> >> Steven Liu: >>> >>> >>>> 2020年4月9日 下午8:58，Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道： >>>> >>>> Steven Liu: >>>>> >>>>> >>>>>> 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. >>>>> >>>> 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. > of course, you can apply them ;) Applied. Thanks. - Andreas
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); }
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavformat/hlsenc.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)