[PATCH] image: Ensure image header name is null terminated
When building with GCC 12:
../include/image.h:779:9: warning: ‘strncpy’ specified bound 32 equals destination size [-Wstringop-truncation] 779 | strncpy(image_get_name(hdr), name, IH_NMLEN); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Ensure the copied string is null terminated by always setting the final byte to 0. Shorten the strncpy to IH_NMLEN-1 as we will always overwrite the last byte.
We can't use strlcpy as this is code is built on the host as well as the target.
Fixes: b97a2a0a21f2 ("[new uImage] Define a API for image handling operations") Signed-off-by: Joel Stanley joel@jms.id.au --- include/image.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/image.h b/include/image.h index e4c6a50b885f..665b2278b7fb 100644 --- a/include/image.h +++ b/include/image.h @@ -776,7 +776,10 @@ image_set_hdr_b(comp) /* image_set_comp */
static inline void image_set_name(image_header_t *hdr, const char *name) { - strncpy(image_get_name(hdr), name, IH_NMLEN); + char *hdr_name = image_get_name(hdr); + + strncpy(hdr_name, name, IH_NMLEN - 1); + hdr_name[IH_NMLEN - 1] = '\0'; }
int image_check_hcrc(const image_header_t *hdr);
Dear Joel,
In message 20220823055907.416060-1-joel@jms.id.au you wrote:
When building with GCC 12:
../include/image.h:779:9: warning: ‘strncpy’ specified bound 32 equals destination size [-Wstringop-truncation] 779 | strncpy(image_get_name(hdr), name, IH_NMLEN); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Ensure the copied string is null terminated by always setting the final byte to 0. Shorten the strncpy to IH_NMLEN-1 as we will always overwrite the last byte.
We can't use strlcpy as this is code is built on the host as well as the target.
Fixes: b97a2a0a21f2 ("[new uImage] Define a API for image handling operations") Signed-off-by: Joel Stanley joel@jms.id.au
include/image.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/image.h b/include/image.h index e4c6a50b885f..665b2278b7fb 100644 --- a/include/image.h +++ b/include/image.h @@ -776,7 +776,10 @@ image_set_hdr_b(comp) /* image_set_comp */
static inline void image_set_name(image_header_t *hdr, const char *name) {
- strncpy(image_get_name(hdr), name, IH_NMLEN);
- char *hdr_name = image_get_name(hdr);
- strncpy(hdr_name, name, IH_NMLEN - 1);
- hdr_name[IH_NMLEN - 1] = '\0';
}
Why don't you use strlcpy() instead? This covers exactly the situation we have here.
Best regards,
Wolfgang Denk
On Tue, Aug 23, 2022 at 03:59:07PM +1000, Joel Stanley wrote:
When building with GCC 12:
../include/image.h:779:9: warning: ‘strncpy’ specified bound 32 equals destination size [-Wstringop-truncation] 779 | strncpy(image_get_name(hdr), name, IH_NMLEN); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Ensure the copied string is null terminated by always setting the final byte to 0. Shorten the strncpy to IH_NMLEN-1 as we will always overwrite the last byte.
We can't use strlcpy as this is code is built on the host as well as the target.
Since this is in the header, isn't the point that it doesn't need to be null-terminated?
When printing we're careful to use:
"%.*s", IH_NMLEN, ...
so I think the warning is wrong here - we want both of the strncpy() behaviours that are normally considered strange:
- it's okay not to null terminate as this is an explicitly sized field
- we want to pad the whole field with zeroes if the string is short
Fixes: b97a2a0a21f2 ("[new uImage] Define a API for image handling operations") Signed-off-by: Joel Stanley joel@jms.id.au
include/image.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/image.h b/include/image.h index e4c6a50b885f..665b2278b7fb 100644 --- a/include/image.h +++ b/include/image.h @@ -776,7 +776,10 @@ image_set_hdr_b(comp) /* image_set_comp */
static inline void image_set_name(image_header_t *hdr, const char *name) {
- strncpy(image_get_name(hdr), name, IH_NMLEN);
- char *hdr_name = image_get_name(hdr);
- strncpy(hdr_name, name, IH_NMLEN - 1);
- hdr_name[IH_NMLEN - 1] = '\0';
}
int image_check_hcrc(const image_header_t *hdr);
2.35.1
Hi John,
On Tue, 23 Aug 2022 at 03:46, John Keeping john@metanate.com wrote:
On Tue, Aug 23, 2022 at 03:59:07PM +1000, Joel Stanley wrote:
When building with GCC 12:
../include/image.h:779:9: warning: ‘strncpy’ specified bound 32 equals destination size [-Wstringop-truncation] 779 | strncpy(image_get_name(hdr), name, IH_NMLEN); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Ensure the copied string is null terminated by always setting the final byte to 0. Shorten the strncpy to IH_NMLEN-1 as we will always overwrite the last byte.
We can't use strlcpy as this is code is built on the host as well as the target.
Since this is in the header, isn't the point that it doesn't need to be null-terminated?
When printing we're careful to use:
"%.*s", IH_NMLEN, ...so I think the warning is wrong here - we want both of the strncpy() behaviours that are normally considered strange:
it's okay not to null terminate as this is an explicitly sized field
we want to pad the whole field with zeroes if the string is short
That's my understanding too. We are careful to avoid expecting a terminator. I am not sure what to do with the warning though
Regards, Simon
Fixes: b97a2a0a21f2 ("[new uImage] Define a API for image handling operations") Signed-off-by: Joel Stanley joel@jms.id.au
include/image.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/image.h b/include/image.h index e4c6a50b885f..665b2278b7fb 100644 --- a/include/image.h +++ b/include/image.h @@ -776,7 +776,10 @@ image_set_hdr_b(comp) /* image_set_comp */
static inline void image_set_name(image_header_t *hdr, const char *name) {
strncpy(image_get_name(hdr), name, IH_NMLEN);
char *hdr_name = image_get_name(hdr);strncpy(hdr_name, name, IH_NMLEN - 1);hdr_name[IH_NMLEN - 1] = '\0';}
int image_check_hcrc(const image_header_t *hdr);
2.35.1
On 23/08/2022 15.38, Simon Glass wrote:
Hi John,
On Tue, 23 Aug 2022 at 03:46, John Keeping john@metanate.com wrote:
On Tue, Aug 23, 2022 at 03:59:07PM +1000, Joel Stanley wrote:
When building with GCC 12:
../include/image.h:779:9: warning: ‘strncpy’ specified bound 32 equals destination size [-Wstringop-truncation] 779 | strncpy(image_get_name(hdr), name, IH_NMLEN); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Ensure the copied string is null terminated by always setting the final byte to 0. Shorten the strncpy to IH_NMLEN-1 as we will always overwrite the last byte.
We can't use strlcpy as this is code is built on the host as well as the target.
Since this is in the header, isn't the point that it doesn't need to be null-terminated?
When printing we're careful to use:
"%.*s", IH_NMLEN, ...so I think the warning is wrong here - we want both of the strncpy() behaviours that are normally considered strange:
it's okay not to null terminate as this is an explicitly sized field
we want to pad the whole field with zeroes if the string is short
That's my understanding too. We are careful to avoid expecting a terminator. I am not sure what to do with the warning though
Maybe this could be some inspiration:
info gcc
'nonstring' The 'nonstring' variable attribute specifies that an object or member declaration with type array of 'char', 'signed char', or 'unsigned char', or pointer to such a type is intended to store character arrays that do not necessarily contain a terminating 'NUL'. This is useful in detecting uses of such arrays or pointers with functions that expect 'NUL'-terminated strings, and to avoid warnings when such an array or pointer is used as an argument to a bounded string manipulation function such as 'strncpy'. For example, without the attribute, GCC will issue a warning for the 'strncpy' call below because it may truncate the copy without appending the terminating 'NUL' character. Using the attribute makes it possible to suppress the warning. However, when the array is declared with the attribute the call to 'strlen' is diagnosed because when the array doesn't contain a 'NUL'-terminated string the call is undefined. To copy, compare, of search non-string character arrays use the 'memcpy', 'memcmp', 'memchr', and other functions that operate on arrays of bytes. In addition, calling 'strnlen' and 'strndup' with such arrays is safe provided a suitable bound is specified, and not diagnosed.
struct Data { char name [32] __attribute__ ((nonstring)); };
int f (struct Data *pd, const char *s) { strncpy (pd->name, s, sizeof pd->name); ... return strlen (pd->name); // unsafe, gets a warning }
[https://gcc.gnu.org/onlinedocs/gcc-12.2.0/gcc/Common-Variable-Attributes.htm...]
On Tue, Aug 23, 2022 at 03:59:07PM +1000, Joel Stanley wrote:
When building with GCC 12:
../include/image.h:779:9: warning: ‘strncpy’ specified bound 32 equals destination size [-Wstringop-truncation] 779 | strncpy(image_get_name(hdr), name, IH_NMLEN); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Ensure the copied string is null terminated by always setting the final byte to 0. Shorten the strncpy to IH_NMLEN-1 as we will always overwrite the last byte.
We can't use strlcpy as this is code is built on the host as well as the target.
Fixes: b97a2a0a21f2 ("[new uImage] Define a API for image handling operations") Signed-off-by: Joel Stanley joel@jms.id.au
So this breaks some tests: https://source.denx.de/u-boot/u-boot/-/jobs/496773#L201 and it's not clear to me if the problem is the tests or the fix itself (should we be doing a buffer of IH_NMLEN+1 and ensuring that's NULL terminated? I don't know).
Hi,
On Wed, 14 Sept 2022 at 16:11, Tom Rini trini@konsulko.com wrote:
On Tue, Aug 23, 2022 at 03:59:07PM +1000, Joel Stanley wrote:
When building with GCC 12:
../include/image.h:779:9: warning: ‘strncpy’ specified bound 32 equals destination size [-Wstringop-truncation] 779 | strncpy(image_get_name(hdr), name, IH_NMLEN); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Ensure the copied string is null terminated by always setting the final byte to 0. Shorten the strncpy to IH_NMLEN-1 as we will always overwrite the last byte.
We can't use strlcpy as this is code is built on the host as well as the target.
Fixes: b97a2a0a21f2 ("[new uImage] Define a API for image handling operations") Signed-off-by: Joel Stanley joel@jms.id.au
So this breaks some tests: https://source.denx.de/u-boot/u-boot/-/jobs/496773#L201 and it's not clear to me if the problem is the tests or the fix itself (should we be doing a buffer of IH_NMLEN+1 and ensuring that's NULL terminated? I don't know).
My reading of it is that the field is of length IH_NMLEN and there is only a terminator if the string is shorter than that.
So I don't think this patch is correct / needed. Perhaps we can find a way to silence the warning, e.g. using memcyp(xx,yy, min(IH_NMLEN, strnlen(yy, ...) + 1)) ?
Regards, Simon
participants (6)
-
Joel Stanley -
John Keeping -
Rasmus Villemoes -
Simon Glass -
Tom Rini -
Wolfgang Denk