Implement and use MBEDTLS_STATIC_ASSERT() #7229

tom-cosgrove-arm · 2023-03-08T14:22:04Z

Tested the following combinations of systems/compilers/compiler flags with assert ok/expected to fail:

OpenBSD gcc 4.2.1 assert_ok [-std=c99 [-pedantic]] -Wall -Wextra -Werror OK
OpenBSD gcc 4.2.1 assert_fails [-std=c99 [-pedantic]] -Wall -Wextra -Werror OK

clang 10.0.1 assert_ok [-std=c99 [-pedantic]] -Wall -Wextra -Werror OK
clang 10.0.1 assert_fails [-std=c99 [-pedantic]] -Wall -Wextra -Werror OK

Ubuntu gcc 12.0.1 assert_ok [-std=c99 [-pedantic]] -Wall -Wextra -Werror OK
Ubuntu gcc 12.0.1 assert_fails [-std=c99 [-pedantic]] -Wall -Wextra -Werror OK

Ubuntu clang 15.0.7 assert_ok [-std=c99 [-pedantic]] -Wall -Wextra -Werror OK
Ubuntu clang 15.0.7 assert_fails [-std=c99 [-pedantic]] -Wall -Wextra -Werror OK

macOS clang 14.0.0 assert_ok [-std=c99 [-pedantic]] -Wall -Wextra -Werror OK
macOS clang 14.0.0 assert_fails [-std=c99 [-pedantic]] -Wall -Wextra -Werror OK

Windows Visual Studio 2017 assert_ok (cmake standard build) OK
Windows Visual Studio 2017 assert_fails (cmake standard build) OK

Gatekeeper checklist

changelog not required - this is internal to the library, just enhancing existing functionality
backport Backport 2.28: Implement and use MBEDTLS_STATIC_ASSERT() #7320
tests not required

Fixes Mbed-TLS#3693 Signed-off-by: Tom Cosgrove <[email protected]>

tom-cosgrove-arm · 2023-03-08T14:23:13Z

library/ssl_tls12_server.c

@@ -1510,10 +1510,9 @@ static int ssl_parse_client_hello(mbedtls_ssl_context *ssl)
            MBEDTLS_TLS_SIG_NONE
        };

-#if defined(static_assert)


NOTE: since there was no #include <assert.h> at the top of this file, defined(static_assert) would always be false, so this check was never going to happen - just showing that we should have an "always-present" static assert like this

tom-cosgrove-arm · 2023-03-08T14:24:24Z

library/psa_crypto.c

-#endif
+MBEDTLS_STATIC_ASSERT(
+    (MBEDTLS_PSA_KA_MASK_EXTERNAL_ONLY & MBEDTLS_PSA_KA_MASK_DUAL_USE) == 0,
+    "One or more key attribute flag is listed as both external-only and dual-use")


Annoyingly I couldn't find a way to have the macro work outside functions where it could have a ; after it, since if it expands to nothing, you end up with a stray ; that the compiler complains about

Would it work to make it expand to a function declaration?

daverodgman

LGTM, but some testing would be nice. It's easy to add a test for a static assert that always passes. We'd need a new component in all.sh for a static assert that should fail to compile for certain compilers though - it would be good to have if it's not excessive effort.

gilles-peskine-arm

Looks ok to me, but possibly incomplete.

I don't think we need tests of negative assertions on the CI. That would be a lot of work if we want to have that for multiple compilers, and not much benefit if we only do it for GCC/Clang.

gilles-peskine-arm · 2023-03-09T22:05:08Z

library/psa_crypto.c

-#endif
+MBEDTLS_STATIC_ASSERT(
+    (MBEDTLS_PSA_KA_MASK_EXTERNAL_ONLY & MBEDTLS_PSA_KA_MASK_DUAL_USE) == 0,
+    "One or more key attribute flag is listed as both external-only and dual-use")


Would it work to make it expand to a function declaration?

gilles-peskine-arm · 2023-03-09T22:14:18Z

library/common.h

+ * Can't use the C11-style `defined(static_assert)` on FreeBSD, since it
+ * defines static_assert even with -std=c99, but then complains about it.
+ */
+#if defined(static_assert) && !defined(__FreeBSD__)


In C23, static_assert will no longer be a macro, it's a keyword. Should we add || __STDC_VERSION__ >= 202300L? Or just make a note to add this when C23 is finalized?

We've already decided not to pre-add stuff for C23 on other PRs, so let's not add the __STDC_VERSION__ check. Not sure of the benefit of adding a TODO into the code, vs coming back once C23 is a real thing

gilles-peskine-arm · 2023-03-09T22:16:44Z

library/common.h

