| Message ID | 20231029124723.398581-1-leo.izen@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v2] avformat/hls: use av_strlcopy instead of strncpy | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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
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
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.
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,
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)
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,
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
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,
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); } }
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(-)