| Message ID | 20170425233011.66282-1-lq@chinaffmpeg.org |
|---|---|
| State | Accepted |
| Commit | 363e4f0810d4085bbee3dced41a2de2d2c135dca |
| Headers | show |
2017-04-26 7:30 GMT+08:00 Steven Liu <lq@chinaffmpeg.org>: > fix ticket id: #6353 > > Signed-off-by: Steven Liu <lq@chinaffmpeg.org> > --- > libavformat/hlsenc.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c > index 27c8e3355d..3ec0f330fb 100644 > --- a/libavformat/hlsenc.c > +++ b/libavformat/hlsenc.c > @@ -810,6 +810,7 @@ static int parse_playlist(AVFormatContext *s, const > char *url) > int64_t new_start_pos; > char line[1024]; > const char *ptr; > + const char *end; > > if ((ret = ffio_open_whitelist(&in, url, AVIO_FLAG_READ, > &s->interrupt_callback, NULL, > @@ -842,6 +843,29 @@ static int parse_playlist(AVFormatContext *s, const > char *url) > } else if (av_strstart(line, "#EXTINF:", &ptr)) { > is_segment = 1; > hls->duration = atof(ptr); > + } else if (av_stristart(line, "#EXT-X-KEY:", &ptr)) { > + ptr = av_stristr(line, "URI=\""); > + if (ptr) { > + ptr += strlen("URI=\""); > + end = av_stristr(ptr, ","); > + if (end) { > + av_strlcpy(hls->key_uri, ptr, end - ptr); > + } else { > + av_strlcpy(hls->key_uri, ptr, sizeof(hls->key_uri)); > + } > + } > + > + ptr = av_stristr(line, "IV=0x"); > + if (ptr) { > + ptr += strlen("IV=0x"); > + end = av_stristr(ptr, ","); > + if (end) { > + av_strlcpy(hls->iv_string, ptr, end - ptr); > + } else { > + av_strlcpy(hls->iv_string, ptr, > sizeof(hls->iv_string)); > + } > + } > + > } else if (av_strstart(line, "#", NULL)) { > continue; > } else if (line[0]) { > -- > 2.11.0 (Apple Git-81) > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > Applied! Thanks
On 4/27/2017 7:21 PM, Steven Liu wrote: > 2017-04-26 7:30 GMT+08:00 Steven Liu <lq@chinaffmpeg.org>: > >> fix ticket id: #6353 >> >> Signed-off-by: Steven Liu <lq@chinaffmpeg.org> >> --- >> libavformat/hlsenc.c | 24 ++++++++++++++++++++++++ >> 1 file changed, 24 insertions(+) >> >> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c >> index 27c8e3355d..3ec0f330fb 100644 >> --- a/libavformat/hlsenc.c >> +++ b/libavformat/hlsenc.c >> @@ -810,6 +810,7 @@ static int parse_playlist(AVFormatContext *s, const >> char *url) >> int64_t new_start_pos; >> char line[1024]; >> const char *ptr; >> + const char *end; >> >> if ((ret = ffio_open_whitelist(&in, url, AVIO_FLAG_READ, >> &s->interrupt_callback, NULL, >> @@ -842,6 +843,29 @@ static int parse_playlist(AVFormatContext *s, const >> char *url) >> } else if (av_strstart(line, "#EXTINF:", &ptr)) { >> is_segment = 1; >> hls->duration = atof(ptr); >> + } else if (av_stristart(line, "#EXT-X-KEY:", &ptr)) { >> + ptr = av_stristr(line, "URI=\""); >> + if (ptr) { >> + ptr += strlen("URI=\""); >> + end = av_stristr(ptr, ","); >> + if (end) { >> + av_strlcpy(hls->key_uri, ptr, end - ptr); >> + } else { >> + av_strlcpy(hls->key_uri, ptr, sizeof(hls->key_uri)); I know that this patch has already been applied (although it didn't get any reviews on the ML), but I think that there are some issues with it. First, it seems that both av_strlcpy() cases above will copy the terminating '\"' character into hls->key_uri. Perhaps that won't cause an issue in practice, but it is likely not the intended result. Second, with both av_strlcpy() calls that use a length of end - ptr, this could in theory result in a buffer overrun depending on how long the URI is, since the destination buffers have a fixed size. This issue is prevented in the second call to av_strlcpy(), since it uses sizeof(hls->key_uri), but it is a potential issue with the first calls (note that this comment applies to the IV=0x code below). If you want to use av_strlcpy(), either make sure that end - ptr is less than or equal to sizeof(hls->key_uri) or do something like *end = 0 first and then use av_strlcpy(hls->key_uri, ptr, sizeof(hls->key_uri)). In addition, based on the EXT-X-KEY example at https://developer.apple.com/library/content/technotes/tn2288/_index.html , it would appear that an EXT-X-KEY declaration may span multiple lines. Your solution will not work properly in this case. Aaron Levinson >> + } >> + } >> + >> + ptr = av_stristr(line, "IV=0x"); >> + if (ptr) { >> + ptr += strlen("IV=0x"); >> + end = av_stristr(ptr, ","); >> + if (end) { >> + av_strlcpy(hls->iv_string, ptr, end - ptr); >> + } else { >> + av_strlcpy(hls->iv_string, ptr, >> sizeof(hls->iv_string)); >> + } >> + } >> + >> } else if (av_strstart(line, "#", NULL)) { >> continue; >> } else if (line[0]) { >> -- >> 2.11.0 (Apple Git-81) >> >> > Applied! > > > Thanks
2017-05-03 9:49 GMT+08:00 Aaron Levinson <alevinsn@aracnet.com>: > On 4/27/2017 7:21 PM, Steven Liu wrote: > >> 2017-04-26 7:30 GMT+08:00 Steven Liu <lq@chinaffmpeg.org>: >> >> fix ticket id: #6353 >>> >>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org> >>> --- >>> libavformat/hlsenc.c | 24 ++++++++++++++++++++++++ >>> 1 file changed, 24 insertions(+) >>> >>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c >>> index 27c8e3355d..3ec0f330fb 100644 >>> --- a/libavformat/hlsenc.c >>> +++ b/libavformat/hlsenc.c >>> @@ -810,6 +810,7 @@ static int parse_playlist(AVFormatContext *s, const >>> char *url) >>> int64_t new_start_pos; >>> char line[1024]; >>> const char *ptr; >>> + const char *end; >>> >>> if ((ret = ffio_open_whitelist(&in, url, AVIO_FLAG_READ, >>> &s->interrupt_callback, NULL, >>> @@ -842,6 +843,29 @@ static int parse_playlist(AVFormatContext *s, const >>> char *url) >>> } else if (av_strstart(line, "#EXTINF:", &ptr)) { >>> is_segment = 1; >>> hls->duration = atof(ptr); >>> + } else if (av_stristart(line, "#EXT-X-KEY:", &ptr)) { >>> + ptr = av_stristr(line, "URI=\""); >>> + if (ptr) { >>> + ptr += strlen("URI=\""); >>> + end = av_stristr(ptr, ","); >>> + if (end) { >>> + av_strlcpy(hls->key_uri, ptr, end - ptr); >>> + } else { >>> + av_strlcpy(hls->key_uri, ptr, sizeof(hls->key_uri)); >>> >> > I know that this patch has already been applied (although it didn't get > any reviews on the ML), applied after ticket submiter check ok, and after 2 days. > but I think that there are some issues with it. First, it seems that both > av_strlcpy() cases above will copy the terminating '\"' character into > hls->key_uri. Perhaps that won't cause an issue in practice, but it is > likely not the intended result. Second, with both av_strlcpy() calls that > use a length of end - ptr, this could in theory result in a buffer overrun > depending on how long the URI is, since the destination buffers have a > fixed size. This issue is prevented in the second call to av_strlcpy(), > since it uses sizeof(hls->key_uri), but it is a potential issue with the > first calls (note that this comment applies to the IV=0x code below). If > you want to use av_strlcpy(), either make sure that end - ptr is less than > or equal to sizeof(hls->key_uri) or do something like *end = 0 first and > then use av_strlcpy(hls->key_uri, ptr, sizeof(hls->key_uri)). > > In addition, based on the EXT-X-KEY example at > https://developer.apple.com/library/content/technotes/tn2288/_index.html > , it would appear that an EXT-X-KEY declaration may span multiple lines. > Your solution will not work properly in this case. > i will fix it. Thanks. > > Aaron Levinson > > + } >>> + } >>> + >>> + ptr = av_stristr(line, "IV=0x"); >>> + if (ptr) { >>> + ptr += strlen("IV=0x"); >>> + end = av_stristr(ptr, ","); >>> + if (end) { >>> + av_strlcpy(hls->iv_string, ptr, end - ptr); >>> + } else { >>> + av_strlcpy(hls->iv_string, ptr, >>> sizeof(hls->iv_string)); >>> + } >>> + } >>> + >>> } else if (av_strstart(line, "#", NULL)) { >>> continue; >>> } else if (line[0]) { >>> -- >>> 2.11.0 (Apple Git-81) >>> >>> >>> Applied! >> >> >> Thanks >> > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
2017-05-03 9:49 GMT+08:00 Aaron Levinson <alevinsn@aracnet.com>: > On 4/27/2017 7:21 PM, Steven Liu wrote: > >> 2017-04-26 7:30 GMT+08:00 Steven Liu <lq@chinaffmpeg.org>: >> >> fix ticket id: #6353 >>> >>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org> >>> --- >>> libavformat/hlsenc.c | 24 ++++++++++++++++++++++++ >>> 1 file changed, 24 insertions(+) >>> >>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c >>> index 27c8e3355d..3ec0f330fb 100644 >>> --- a/libavformat/hlsenc.c >>> +++ b/libavformat/hlsenc.c >>> @@ -810,6 +810,7 @@ static int parse_playlist(AVFormatContext *s, const >>> char *url) >>> int64_t new_start_pos; >>> char line[1024]; >>> const char *ptr; >>> + const char *end; >>> >>> if ((ret = ffio_open_whitelist(&in, url, AVIO_FLAG_READ, >>> &s->interrupt_callback, NULL, >>> @@ -842,6 +843,29 @@ static int parse_playlist(AVFormatContext *s, const >>> char *url) >>> } else if (av_strstart(line, "#EXTINF:", &ptr)) { >>> is_segment = 1; >>> hls->duration = atof(ptr); >>> + } else if (av_stristart(line, "#EXT-X-KEY:", &ptr)) { >>> + ptr = av_stristr(line, "URI=\""); >>> + if (ptr) { >>> + ptr += strlen("URI=\""); >>> + end = av_stristr(ptr, ","); >>> + if (end) { >>> + av_strlcpy(hls->key_uri, ptr, end - ptr); >>> + } else { >>> + av_strlcpy(hls->key_uri, ptr, sizeof(hls->key_uri)); >>> >> > I know that this patch has already been applied (although it didn't get > any reviews on the ML), but I think that there are some issues with it. > First, it seems that both av_strlcpy() cases above will copy the > terminating '\"' character into hls->key_uri. Perhaps that won't cause an > issue in practice, but it is likely not the intended result. Second, with > both av_strlcpy() calls that use a length of end - ptr, this could in > theory result in a buffer overrun depending on how long the URI is, since > the destination buffers have a fixed size. This issue is prevented in the > second call to av_strlcpy(), since it uses sizeof(hls->key_uri), but it is > a potential issue with the first calls (note that this comment applies to > the IV=0x code below). If you want to use av_strlcpy(), either make sure > that end - ptr is less than or equal to sizeof(hls->key_uri) or do > something like *end = 0 first and then use av_strlcpy(hls->key_uri, ptr, > sizeof(hls->key_uri)). > > In addition, based on the EXT-X-KEY example at > https://developer.apple.com/library/content/technotes/tn2288/_index.html > , it would appear that an EXT-X-KEY declaration may span multiple lines. > Your solution will not work properly in this case. > Hi Aaron, I think the libavformat/hls.c maybe have the same problem, because both of hlsenc and hls use read_chomp_line to read one line, that need check the '\' tail a line, and read the next line into a buffer, that maybe better, is that right? > > Aaron Levinson > > + } >>> + } >>> + >>> + ptr = av_stristr(line, "IV=0x"); >>> + if (ptr) { >>> + ptr += strlen("IV=0x"); >>> + end = av_stristr(ptr, ","); >>> + if (end) { >>> + av_strlcpy(hls->iv_string, ptr, end - ptr); >>> + } else { >>> + av_strlcpy(hls->iv_string, ptr, >>> sizeof(hls->iv_string)); >>> + } >>> + } >>> + >>> } else if (av_strstart(line, "#", NULL)) { >>> continue; >>> } else if (line[0]) { >>> -- >>> 2.11.0 (Apple Git-81) >>> >>> >>> Applied! >> >> >> Thanks >> > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On 5/4/2017 9:15 PM, Steven Liu wrote: > 2017-05-03 9:49 GMT+08:00 Aaron Levinson <alevinsn@aracnet.com>: > >> On 4/27/2017 7:21 PM, Steven Liu wrote: >> >>> 2017-04-26 7:30 GMT+08:00 Steven Liu <lq@chinaffmpeg.org>: >>> >>> fix ticket id: #6353 >>>> >>>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org> >>>> --- >>>> libavformat/hlsenc.c | 24 ++++++++++++++++++++++++ >>>> 1 file changed, 24 insertions(+) >>>> >>>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c >>>> index 27c8e3355d..3ec0f330fb 100644 >>>> --- a/libavformat/hlsenc.c >>>> +++ b/libavformat/hlsenc.c >>>> @@ -810,6 +810,7 @@ static int parse_playlist(AVFormatContext *s, const >>>> char *url) >>>> int64_t new_start_pos; >>>> char line[1024]; >>>> const char *ptr; >>>> + const char *end; >>>> >>>> if ((ret = ffio_open_whitelist(&in, url, AVIO_FLAG_READ, >>>> &s->interrupt_callback, NULL, >>>> @@ -842,6 +843,29 @@ static int parse_playlist(AVFormatContext *s, const >>>> char *url) >>>> } else if (av_strstart(line, "#EXTINF:", &ptr)) { >>>> is_segment = 1; >>>> hls->duration = atof(ptr); >>>> + } else if (av_stristart(line, "#EXT-X-KEY:", &ptr)) { >>>> + ptr = av_stristr(line, "URI=\""); >>>> + if (ptr) { >>>> + ptr += strlen("URI=\""); >>>> + end = av_stristr(ptr, ","); >>>> + if (end) { >>>> + av_strlcpy(hls->key_uri, ptr, end - ptr); >>>> + } else { >>>> + av_strlcpy(hls->key_uri, ptr, sizeof(hls->key_uri)); >>>> >>> >> I know that this patch has already been applied (although it didn't get >> any reviews on the ML), but I think that there are some issues with it. >> First, it seems that both av_strlcpy() cases above will copy the >> terminating '\"' character into hls->key_uri. Perhaps that won't cause an >> issue in practice, but it is likely not the intended result. Second, with >> both av_strlcpy() calls that use a length of end - ptr, this could in >> theory result in a buffer overrun depending on how long the URI is, since >> the destination buffers have a fixed size. This issue is prevented in the >> second call to av_strlcpy(), since it uses sizeof(hls->key_uri), but it is >> a potential issue with the first calls (note that this comment applies to >> the IV=0x code below). If you want to use av_strlcpy(), either make sure >> that end - ptr is less than or equal to sizeof(hls->key_uri) or do >> something like *end = 0 first and then use av_strlcpy(hls->key_uri, ptr, >> sizeof(hls->key_uri)). >> >> In addition, based on the EXT-X-KEY example at >> https://developer.apple.com/library/content/technotes/tn2288/_index.html >> , it would appear that an EXT-X-KEY declaration may span multiple lines. >> Your solution will not work properly in this case. >> > Hi Aaron, > I think the libavformat/hls.c maybe have the same problem, because > both of hlsenc and hls use read_chomp_line to read one line, > that need check the '\' tail a line, and read the next line into a > buffer, that maybe better, is that right? Yes, I was thinking the same thing, that read_chomp_line() needs to be enhanced to deal with declarations that span multiple lines. That probably belongs in a separate patch though, even if it is only relevant for EXT-X-KEY. Aaron Levinson
2017-05-05 12:29 GMT+08:00 Aaron Levinson <alevinsn@aracnet.com>: > On 5/4/2017 9:15 PM, Steven Liu wrote: > >> 2017-05-03 9:49 GMT+08:00 Aaron Levinson <alevinsn@aracnet.com>: >> >> On 4/27/2017 7:21 PM, Steven Liu wrote: >>> >>> 2017-04-26 7:30 GMT+08:00 Steven Liu <lq@chinaffmpeg.org>: >>>> >>>> fix ticket id: #6353 >>>> >>>>> >>>>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org> >>>>> --- >>>>> libavformat/hlsenc.c | 24 ++++++++++++++++++++++++ >>>>> 1 file changed, 24 insertions(+) >>>>> >>>>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c >>>>> index 27c8e3355d..3ec0f330fb 100644 >>>>> --- a/libavformat/hlsenc.c >>>>> +++ b/libavformat/hlsenc.c >>>>> @@ -810,6 +810,7 @@ static int parse_playlist(AVFormatContext *s, const >>>>> char *url) >>>>> int64_t new_start_pos; >>>>> char line[1024]; >>>>> const char *ptr; >>>>> + const char *end; >>>>> >>>>> if ((ret = ffio_open_whitelist(&in, url, AVIO_FLAG_READ, >>>>> &s->interrupt_callback, NULL, >>>>> @@ -842,6 +843,29 @@ static int parse_playlist(AVFormatContext *s, >>>>> const >>>>> char *url) >>>>> } else if (av_strstart(line, "#EXTINF:", &ptr)) { >>>>> is_segment = 1; >>>>> hls->duration = atof(ptr); >>>>> + } else if (av_stristart(line, "#EXT-X-KEY:", &ptr)) { >>>>> + ptr = av_stristr(line, "URI=\""); >>>>> + if (ptr) { >>>>> + ptr += strlen("URI=\""); >>>>> + end = av_stristr(ptr, ","); >>>>> + if (end) { >>>>> + av_strlcpy(hls->key_uri, ptr, end - ptr); >>>>> + } else { >>>>> + av_strlcpy(hls->key_uri, ptr, >>>>> sizeof(hls->key_uri)); >>>>> >>>>> >>>> I know that this patch has already been applied (although it didn't get >>> any reviews on the ML), but I think that there are some issues with it. >>> First, it seems that both av_strlcpy() cases above will copy the >>> terminating '\"' character into hls->key_uri. Perhaps that won't cause >>> an >>> issue in practice, but it is likely not the intended result. Second, >>> with >>> both av_strlcpy() calls that use a length of end - ptr, this could in >>> theory result in a buffer overrun depending on how long the URI is, since >>> the destination buffers have a fixed size. This issue is prevented in >>> the >>> second call to av_strlcpy(), since it uses sizeof(hls->key_uri), but it >>> is >>> a potential issue with the first calls (note that this comment applies to >>> the IV=0x code below). If you want to use av_strlcpy(), either make sure >>> that end - ptr is less than or equal to sizeof(hls->key_uri) or do >>> something like *end = 0 first and then use av_strlcpy(hls->key_uri, ptr, >>> sizeof(hls->key_uri)). >>> >>> In addition, based on the EXT-X-KEY example at >>> https://developer.apple.com/library/content/technotes/tn2288/_index.html >>> , it would appear that an EXT-X-KEY declaration may span multiple lines. >>> Your solution will not work properly in this case. >>> >>> Hi Aaron, >> I think the libavformat/hls.c maybe have the same problem, because >> both of hlsenc and hls use read_chomp_line to read one line, >> that need check the '\' tail a line, and read the next line into a >> buffer, that maybe better, is that right? >> > > Yes, I was thinking the same thing, that read_chomp_line() needs to be > enhanced to deal with declarations that span multiple lines. That probably > belongs in a separate patch though, even if it is only relevant for > EXT-X-KEY. Yes, I think the better way is move them to a public space for the hlsenc and hls. > > > Aaron Levinson > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c index 27c8e3355d..3ec0f330fb 100644 --- a/libavformat/hlsenc.c +++ b/libavformat/hlsenc.c @@ -810,6 +810,7 @@ static int parse_playlist(AVFormatContext *s, const char *url) int64_t new_start_pos; char line[1024]; const char *ptr; + const char *end; if ((ret = ffio_open_whitelist(&in, url, AVIO_FLAG_READ, &s->interrupt_callback, NULL, @@ -842,6 +843,29 @@ static int parse_playlist(AVFormatContext *s, const char *url) } else if (av_strstart(line, "#EXTINF:", &ptr)) { is_segment = 1; hls->duration = atof(ptr); + } else if (av_stristart(line, "#EXT-X-KEY:", &ptr)) { + ptr = av_stristr(line, "URI=\""); + if (ptr) { + ptr += strlen("URI=\""); + end = av_stristr(ptr, ","); + if (end) { + av_strlcpy(hls->key_uri, ptr, end - ptr); + } else { + av_strlcpy(hls->key_uri, ptr, sizeof(hls->key_uri)); + } + } + + ptr = av_stristr(line, "IV=0x"); + if (ptr) { + ptr += strlen("IV=0x"); + end = av_stristr(ptr, ","); + if (end) { + av_strlcpy(hls->iv_string, ptr, end - ptr); + } else { + av_strlcpy(hls->iv_string, ptr, sizeof(hls->iv_string)); + } + } + } else if (av_strstart(line, "#", NULL)) { continue; } else if (line[0]) {
fix ticket id: #6353 Signed-off-by: Steven Liu <lq@chinaffmpeg.org> --- libavformat/hlsenc.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)