[FFmpeg-devel] [PATCH v2] avformat/hls: use av_strlcopy instead of strncpy

[FFmpeg-devel] [PATCH v2] avformat/hls: use av_strlcopy instead of strncpy

[Leo Izen]

Avoids a -Wstringop-truncation warning by using av_strlcopy instead of
strncpy.

Signed-off-by: Leo Izen <leo.izen@gmail.com>
---
 libavformat/hls.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavformat/hls.c b/libavformat/hls.c
index f5f549b24d..39440176c9 100644
--- a/libavformat/hls.c
+++ b/libavformat/hls.c
@@ -543,8 +543,8 @@ static struct rendition *new_rendition(HLSContext *c, struct rendition_info *inf
         int langlen = strlen(rend->language);
         if (langlen < sizeof(rend->language) - 3) {
             rend->language[langlen] = ',';
-            strncpy(rend->language + langlen + 1, info->assoc_language,
-                    sizeof(rend->language) - langlen - 2);
+            av_strlcpy(rend->language + langlen + 1, info->assoc_language,
+                    sizeof(rend->language) - langlen - 1);
         }
     }
 
-- 
2.42.0

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avformat/hls: use av_strlcopy instead of strncpy

[Leo Izen]

On 10/29/23 08:47, Leo Izen wrote:
> Avoids a -Wstringop-truncation warning by using av_strlcopy instead of
> strncpy.
> 

Changes from v1:

- changed the length attribute, so now it has similar semantics to earlier.

This patch has the same semantics with regard to the null-termination 
and the length of the string. However, it avoids a compiler warning 
because the compiler thinks strncpy is unsafe.

- Leo Izen
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avformat/hls: use av_strlcopy instead of strncpy

[Andreas Rheinhardt]

Leo Izen:
> Avoids a -Wstringop-truncation warning by using av_strlcopy instead of
> strncpy.
> 
> Signed-off-by: Leo Izen <leo.izen@gmail.com>
> ---
>  libavformat/hls.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/hls.c b/libavformat/hls.c
> index f5f549b24d..39440176c9 100644
> --- a/libavformat/hls.c
> +++ b/libavformat/hls.c
> @@ -543,8 +543,8 @@ static struct rendition *new_rendition(HLSContext *c, struct rendition_info *inf
>          int langlen = strlen(rend->language);
>          if (langlen < sizeof(rend->language) - 3) {
>              rend->language[langlen] = ',';
> -            strncpy(rend->language + langlen + 1, info->assoc_language,
> -                    sizeof(rend->language) - langlen - 2);
> +            av_strlcpy(rend->language + langlen + 1, info->assoc_language,
> +                    sizeof(rend->language) - langlen - 1);
>          }
>      }
>  

As I said before: You are merely hiding the truncation issue instead of
fixing it.

- 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".

Re: [FFmpeg-devel] [PATCH v2] avformat/hls: use av_strlcopy instead of strncpy

[Leo Izen]

On 10/29/23 20:23, Andreas Rheinhardt wrote:
> Leo Izen:
>> Avoids a -Wstringop-truncation warning by using av_strlcopy instead of
>> strncpy.
>>
>> Signed-off-by: Leo Izen <leo.izen@gmail.com>
>> ---
>>   libavformat/hls.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavformat/hls.c b/libavformat/hls.c
>> index f5f549b24d..39440176c9 100644
>> --- a/libavformat/hls.c
>> +++ b/libavformat/hls.c
>> @@ -543,8 +543,8 @@ static struct rendition *new_rendition(HLSContext *c, struct rendition_info *inf
>>           int langlen = strlen(rend->language);
>>           if (langlen < sizeof(rend->language) - 3) {
>>               rend->language[langlen] = ',';
>> -            strncpy(rend->language + langlen + 1, info->assoc_language,
>> -                    sizeof(rend->language) - langlen - 2);
>> +            av_strlcpy(rend->language + langlen + 1, info->assoc_language,
>> +                    sizeof(rend->language) - langlen - 1);
>>           }
>>       }
>>   
> 
> As I said before: You are merely hiding the truncation issue instead of
> fixing it.
> 

I don't see how? It will only be truncated if info->assoc_language is 
very very long, and in that case it won't fit inside a 64-byte buffer so 
it will *have* to be truncated. But it will be nul-terminated in either 
case, so there's no real difference between the old and new code, other 
than the warning.

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avformat/hls: use av_strlcopy instead of strncpy

[Nicolas George]

Leo Izen (12023-10-29):
>			  But it will be nul-terminated in either case, so
> there's no real difference between the old and new code, other than the
> warning.

No real difference = you are not fixing the bug. There should be a
difference: user data should not be silently truncated.

Regards,

-- 
  Nicolas George
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avformat/hls: use av_strlcopy instead of strncpy

[Leo Izen]

On 10/30/23 03:51, Nicolas George wrote:
> Leo Izen (12023-10-29):
>> 			  But it will be nul-terminated in either case, so
>> there's no real difference between the old and new code, other than the
>> warning.
> 
> No real difference = you are not fixing the bug. There should be a
> difference: user data should not be silently truncated.
> 

There isn't really a bug here, just an extraneous compiler warning.

- Leo Izen (Traneptora)
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avformat/hls: use av_strlcopy instead of strncpy

[Nicolas George]

Leo Izen (12023-10-30):
> > difference: user data should not be silently truncated.
> There isn't really a bug here, just an extraneous compiler warning.

Truncating user data silently is a bug. The warning is right and needs
proper fix.

Regards,

-- 
  Nicolas George
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avformat/hls: use av_strlcopy instead of strncpy

[Leo Izen]

On 10/30/23 09:55, Nicolas George wrote:
> Leo Izen (12023-10-30):
>>> difference: user data should not be silently truncated.
>> There isn't really a bug here, just an extraneous compiler warning.
> 
> Truncating user data silently is a bug. The warning is right and needs
> proper fix.
> 
> Regards,
> 

The warning is not that truncation might occur, but that if it does 
occur, the buffer won't be nul-terminated. This is not actually the case 
because the size passed to strncpy is one smaller than the buffer size, 
and the buffer is zeroed. However, the compiler doesn't know this.

Also, fwiw, info->assoc_language is the name of a language. It shouldn't 
actually ever be 63 characters long for real streams in the wild, and 
truncating it is just a memory-safety precaution. It's not a bug.

- Leo Izen
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avformat/hls: use av_strlcopy instead of strncpy

[Nicolas George]

Leo Izen (12023-10-30):
> Also, fwiw, info->assoc_language is the name of a language. It shouldn't
> actually ever be 63 characters long for real streams in the wild, and
> truncating it is just a memory-safety precaution. It's not a bug.


It SHOULD not, but it CAN be, and properly reporting the error to the
user is important.

It is just about adding an if and return error.

Regards,

-- 
  Nicolas George
_______________________________________________
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".