+ * Can be used outside functions (but don't add a trailing ';' in that case:
+ * the semicolon is included here to avoid triggering -Wextra-semi when
+ * MBEDTLS_STATIC_ASSERT() expands to nothing).
+ * Can't use the C11-style `defined(static_assert)` on FreeBSD, since it


Oh, you didn't find a way to bypass than warning 😞 . Maybe we could detect FreeBSD and use _Static_assert there?

_Static_assert() is only available depending what you're building for. But yes, that could be added in a follow-up PR

gilles-peskine-arm · 2023-03-09T22:17:28Z

library/common.h

+#else
+#define MBEDTLS_STATIC_ASSERT(expr, msg)
+#endif
+


Can we also use this in tests/include/test/macros.h to replace STATIC_ASSERT_EXPR?

Yes, should be possible, but I haven't tested it. That would make an excellent follow-up PR

gilles-peskine-arm · 2023-03-09T22:19:29Z

library/common.h

+#else
+#define MBEDTLS_UNUSED
+#endif
+#define MBEDTLS_STATIC_ASSERT2(expr, count) extern int MBEDTLS_UNUSED mbedtls_static_assert_failed ## count [2 * !!(expr) - 1];


Do we really need __COUNTER__? What's the benefit over the implementation in test/macros.h?

STATIC_ASSERT_EXPR() can only be used wherever an expression can be used, i.e. wouldn't work in some of the existing cases.

__COUNTER__ has been around in gcc for at least 10 years, so the systems without it (which this will still work on) should be fairly few.

Note I was aiming for "better (and we can build on it)" rather than "perfect"

Signed-off-by: Tom Cosgrove <[email protected]>

tom-cosgrove-arm · 2023-03-14T12:06:42Z

Okay, I've updated this to more closely match development, except that it will work on FreeBSD.

Basically, just abstracts out the test to common.h and allows MBEDTLS_STATIC_ASSERT() to be used unconditionally.

We can figure out an implementation on other systems later

Implement and use MBEDTLS_STATIC_ASSERT()

6ef9bb3

Fixes Mbed-TLS#3693 Signed-off-by: Tom Cosgrove <[email protected]>

tom-cosgrove-arm commented Mar 8, 2023

View reviewed changes

daverodgman requested changes Mar 9, 2023

View reviewed changes

gilles-peskine-arm reviewed Mar 9, 2023

View reviewed changes

gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Mar 9, 2023

Have MBEDTLS_STATIC_ASSERT() match current development more closely

57f04b8

Signed-off-by: Tom Cosgrove <[email protected]>

daverodgman approved these changes Mar 14, 2023

View reviewed changes

daverodgman added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Mar 14, 2023

paul-elliott-arm approved these changes Mar 14, 2023

View reviewed changes

tom-cosgrove-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Mar 14, 2023

daverodgman removed the needs-ci Needs to pass CI tests label Mar 14, 2023

daverodgman merged commit 4a1d3be into Mbed-TLS:development Mar 14, 2023

paul-elliott-arm mentioned this pull request Mar 20, 2023

Backport 2.28: Implement and use MBEDTLS_STATIC_ASSERT() #7319

Closed

3 tasks

tom-cosgrove-arm mentioned this pull request Mar 20, 2023

Backport 2.28: Implement and use MBEDTLS_STATIC_ASSERT() #7320

Merged

3 tasks

gilles-peskine-arm mentioned this pull request Jul 6, 2023

Remove cruft from groovy scripts Mbed-TLS/mbedtls-test#111

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement and use MBEDTLS_STATIC_ASSERT() #7229

Implement and use MBEDTLS_STATIC_ASSERT() #7229

tom-cosgrove-arm commented Mar 8, 2023 •

edited

Loading

tom-cosgrove-arm Mar 8, 2023

tom-cosgrove-arm Mar 8, 2023

gilles-peskine-arm Mar 9, 2023

daverodgman left a comment

gilles-peskine-arm left a comment

gilles-peskine-arm Mar 9, 2023

gilles-peskine-arm Mar 9, 2023

tom-cosgrove-arm Mar 14, 2023

gilles-peskine-arm Mar 9, 2023

tom-cosgrove-arm Mar 14, 2023

gilles-peskine-arm Mar 9, 2023

tom-cosgrove-arm Mar 14, 2023

gilles-peskine-arm Mar 9, 2023

tom-cosgrove-arm Mar 14, 2023

tom-cosgrove-arm commented Mar 14, 2023

Implement and use MBEDTLS_STATIC_ASSERT() #7229

Implement and use MBEDTLS_STATIC_ASSERT() #7229

Conversation

tom-cosgrove-arm commented Mar 8, 2023 • edited Loading

Gatekeeper checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daverodgman left a comment

Choose a reason for hiding this comment

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tom-cosgrove-arm commented Mar 14, 2023

tom-cosgrove-arm commented Mar 8, 2023 •

edited

Loading