feat: add checked return values diagnostic (#3798)

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_PUSH_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_POP_SUPPORTED
     #pragma GCC diagnostic pop
 #endif
 

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_PUSH_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_POP_SUPPORTED
     #pragma GCC diagnostic pop
 #endif
 

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'],

tests/features/S2N_DIAGNOSTICS_POP_SUPPORTED.c

@@ -0,0 +1,50 @@
+/*
+ * 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 <stdint.h>
+
+#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.
+ *
+ * GCC 4 and lower don't correctly pop diagnostics so this will fail on those systems.
+ **/
+uint8_t unsigned_fun()
+{
+    return -1;
+}
+
+int main()
+{
+    signed_fun();
+    unsigned_fun();
+
+    MACRO_CHECK;
+}

tests/features/S2N_DIAGNOSTICS_POP_SUPPORTED.flags

@@ -0,0 +1 @@
+-Werror

tests/features/S2N_DIAGNOSTICS_PUSH_SUPPORTED.c

@@ -0,0 +1,44 @@
+/*
+ * 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 <stdint.h>
+
+/* make sure we can call `_Pragma` in macros */
+#define MACRO_CHECK \
+    do { \
+        _Pragma("GCC diagnostic push") \
+        _Pragma("GCC diagnostic ignored \"-Wsign-conversion\"") \
+        return -1; \
+    } 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"
+    int *value_ptr_mut = (int*)value_ptr;
+
+    return *value_ptr_mut;
+}

tests/features/S2N_DIAGNOSTICS_PUSH_SUPPORTED.flags

@@ -0,0 +1 @@
+-Werror -Wconversion -Wcast-qual

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,

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 */

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);

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);

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_PUSH_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_POP_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)

utils/s2n_ensure.h

@@ -79,6 +79,17 @@
         }                                        \
     } while (0)
 
+#if defined(S2N_DIAGNOSTICS_PUSH_SUPPORTED) && 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