[3.6] Fix GCC 15 warning 'Wunterminated-string-initialization'

[felixc-arm]

mbedtls-3.6 part of #9944

Merge order (for 3.6):
Merge this PR

Once this PR + the mbedtls development (#10216) and TF-PSA-Crypto (Mbed-TLS/TF-PSA-Crypto#313) PRs have been merged, then merge the framework PR (Mbed-TLS/mbedtls-framework#173)

This PR does not depend on the framework PR Mbed-TLS/mbedtls-framework#173 in terms of submodule updates, but will require it to be merged to fix the issue completely. AIUI during the release process the framework pointer is updated, so I haven't done it here on the assumption that it will be done later, but I can change this to ensure the framework change gets pulled in when this is merged. so if we want the issue to be resolved before the release process starts, then there will need to be an extra commit updating the framework pointer once the framework PR is merged.

PR checklist

• changelog provided
• development PR provided: [Development] Fix GCC 15 warning 'Wunterminated-string-initialization' #10216
• TF-PSA-Crypto PR provided: [Crypto] Fix GCC 15 warning 'Wunterminated-string-initialization' TF-PSA-Crypto#313
• framework PR provided: Fix GCC 15 warning 'Wunterminated-string-initialization' mbedtls-framework#173
• 3.6 PR provided: HERE
• tests not required because: build only change

[gilles-peskine-arm]

__attribute__ is specific to GCC and GCC-like compilers such as Clang. And this attribute might not exist in older versions that we still care about, which we can check for with __has_attribute. So we need something like

#if __GCC__ && defined(__has_attribute) && __has_attribute()__non_string__)
#define MBEDTLS_ATTRIBUTE_STRING_IS_BINARY __attribute__(__nonstring__)
#else
#define MBEDTLS_ATTRIBUTE_STRING_IS_BINARY
#endif

Probably in tf-psa-crypto/core/common.h or some such.

[mpg]

You shouldn't use defined(__has_attribute) && __has_attribute() in the same expression, because if the LHS of the && evaluates to false, the preprocessor still needs to be able to parse the RHS which it won't. Proper usage of __has_attribute is as follows:

#if defined(__has_attribute)
#if __has_attribute(__nonstring__)
#define MBEDTLS_HAS_ATTRIBUTE_NONSTRING
#endif
#endif
#if MBEDTLS_HAS_ATTRIBUTE_NONSTRING
#define MBEDTLS_ATTRIBUTE_STRING_IS_BINARY __attribute__(__nonstring__)
#else
#define MBEDTLS_ATTRIBUTE_STRING_IS_BINARY
#endif

[mpg]

(Btw this is what the GCC documentation, which you linked to, says.)

[irwir]

Using non-standard keywords overcomplicates the matter for a handful of small arrays.
Direct and portable way:
static const char tls13_label_prefix[6] = { 't', 'l', 's', '1', '3', ' ' };
If necessary (probably not) - a comment might be added that this is "tls13 " string without terminating null.

[gilles-peskine-arm]

A downside of using {'...'} is that the strings from the RFC wouldn't be greppable.

Also, using a macro has the advantage that if a reader wonders why it's there, they can just look at the definition of the macro. We don't need to repeat a long explanation about strings and warnings in many places.

[irwir]

There is no reason grep would fail to find this:
static const char tls13_label_prefix[6] = { 't', 'l', 's', '1', '3', ' ' }; // "tls13 "

[gilles-peskine-arm]

LGTM except the changelog entry.

I've checked locally that #10215 plus Mbed-TLS/mbedtls-framework#173 results in no warning with GCC 15 in the full config.

[gilles-peskine-arm]

This is both longer than it needs (we don't need to explain the details of what the warning is about), and a bit too non-committal. We should claim that we've silenced the warning. (We don't test every configuration with GCC 15, so we may have missed one, but since we're testing the full config plus drivers, it's unlikely that something slipped through the crack.)

Suggested wording, partly based on prior changelog entries:

Silence spurious -Wunterminated-string-initialization warnings introduced by GCC 15. Fixes #9944.

(We've gone as short as “Fix IAR compiler warnings.”, but I think that was too extreme on the short side.)

[mpg]

LGTM, thanks!

[mpg]

I've checked locally that #10215 plus Mbed-TLS/mbedtls-framework#173 results in no warning with GCC 15 in the full config.

That's a good intermediate step but in the end we want that checked by the CI, so I think we need an extra PR once everything is merged to update component_test_gcc15_drivers_opt(). So I think the merge order would be:

1. development (crypto then mbedtls) and 3.6
2. framework (which depends on the above)
3. in development and 3.6, update the framework pointer and component_test_gcc15_drivers_opt()

Wdyt @felixc-arm ?

[felixc-arm]

Sure, makes sense to me - I can put up those (three? - I guess TF-PSA-Crypto will also need a PR that just updates the framework pointer? or maybe not if the CI doesn't use TF-PSA-Crypto's framework) PRs soon, although I likely won't be around to merge them unless everything is very efficient today, so it might make sense for someone else to do those PRs, but I can still put them up and then have someone take over if needed.

[gilles-peskine-arm]

Hopefully we'll merge 3.6 and crypto this morning, let's at least wait until that's done. The follow-up PR will be a submodule update plus a very simple patch, so we won't really save time by preparing it in advance. (We'd save risk to have a positive CI result, but the risk is low and we'd have to wait for the CI anyway.)

[gilles-peskine-arm]

LGTM

[mpg]

LGTM, thanks!

[irwir]

The issue was not the warning itself, but "too many initializer values" as in
char f[2] = {1,2,3};
It was not fixed properly.

[gilles-peskine-arm]

@irwir Sorry, where is that? We're not silencing any warning about having too many initializer values, and it's not popping up.

[irwir]

The root of the issue is in the fact that sizeof("abc") is 4, not 3.
Hence it was always incorrect to initialize array of 6 char with 7 chars, even if compilers allowed this - for a while. Now they decided to discourage such practice.

To close my comments here, there should have been no macros, no explanations of compiler's weirdness in changes.log, but real elimination of the cause instead.

1. An array of 7 chars and use of sizeof(array) - 1 where required.
   static const char tls13_label_prefix[7] = "tls13 ";

2. Or an array of 6 as with exactly 6 initializer values as posted above.

[gilles-peskine-arm]

@irwir char a[3] = "abc"; has been valid C since K&R and is still valid C23. When initializing a fixed-size array with a string literal, the array contains the actual content of the literal, plus zeros to pad it. A null terminator only comes into play when initializing an array of unspecified size: char b[] = "abc" is a 4-byte array.

Because string literals have a null terminator in every other context, this is a gotcha in the C language. So it makes sense that some compilers warn about it. But the behavior is well-defined and has always been. We know about this behavior and rely on it, so it's natural that we tell the compiler that we know what we're doing.