diff mbox series

[FFmpeg-devel,2/2] avformat/hls: Don't strdup non-null-terminated string

Message ID 20200303042006.6370-2-andreas.rheinhardt@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel,1/2] avcodec/v4l2_m2m: Avoid using intermediate buffer
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 March 3, 2020, 4:20 a.m. UTC
If an URI indicated that the data protocol was in use, it would be
copied into a temporary buffer via strncpy(dst, src, strlen(src)),
thereby ensuring that the trailing \0 would not be copied, despite dst
being uninitialized. dst would then be av_strdup'ed, leading to
potential segfaults.

The solution to this is simple: Don't copy the URI in the temporary
buffer at all, instead av_strdup it directly.

This fixes a -Wstringop-truncation warning emitted by GCC 9.2.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
This is honestly untested, as this is not covered by FATE.

 libavformat/hls.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Andreas Rheinhardt March 21, 2020, 11:28 p.m. UTC | #1
Andreas Rheinhardt:
> If an URI indicated that the data protocol was in use, it would be
> copied into a temporary buffer via strncpy(dst, src, strlen(src)),
> thereby ensuring that the trailing \0 would not be copied, despite dst
> being uninitialized. dst would then be av_strdup'ed, leading to
> potential segfaults.
> 
> The solution to this is simple: Don't copy the URI in the temporary
> buffer at all, instead av_strdup it directly.
> 
> This fixes a -Wstringop-truncation warning emitted by GCC 9.2.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> This is honestly untested, as this is not covered by FATE.
> 
>  libavformat/hls.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/libavformat/hls.c b/libavformat/hls.c
> index 1f58e745a7..fc45719d1c 100644
> --- a/libavformat/hls.c
> +++ b/libavformat/hls.c
> @@ -403,8 +403,7 @@ static struct segment *new_init_section(struct playlist *pls,
>                                          const char *url_base)
>  {
>      struct segment *sec;
> -    char *ptr;
> -    char tmp_str[MAX_URL_SIZE];
> +    char tmp_str[MAX_URL_SIZE], *ptr = tmp_str;
>  
>      if (!info->uri[0])
>          return NULL;
> @@ -414,11 +413,11 @@ static struct segment *new_init_section(struct playlist *pls,
>          return NULL;
>  
>      if (!av_strncasecmp(info->uri, "data:", 5)) {
> -        strncpy(tmp_str, info->uri, strlen(info->uri));
> +        ptr = info->uri;
>      } else {
>          ff_make_absolute_url(tmp_str, sizeof(tmp_str), url_base, info->uri);
>      }
> -    sec->url = av_strdup(tmp_str);
> +    sec->url = av_strdup(ptr);
>      if (!sec->url) {
>          av_free(sec);
>          return NULL;
> 
Ping.

- Andreas
Steven Liu March 22, 2020, 12:57 a.m. UTC | #2
> 2020年3月22日 上午7:28,Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道:
> 
> Andreas Rheinhardt:
>> If an URI indicated that the data protocol was in use, it would be
>> copied into a temporary buffer via strncpy(dst, src, strlen(src)),
>> thereby ensuring that the trailing \0 would not be copied, despite dst
>> being uninitialized. dst would then be av_strdup'ed, leading to
>> potential segfaults.
>> 
>> The solution to this is simple: Don't copy the URI in the temporary
>> buffer at all, instead av_strdup it directly.
>> 
>> This fixes a -Wstringop-truncation warning emitted by GCC 9.2.
>> 
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>> This is honestly untested, as this is not covered by FATE.
>> 
>> libavformat/hls.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>> 
>> diff --git a/libavformat/hls.c b/libavformat/hls.c
>> index 1f58e745a7..fc45719d1c 100644
>> --- a/libavformat/hls.c
>> +++ b/libavformat/hls.c
>> @@ -403,8 +403,7 @@ static struct segment *new_init_section(struct playlist *pls,
>>                                         const char *url_base)
>> {
>>     struct segment *sec;
>> -    char *ptr;
>> -    char tmp_str[MAX_URL_SIZE];
>> +    char tmp_str[MAX_URL_SIZE], *ptr = tmp_str;
>> 
>>     if (!info->uri[0])
>>         return NULL;
>> @@ -414,11 +413,11 @@ static struct segment *new_init_section(struct playlist *pls,
>>         return NULL;
>> 
>>     if (!av_strncasecmp(info->uri, "data:", 5)) {
>> -        strncpy(tmp_str, info->uri, strlen(info->uri));
>> +        ptr = info->uri;
>>     } else {
>>         ff_make_absolute_url(tmp_str, sizeof(tmp_str), url_base, info->uri);
>>     }
>> -    sec->url = av_strdup(tmp_str);
>> +    sec->url = av_strdup(ptr);
>>     if (!sec->url) {
>>         av_free(sec);
>>         return NULL;
>> 
> Ping.
Looks ok.
> 
> - 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
Andreas Rheinhardt March 24, 2020, 8:30 p.m. UTC | #3
Steven Liu:
> 
> 
>> 2020年3月22日 上午7:28,Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道:
>>
>> Andreas Rheinhardt:
>>> If an URI indicated that the data protocol was in use, it would be
>>> copied into a temporary buffer via strncpy(dst, src, strlen(src)),
>>> thereby ensuring that the trailing \0 would not be copied, despite dst
>>> being uninitialized. dst would then be av_strdup'ed, leading to
>>> potential segfaults.
>>>
>>> The solution to this is simple: Don't copy the URI in the temporary
>>> buffer at all, instead av_strdup it directly.
>>>
>>> This fixes a -Wstringop-truncation warning emitted by GCC 9.2.
>>>
>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>>> ---
>>> This is honestly untested, as this is not covered by FATE.
>>>
>>> libavformat/hls.c | 7 +++----
>>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/libavformat/hls.c b/libavformat/hls.c
>>> index 1f58e745a7..fc45719d1c 100644
>>> --- a/libavformat/hls.c
>>> +++ b/libavformat/hls.c
>>> @@ -403,8 +403,7 @@ static struct segment *new_init_section(struct playlist *pls,
>>>                                         const char *url_base)
>>> {
>>>     struct segment *sec;
>>> -    char *ptr;
>>> -    char tmp_str[MAX_URL_SIZE];
>>> +    char tmp_str[MAX_URL_SIZE], *ptr = tmp_str;
>>>
>>>     if (!info->uri[0])
>>>         return NULL;
>>> @@ -414,11 +413,11 @@ static struct segment *new_init_section(struct playlist *pls,
>>>         return NULL;
>>>
>>>     if (!av_strncasecmp(info->uri, "data:", 5)) {
>>> -        strncpy(tmp_str, info->uri, strlen(info->uri));
>>> +        ptr = info->uri;
>>>     } else {
>>>         ff_make_absolute_url(tmp_str, sizeof(tmp_str), url_base, info->uri);
>>>     }
>>> -    sec->url = av_strdup(tmp_str);
>>> +    sec->url = av_strdup(ptr);
>>>     if (!sec->url) {
>>>         av_free(sec);
>>>         return NULL;
>>>
>> Ping.
> Looks ok.
>>
>> - 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
> 
Applied, thanks.

- Andreas
diff mbox series

Patch

diff --git a/libavformat/hls.c b/libavformat/hls.c
index 1f58e745a7..fc45719d1c 100644
--- a/libavformat/hls.c
+++ b/libavformat/hls.c
@@ -403,8 +403,7 @@  static struct segment *new_init_section(struct playlist *pls,
                                         const char *url_base)
 {
     struct segment *sec;
-    char *ptr;
-    char tmp_str[MAX_URL_SIZE];
+    char tmp_str[MAX_URL_SIZE], *ptr = tmp_str;
 
     if (!info->uri[0])
         return NULL;
@@ -414,11 +413,11 @@  static struct segment *new_init_section(struct playlist *pls,
         return NULL;
 
     if (!av_strncasecmp(info->uri, "data:", 5)) {
-        strncpy(tmp_str, info->uri, strlen(info->uri));
+        ptr = info->uri;
     } else {
         ff_make_absolute_url(tmp_str, sizeof(tmp_str), url_base, info->uri);
     }
-    sec->url = av_strdup(tmp_str);
+    sec->url = av_strdup(ptr);
     if (!sec->url) {
         av_free(sec);
         return NULL;