Skip to content

Commit

Permalink
feat: add checked return values diagnostic (aws#3798)
Browse files Browse the repository at this point in the history
  • Loading branch information
camshaft authored and dougch committed Jun 13, 2023
1 parent 5b08e17 commit 9864a27
Show file tree
Hide file tree
Showing 14 changed files with 177 additions and 55 deletions.
5 changes: 2 additions & 3 deletions crypto/s2n_ecdsa.c
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
5 changes: 2 additions & 3 deletions crypto/s2n_rsa.c
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
6 changes: 3 additions & 3 deletions scripts/s2n_safety_macros.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
{{
Expand Down Expand Up @@ -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)
{{
Expand Down Expand Up @@ -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'],
Expand Down
50 changes: 50 additions & 0 deletions tests/features/S2N_DIAGNOSTICS_POP_SUPPORTED.c
Original file line number Diff line number Diff line change
@@ -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;
}
1 change: 1 addition & 0 deletions tests/features/S2N_DIAGNOSTICS_POP_SUPPORTED.flags
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
-Werror
44 changes: 44 additions & 0 deletions tests/features/S2N_DIAGNOSTICS_PUSH_SUPPORTED.c
Original file line number Diff line number Diff line change
@@ -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;
}
1 change: 1 addition & 0 deletions tests/features/S2N_DIAGNOSTICS_PUSH_SUPPORTED.flags
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
-Werror -Wconversion -Wcast-qual
1 change: 1 addition & 0 deletions tests/sidetrail/working/stubs/s2n_ensure.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
28 changes: 24 additions & 4 deletions tests/unit/s2n_client_key_share_extension_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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 */
Expand Down
18 changes: 0 additions & 18 deletions tls/extensions/s2n_client_key_share.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
1 change: 0 additions & 1 deletion tls/extensions/s2n_client_key_share.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
37 changes: 26 additions & 11 deletions tls/s2n_connection.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Expand Down Expand Up @@ -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)
Expand Down
11 changes: 11 additions & 0 deletions utils/s2n_ensure.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading

0 comments on commit 9864a27

Please sign in to comment.