From 152f8d5b4655488ecc29d9ccdc36593a5a8b0b4e Mon Sep 17 00:00:00 2001 From: Cameron Bytheway Date: Tue, 31 Jan 2023 16:20:05 -0700 Subject: [PATCH 1/2] feat: add checked return values diagnostic --- crypto/s2n_ecdsa.c | 5 +- crypto/s2n_rsa.c | 5 +- scripts/s2n_safety_macros.py | 6 +-- .../features/S2N_DIAGNOSTICS_POP_SUPPORTED.c | 47 +++++++++++++++++++ .../S2N_DIAGNOSTICS_POP_SUPPORTED.flags | 1 + tests/features/S2N_DIAGNOSTICS_SUPPORTED.c | 33 +++++++++++++ .../features/S2N_DIAGNOSTICS_SUPPORTED.flags | 1 + tests/sidetrail/working/stubs/s2n_ensure.h | 1 + .../s2n_client_key_share_extension_test.c | 28 +++++++++-- tls/extensions/s2n_client_key_share.c | 18 ------- tls/extensions/s2n_client_key_share.h | 1 - tls/s2n_connection.c | 37 ++++++++++----- utils/s2n_ensure.h | 11 +++++ utils/s2n_safety_macros.h | 24 +++++----- 14 files changed, 163 insertions(+), 55 deletions(-) create mode 100644 tests/features/S2N_DIAGNOSTICS_POP_SUPPORTED.c create mode 100644 tests/features/S2N_DIAGNOSTICS_POP_SUPPORTED.flags create mode 100644 tests/features/S2N_DIAGNOSTICS_SUPPORTED.c create mode 100644 tests/features/S2N_DIAGNOSTICS_SUPPORTED.flags diff --git a/crypto/s2n_ecdsa.c b/crypto/s2n_ecdsa.c index e4da43f7f03..9fbc75049a2 100644 --- a/crypto/s2n_ecdsa.c +++ b/crypto/s2n_ecdsa.c @@ -40,13 +40,12 @@ EC_KEY *s2n_unsafe_ecdsa_get_non_const(const struct s2n_ecdsa_key *ecdsa_key) { PTR_ENSURE_REF(ecdsa_key); - /* pragma gcc diagnostic was added in gcc 4.6 */ -#if defined(__clang__) || S2N_GCC_VERSION_AT_LEAST(4, 6, 0) +#ifdef S2N_DIAGNOSTICS_SUPPORTED #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wcast-qual" #endif EC_KEY *out_ec_key = (EC_KEY *) ecdsa_key->ec_key; -#if defined(__clang__) || S2N_GCC_VERSION_AT_LEAST(4, 6, 0) +#ifdef S2N_DIAGNOSTICS_SUPPORTED #pragma GCC diagnostic pop #endif diff --git a/crypto/s2n_rsa.c b/crypto/s2n_rsa.c index 9fc1d4b5ca4..4bfd698f83f 100644 --- a/crypto/s2n_rsa.c +++ b/crypto/s2n_rsa.c @@ -36,13 +36,12 @@ RSA *s2n_unsafe_rsa_get_non_const(const struct s2n_rsa_key *rsa_key) { PTR_ENSURE_REF(rsa_key); - /* pragma gcc diagnostic was added in gcc 4.6 */ -#if defined(__clang__) || S2N_GCC_VERSION_AT_LEAST(4, 6, 0) +#ifdef S2N_DIAGNOSTICS_SUPPORTED #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wcast-qual" #endif RSA *out_rsa_key = (RSA *) rsa_key->rsa; -#if defined(__clang__) || S2N_GCC_VERSION_AT_LEAST(4, 6, 0) +#ifdef S2N_DIAGNOSTICS_SUPPORTED #pragma GCC diagnostic pop #endif diff --git a/scripts/s2n_safety_macros.py b/scripts/s2n_safety_macros.py index 6a5e64755ea..ada7107d29f 100644 --- a/scripts/s2n_safety_macros.py +++ b/scripts/s2n_safety_macros.py @@ -107,7 +107,7 @@ def cmp_check(op): MACROS = { 'BAIL(error)': dict( doc = 'Sets the global `s2n_errno` to `error` and returns with an `{error}`', - impl = 'do {{ _S2N_ERROR((error)); return {error}; }} while (0)', + impl = 'do {{ _S2N_ERROR((error)); __S2N_ENSURE_CHECKED_RETURN({error}); }} while (0)', harness = ''' static {ret} {bail}_harness() {{ @@ -588,7 +588,7 @@ def cmp_check(op): ), 'GUARD(result)': dict( doc = 'Ensures `{is_ok}`, otherwise the function will return `{error}`', - impl = '__S2N_ENSURE({is_ok}, return {error})', + impl = '__S2N_ENSURE({is_ok}, __S2N_ENSURE_CHECKED_RETURN({error}))', harness = ''' static {ret} {prefix}GUARD_harness({ret} result) {{ @@ -714,7 +714,7 @@ def push_doc(args): if other == context: continue; - impl = '__S2N_ENSURE({is_ok}, return {error})' + impl = '__S2N_ENSURE({is_ok}, __S2N_ENSURE_CHECKED_RETURN({error}))' args = { 'prefix': context['prefix'], 'suffix': other['suffix'], diff --git a/tests/features/S2N_DIAGNOSTICS_POP_SUPPORTED.c b/tests/features/S2N_DIAGNOSTICS_POP_SUPPORTED.c new file mode 100644 index 00000000000..a7c9d2be341 --- /dev/null +++ b/tests/features/S2N_DIAGNOSTICS_POP_SUPPORTED.c @@ -0,0 +1,47 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +/** + * This feature detects if the compiler properly pops diagnostics + */ + +#include + +#define MACRO_CHECK \ + do { \ + _Pragma("GCC diagnostic push") \ + _Pragma("GCC diagnostic error \"-Wconversion\"") \ + return -1; \ + _Pragma("GCC diagnostic pop") \ + } while (0) + +int signed_fun() +{ + MACRO_CHECK; +} + +/* this function is here to ensure the compiler properly pops the previous diagnostic */ +uint8_t unsigned_fun() +{ + return -1; +} + +int main() +{ + signed_fun(); + unsigned_fun(); + + MACRO_CHECK; +} diff --git a/tests/features/S2N_DIAGNOSTICS_POP_SUPPORTED.flags b/tests/features/S2N_DIAGNOSTICS_POP_SUPPORTED.flags new file mode 100644 index 00000000000..2f41be663b2 --- /dev/null +++ b/tests/features/S2N_DIAGNOSTICS_POP_SUPPORTED.flags @@ -0,0 +1 @@ +-Werror diff --git a/tests/features/S2N_DIAGNOSTICS_SUPPORTED.c b/tests/features/S2N_DIAGNOSTICS_SUPPORTED.c new file mode 100644 index 00000000000..79918890534 --- /dev/null +++ b/tests/features/S2N_DIAGNOSTICS_SUPPORTED.c @@ -0,0 +1,33 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +#include + +#define MACRO_CHECK \ + do { \ + _Pragma("GCC diagnostic push") \ + _Pragma("GCC diagnostic error \"-Wconversion\"") \ + return -1; \ + _Pragma("GCC diagnostic pop") \ + } while (0) + +int main() +{ + #pragma GCC diagnostic push + #pragma GCC diagnostic ignored "-Wcast-qual" + #pragma GCC diagnostic pop + + MACRO_CHECK; +} diff --git a/tests/features/S2N_DIAGNOSTICS_SUPPORTED.flags b/tests/features/S2N_DIAGNOSTICS_SUPPORTED.flags new file mode 100644 index 00000000000..2f41be663b2 --- /dev/null +++ b/tests/features/S2N_DIAGNOSTICS_SUPPORTED.flags @@ -0,0 +1 @@ +-Werror diff --git a/tests/sidetrail/working/stubs/s2n_ensure.h b/tests/sidetrail/working/stubs/s2n_ensure.h index 93657dd38c5..521e3e56f55 100644 --- a/tests/sidetrail/working/stubs/s2n_ensure.h +++ b/tests/sidetrail/working/stubs/s2n_ensure.h @@ -43,6 +43,7 @@ void *s2n_sidetrail_memset(void * ptr, int value, size_t num); } \ } while(0) +#define __S2N_ENSURE_CHECKED_RETURN(v) do { return v; } while(0) /** * The C runtime does not give a way to check these properties, diff --git a/tests/unit/s2n_client_key_share_extension_test.c b/tests/unit/s2n_client_key_share_extension_test.c index dca28cfdbb0..e92bea06382 100644 --- a/tests/unit/s2n_client_key_share_extension_test.c +++ b/tests/unit/s2n_client_key_share_extension_test.c @@ -42,6 +42,26 @@ static int s2n_write_named_curve(struct s2n_stuffer *out, const struct s2n_ecc_n static int s2n_write_key_share(struct s2n_stuffer *out, uint16_t iana_value, uint16_t share_size, const struct s2n_ecc_named_curve *existing_curve); +S2N_RESULT s2n_extensions_client_key_share_size(struct s2n_connection *conn, uint32_t *size) +{ + RESULT_ENSURE_REF(conn); + + const struct s2n_ecc_preferences *ecc_pref = NULL; + RESULT_GUARD_POSIX(s2n_connection_get_ecc_preferences(conn, &ecc_pref)); + RESULT_ENSURE_REF(ecc_pref); + + uint32_t s2n_client_key_share_extension_size = S2N_SIZE_OF_EXTENSION_TYPE + + S2N_SIZE_OF_EXTENSION_DATA_SIZE + + S2N_SIZE_OF_CLIENT_SHARES_SIZE; + + s2n_client_key_share_extension_size += S2N_SIZE_OF_KEY_SHARE_SIZE + S2N_SIZE_OF_NAMED_GROUP; + s2n_client_key_share_extension_size += ecc_pref->ecc_curves[0]->share_size; + + *size = s2n_client_key_share_extension_size; + + return S2N_RESULT_OK; +} + int main(int argc, char **argv) { BEGIN_TEST(); @@ -53,12 +73,12 @@ int main(int argc, char **argv) struct s2n_connection *conn; EXPECT_NOT_NULL(conn = s2n_connection_new(S2N_CLIENT)); - int key_share_size; - EXPECT_SUCCESS(key_share_size = s2n_extensions_client_key_share_size(conn)); + uint32_t key_share_size = 0; + EXPECT_OK(s2n_extensions_client_key_share_size(conn, &key_share_size)); /* should produce the same result if called twice */ - int key_share_size_again; - EXPECT_SUCCESS(key_share_size_again = s2n_extensions_client_key_share_size(conn)); + uint32_t key_share_size_again = 0; + EXPECT_OK(s2n_extensions_client_key_share_size(conn, &key_share_size_again)); EXPECT_EQUAL(key_share_size, key_share_size_again); /* should equal the size of the data written on send */ diff --git a/tls/extensions/s2n_client_key_share.c b/tls/extensions/s2n_client_key_share.c index 1b59001d398..55de6956eeb 100644 --- a/tls/extensions/s2n_client_key_share.c +++ b/tls/extensions/s2n_client_key_share.c @@ -451,24 +451,6 @@ static int s2n_client_key_share_recv(struct s2n_connection *conn, struct s2n_stu /* Old-style extension functions -- remove after extensions refactor is complete */ -uint32_t s2n_extensions_client_key_share_size(struct s2n_connection *conn) -{ - POSIX_ENSURE_REF(conn); - - const struct s2n_ecc_preferences *ecc_pref = NULL; - POSIX_GUARD(s2n_connection_get_ecc_preferences(conn, &ecc_pref)); - POSIX_ENSURE_REF(ecc_pref); - - uint32_t s2n_client_key_share_extension_size = S2N_SIZE_OF_EXTENSION_TYPE - + S2N_SIZE_OF_EXTENSION_DATA_SIZE - + S2N_SIZE_OF_CLIENT_SHARES_SIZE; - - s2n_client_key_share_extension_size += S2N_SIZE_OF_KEY_SHARE_SIZE + S2N_SIZE_OF_NAMED_GROUP; - s2n_client_key_share_extension_size += ecc_pref->ecc_curves[0]->share_size; - - return s2n_client_key_share_extension_size; -} - int s2n_extensions_client_key_share_recv(struct s2n_connection *conn, struct s2n_stuffer *extension) { return s2n_extension_recv(&s2n_client_key_share_extension, conn, extension); diff --git a/tls/extensions/s2n_client_key_share.h b/tls/extensions/s2n_client_key_share.h index 9da85e789c2..fd570f0c28f 100644 --- a/tls/extensions/s2n_client_key_share.h +++ b/tls/extensions/s2n_client_key_share.h @@ -22,4 +22,3 @@ extern const s2n_extension_type s2n_client_key_share_extension; /* Old-style extension functions -- remove after extensions refactor is complete */ int s2n_extensions_client_key_share_recv(struct s2n_connection *conn, struct s2n_stuffer *extension); -uint32_t s2n_extensions_client_key_share_size(struct s2n_connection *conn); diff --git a/tls/s2n_connection.c b/tls/s2n_connection.c index f3e33c638d8..318afdd72be 100644 --- a/tls/s2n_connection.c +++ b/tls/s2n_connection.c @@ -523,9 +523,8 @@ int s2n_connection_wipe(struct s2n_connection *conn) conn->data_for_verify_host = NULL; /* Clone the stuffers */ - /* ignore gcc 4.7 address warnings because dest is allocated on the stack */ - /* pragma gcc diagnostic was added in gcc 4.6 */ -#if S2N_GCC_VERSION_AT_LEAST(4, 6, 0) + /* ignore address warnings because dest is allocated on the stack */ +#ifdef S2N_DIAGNOSTICS_SUPPORTED #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Waddress" #endif @@ -535,7 +534,7 @@ int s2n_connection_wipe(struct s2n_connection *conn) POSIX_CHECKED_MEMCPY(&header_in, &conn->header_in, sizeof(struct s2n_stuffer)); POSIX_CHECKED_MEMCPY(&in, &conn->in, sizeof(struct s2n_stuffer)); POSIX_CHECKED_MEMCPY(&out, &conn->out, sizeof(struct s2n_stuffer)); -#if S2N_GCC_VERSION_AT_LEAST(4, 6, 0) +#ifdef S2N_DIAGNOSTICS_SUPPORTED #pragma GCC diagnostic pop #endif @@ -1060,21 +1059,37 @@ int s2n_connection_set_blinding(struct s2n_connection *conn, s2n_blinding blindi #define ONE_S INT64_C(1000000000) #define TEN_S INT64_C(10000000000) -uint64_t s2n_connection_get_delay(struct s2n_connection *conn) +static S2N_RESULT s2n_connection_get_delay_impl(struct s2n_connection *conn, uint64_t *delay) { + RESULT_ENSURE_REF(conn); + RESULT_ENSURE_REF(delay); + if (!conn->delay) { - return 0; + *delay = 0; + return S2N_RESULT_OK; } - uint64_t elapsed; - /* This will cast -1 to max uint64_t */ - POSIX_GUARD_RESULT(s2n_timer_elapsed(conn->config, &conn->write_timer, &elapsed)); + uint64_t elapsed = 0; + RESULT_GUARD(s2n_timer_elapsed(conn->config, &conn->write_timer, &elapsed)); if (elapsed > conn->delay) { - return 0; + *delay = 0; + return S2N_RESULT_OK; } - return conn->delay - elapsed; + *delay = conn->delay - elapsed; + + return S2N_RESULT_OK; +} + +uint64_t s2n_connection_get_delay(struct s2n_connection *conn) +{ + uint64_t delay = 0; + if (s2n_result_is_ok(s2n_connection_get_delay_impl(conn, &delay))) { + return delay; + } else { + return UINT64_MAX; + } } static S2N_RESULT s2n_connection_kill(struct s2n_connection *conn) diff --git a/utils/s2n_ensure.h b/utils/s2n_ensure.h index 54b208a35b3..cc81081819a 100644 --- a/utils/s2n_ensure.h +++ b/utils/s2n_ensure.h @@ -79,6 +79,17 @@ } \ } while (0) +#if defined(S2N_DIAGNOSTICS_POP_SUPPORTED) + #define __S2N_ENSURE_CHECKED_RETURN(v) \ + do { \ + _Pragma("GCC diagnostic push") \ + _Pragma("GCC diagnostic error \"-Wconversion\"") return v; \ + _Pragma("GCC diagnostic pop") \ + } while (0) +#else + #define __S2N_ENSURE_CHECKED_RETURN(v) return v +#endif + /** * `restrict` is a part of the c99 standard and will work with any C compiler. If you're trying to * compile with a C++ compiler `restrict` is invalid. However some C++ compilers support the behavior diff --git a/utils/s2n_safety_macros.h b/utils/s2n_safety_macros.h index fb71ef21ee4..41d8a88fab4 100644 --- a/utils/s2n_safety_macros.h +++ b/utils/s2n_safety_macros.h @@ -40,7 +40,7 @@ /** * Sets the global `s2n_errno` to `error` and returns with an `S2N_RESULT_ERROR` */ -#define RESULT_BAIL(error) do { _S2N_ERROR((error)); return S2N_RESULT_ERROR; } while (0) +#define RESULT_BAIL(error) do { _S2N_ERROR((error)); __S2N_ENSURE_CHECKED_RETURN(S2N_RESULT_ERROR); } while (0) /** * Ensures the `condition` is `true`, otherwise the function will `RESULT_BAIL` with `error` @@ -180,7 +180,7 @@ /** * Ensures `s2n_result_is_ok(result)`, otherwise the function will return `S2N_RESULT_ERROR` */ -#define RESULT_GUARD(result) __S2N_ENSURE(s2n_result_is_ok(result), return S2N_RESULT_ERROR) +#define RESULT_GUARD(result) __S2N_ENSURE(s2n_result_is_ok(result), __S2N_ENSURE_CHECKED_RETURN(S2N_RESULT_ERROR)) /** * Ensures `result == _OSSL_SUCCESS`, otherwise the function will `RESULT_BAIL` with `error` @@ -190,21 +190,21 @@ /** * Ensures `(result) > S2N_FAILURE`, otherwise the function will return `S2N_RESULT_ERROR` */ -#define RESULT_GUARD_POSIX(result) __S2N_ENSURE((result) > S2N_FAILURE, return S2N_RESULT_ERROR) +#define RESULT_GUARD_POSIX(result) __S2N_ENSURE((result) > S2N_FAILURE, __S2N_ENSURE_CHECKED_RETURN(S2N_RESULT_ERROR)) /** * Ensures `(result) != NULL`, otherwise the function will return `S2N_RESULT_ERROR` * * Does not set s2n_errno to S2N_ERR_NULL, so is NOT a direct replacement for RESULT_ENSURE_REF. */ -#define RESULT_GUARD_PTR(result) __S2N_ENSURE((result) != NULL, return S2N_RESULT_ERROR) +#define RESULT_GUARD_PTR(result) __S2N_ENSURE((result) != NULL, __S2N_ENSURE_CHECKED_RETURN(S2N_RESULT_ERROR)) /** * DEPRECATED: all methods (except those in s2n.h) should return s2n_result. * * Sets the global `s2n_errno` to `error` and returns with an `S2N_FAILURE` */ -#define POSIX_BAIL(error) do { _S2N_ERROR((error)); return S2N_FAILURE; } while (0) +#define POSIX_BAIL(error) do { _S2N_ERROR((error)); __S2N_ENSURE_CHECKED_RETURN(S2N_FAILURE); } while (0) /** * DEPRECATED: all methods (except those in s2n.h) should return s2n_result. @@ -380,7 +380,7 @@ * * Ensures `(result) > S2N_FAILURE`, otherwise the function will return `S2N_FAILURE` */ -#define POSIX_GUARD(result) __S2N_ENSURE((result) > S2N_FAILURE, return S2N_FAILURE) +#define POSIX_GUARD(result) __S2N_ENSURE((result) > S2N_FAILURE, __S2N_ENSURE_CHECKED_RETURN(S2N_FAILURE)) /** * DEPRECATED: all methods (except those in s2n.h) should return s2n_result. @@ -394,7 +394,7 @@ * * Ensures `s2n_result_is_ok(result)`, otherwise the function will return `S2N_FAILURE` */ -#define POSIX_GUARD_RESULT(result) __S2N_ENSURE(s2n_result_is_ok(result), return S2N_FAILURE) +#define POSIX_GUARD_RESULT(result) __S2N_ENSURE(s2n_result_is_ok(result), __S2N_ENSURE_CHECKED_RETURN(S2N_FAILURE)) /** * DEPRECATED: all methods (except those in s2n.h) should return s2n_result. @@ -403,14 +403,14 @@ * * Does not set s2n_errno to S2N_ERR_NULL, so is NOT a direct replacement for POSIX_ENSURE_REF. */ -#define POSIX_GUARD_PTR(result) __S2N_ENSURE((result) != NULL, return S2N_FAILURE) +#define POSIX_GUARD_PTR(result) __S2N_ENSURE((result) != NULL, __S2N_ENSURE_CHECKED_RETURN(S2N_FAILURE)) /** * DEPRECATED: all methods (except those in s2n.h) should return s2n_result. * * Sets the global `s2n_errno` to `error` and returns with an `NULL` */ -#define PTR_BAIL(error) do { _S2N_ERROR((error)); return NULL; } while (0) +#define PTR_BAIL(error) do { _S2N_ERROR((error)); __S2N_ENSURE_CHECKED_RETURN(NULL); } while (0) /** * DEPRECATED: all methods (except those in s2n.h) should return s2n_result. @@ -586,7 +586,7 @@ * * Ensures `(result) != NULL`, otherwise the function will return `NULL` */ -#define PTR_GUARD(result) __S2N_ENSURE((result) != NULL, return NULL) +#define PTR_GUARD(result) __S2N_ENSURE((result) != NULL, __S2N_ENSURE_CHECKED_RETURN(NULL)) /** * DEPRECATED: all methods (except those in s2n.h) should return s2n_result. @@ -600,12 +600,12 @@ * * Ensures `s2n_result_is_ok(result)`, otherwise the function will return `NULL` */ -#define PTR_GUARD_RESULT(result) __S2N_ENSURE(s2n_result_is_ok(result), return NULL) +#define PTR_GUARD_RESULT(result) __S2N_ENSURE(s2n_result_is_ok(result), __S2N_ENSURE_CHECKED_RETURN(NULL)) /** * DEPRECATED: all methods (except those in s2n.h) should return s2n_result. * * Ensures `(result) > S2N_FAILURE`, otherwise the function will return `NULL` */ -#define PTR_GUARD_POSIX(result) __S2N_ENSURE((result) > S2N_FAILURE, return NULL) +#define PTR_GUARD_POSIX(result) __S2N_ENSURE((result) > S2N_FAILURE, __S2N_ENSURE_CHECKED_RETURN(NULL)) From 26e57f123c719d7519ee93277633269aa261f4b5 Mon Sep 17 00:00:00 2001 From: Cameron Bytheway Date: Mon, 12 Jun 2023 10:29:06 -0600 Subject: [PATCH 2/2] make push/pop support separate --- crypto/s2n_ecdsa.c | 4 ++-- crypto/s2n_rsa.c | 4 ++-- .../features/S2N_DIAGNOSTICS_POP_SUPPORTED.c | 5 ++++- ...TED.c => S2N_DIAGNOSTICS_PUSH_SUPPORTED.c} | 19 +++++++++++++++---- .../S2N_DIAGNOSTICS_PUSH_SUPPORTED.flags | 1 + .../features/S2N_DIAGNOSTICS_SUPPORTED.flags | 1 - tls/s2n_connection.c | 4 ++-- utils/s2n_ensure.h | 2 +- 8 files changed, 27 insertions(+), 13 deletions(-) rename tests/features/{S2N_DIAGNOSTICS_SUPPORTED.c => S2N_DIAGNOSTICS_PUSH_SUPPORTED.c} (69%) create mode 100644 tests/features/S2N_DIAGNOSTICS_PUSH_SUPPORTED.flags delete mode 100644 tests/features/S2N_DIAGNOSTICS_SUPPORTED.flags diff --git a/crypto/s2n_ecdsa.c b/crypto/s2n_ecdsa.c index 9fbc75049a2..2761f933272 100644 --- a/crypto/s2n_ecdsa.c +++ b/crypto/s2n_ecdsa.c @@ -40,12 +40,12 @@ EC_KEY *s2n_unsafe_ecdsa_get_non_const(const struct s2n_ecdsa_key *ecdsa_key) { PTR_ENSURE_REF(ecdsa_key); -#ifdef S2N_DIAGNOSTICS_SUPPORTED +#ifdef S2N_DIAGNOSTICS_PUSH_SUPPORTED #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wcast-qual" #endif EC_KEY *out_ec_key = (EC_KEY *) ecdsa_key->ec_key; -#ifdef S2N_DIAGNOSTICS_SUPPORTED +#ifdef S2N_DIAGNOSTICS_POP_SUPPORTED #pragma GCC diagnostic pop #endif diff --git a/crypto/s2n_rsa.c b/crypto/s2n_rsa.c index 4bfd698f83f..96ce8f41406 100644 --- a/crypto/s2n_rsa.c +++ b/crypto/s2n_rsa.c @@ -36,12 +36,12 @@ RSA *s2n_unsafe_rsa_get_non_const(const struct s2n_rsa_key *rsa_key) { PTR_ENSURE_REF(rsa_key); -#ifdef S2N_DIAGNOSTICS_SUPPORTED +#ifdef S2N_DIAGNOSTICS_PUSH_SUPPORTED #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wcast-qual" #endif RSA *out_rsa_key = (RSA *) rsa_key->rsa; -#ifdef S2N_DIAGNOSTICS_SUPPORTED +#ifdef S2N_DIAGNOSTICS_POP_SUPPORTED #pragma GCC diagnostic pop #endif diff --git a/tests/features/S2N_DIAGNOSTICS_POP_SUPPORTED.c b/tests/features/S2N_DIAGNOSTICS_POP_SUPPORTED.c index a7c9d2be341..d4997552d4e 100644 --- a/tests/features/S2N_DIAGNOSTICS_POP_SUPPORTED.c +++ b/tests/features/S2N_DIAGNOSTICS_POP_SUPPORTED.c @@ -32,7 +32,10 @@ int signed_fun() MACRO_CHECK; } -/* this function is here to ensure the compiler properly pops the previous diagnostic */ +/* This function is here to ensure the compiler properly pops the previous diagnostic. + * + * GCC 4 and lower don't correctly pop diagnostics so this will fail on those systems. + **/ uint8_t unsigned_fun() { return -1; diff --git a/tests/features/S2N_DIAGNOSTICS_SUPPORTED.c b/tests/features/S2N_DIAGNOSTICS_PUSH_SUPPORTED.c similarity index 69% rename from tests/features/S2N_DIAGNOSTICS_SUPPORTED.c rename to tests/features/S2N_DIAGNOSTICS_PUSH_SUPPORTED.c index 79918890534..ecd1157e4de 100644 --- a/tests/features/S2N_DIAGNOSTICS_SUPPORTED.c +++ b/tests/features/S2N_DIAGNOSTICS_PUSH_SUPPORTED.c @@ -15,19 +15,30 @@ #include +/* make sure we can call `_Pragma` in macros */ #define MACRO_CHECK \ do { \ _Pragma("GCC diagnostic push") \ - _Pragma("GCC diagnostic error \"-Wconversion\"") \ + _Pragma("GCC diagnostic ignored \"-Wsign-conversion\"") \ return -1; \ - _Pragma("GCC diagnostic pop") \ } while (0) +uint8_t unsigned_fun() +{ + MACRO_CHECK; +} + int main() { + const int value = 0; + const int *value_ptr = &value; + + unsigned_fun(); + + /* make sure we can also push diagnostics via `#pragma` */ #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wcast-qual" - #pragma GCC diagnostic pop + int *value_ptr_mut = (int*)value_ptr; - MACRO_CHECK; + return *value_ptr_mut; } diff --git a/tests/features/S2N_DIAGNOSTICS_PUSH_SUPPORTED.flags b/tests/features/S2N_DIAGNOSTICS_PUSH_SUPPORTED.flags new file mode 100644 index 00000000000..0e62f85a8a6 --- /dev/null +++ b/tests/features/S2N_DIAGNOSTICS_PUSH_SUPPORTED.flags @@ -0,0 +1 @@ +-Werror -Wconversion -Wcast-qual diff --git a/tests/features/S2N_DIAGNOSTICS_SUPPORTED.flags b/tests/features/S2N_DIAGNOSTICS_SUPPORTED.flags deleted file mode 100644 index 2f41be663b2..00000000000 --- a/tests/features/S2N_DIAGNOSTICS_SUPPORTED.flags +++ /dev/null @@ -1 +0,0 @@ --Werror diff --git a/tls/s2n_connection.c b/tls/s2n_connection.c index 318afdd72be..285ecd948ce 100644 --- a/tls/s2n_connection.c +++ b/tls/s2n_connection.c @@ -524,7 +524,7 @@ int s2n_connection_wipe(struct s2n_connection *conn) /* Clone the stuffers */ /* ignore address warnings because dest is allocated on the stack */ -#ifdef S2N_DIAGNOSTICS_SUPPORTED +#ifdef S2N_DIAGNOSTICS_PUSH_SUPPORTED #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Waddress" #endif @@ -534,7 +534,7 @@ int s2n_connection_wipe(struct s2n_connection *conn) POSIX_CHECKED_MEMCPY(&header_in, &conn->header_in, sizeof(struct s2n_stuffer)); POSIX_CHECKED_MEMCPY(&in, &conn->in, sizeof(struct s2n_stuffer)); POSIX_CHECKED_MEMCPY(&out, &conn->out, sizeof(struct s2n_stuffer)); -#ifdef S2N_DIAGNOSTICS_SUPPORTED +#ifdef S2N_DIAGNOSTICS_POP_SUPPORTED #pragma GCC diagnostic pop #endif diff --git a/utils/s2n_ensure.h b/utils/s2n_ensure.h index cc81081819a..4fe117f14db 100644 --- a/utils/s2n_ensure.h +++ b/utils/s2n_ensure.h @@ -79,7 +79,7 @@ } \ } while (0) -#if defined(S2N_DIAGNOSTICS_POP_SUPPORTED) +#if defined(S2N_DIAGNOSTICS_PUSH_SUPPORTED) && defined(S2N_DIAGNOSTICS_POP_SUPPORTED) #define __S2N_ENSURE_CHECKED_RETURN(v) \ do { \ _Pragma("GCC diagnostic push") \