From f14659de6ba39137e722f89bc23b0022cabe338f Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Wed, 13 Mar 2024 17:17:48 -0700 Subject: [PATCH] refactor: clang-tidy null deref and undefined mod (#4436) --- crypto/s2n_pkey.c | 6 +++++- stuffer/s2n_stuffer_network_order.c | 8 ++++++-- .../s2n_dh_compute_shared_secret_as_client/Makefile | 2 +- tests/cbmc/proofs/s2n_dh_params_to_p_g_Ys/Makefile | 2 +- .../cbmc/proofs/s2n_stuffer_write_network_order/Makefile | 2 +- tests/cbmc/proofs/s2n_stuffer_write_reservation/Makefile | 9 +-------- tests/cbmc/proofs/s2n_stuffer_write_uint16/Makefile | 2 +- tests/cbmc/proofs/s2n_stuffer_write_uint24/Makefile | 2 +- tests/cbmc/proofs/s2n_stuffer_write_uint32/Makefile | 2 +- tests/cbmc/proofs/s2n_stuffer_write_uint64/Makefile | 2 +- tests/cbmc/proofs/s2n_stuffer_write_vector_size/Makefile | 9 +-------- tls/s2n_security_rules.c | 2 +- utils/s2n_map.c | 1 + 13 files changed, 22 insertions(+), 27 deletions(-) diff --git a/crypto/s2n_pkey.c b/crypto/s2n_pkey.c index 190c73e303a..bcc57c42422 100644 --- a/crypto/s2n_pkey.c +++ b/crypto/s2n_pkey.c @@ -115,7 +115,11 @@ int s2n_pkey_match(const struct s2n_pkey *pub_key, const struct s2n_pkey *priv_k int s2n_pkey_free(struct s2n_pkey *key) { - if (key != NULL && key->free != NULL) { + if (key == NULL) { + return S2N_SUCCESS; + } + + if (key->free != NULL) { POSIX_GUARD(key->free(key)); } diff --git a/stuffer/s2n_stuffer_network_order.c b/stuffer/s2n_stuffer_network_order.c index 9db807a60cb..88927091eb2 100644 --- a/stuffer/s2n_stuffer_network_order.c +++ b/stuffer/s2n_stuffer_network_order.c @@ -21,10 +21,14 @@ /* Writes length bytes of input to stuffer, in network order, starting from the smallest byte of input. */ int s2n_stuffer_write_network_order(struct s2n_stuffer *stuffer, const uint64_t input, const uint8_t length) { + if (length == 0) { + return S2N_SUCCESS; + } + POSIX_ENSURE_REF(stuffer); POSIX_ENSURE(length <= sizeof(input), S2N_ERR_SAFETY); POSIX_GUARD(s2n_stuffer_skip_write(stuffer, length)); - uint8_t *data = (stuffer->blob.data) ? (stuffer->blob.data + stuffer->write_cursor - length) : NULL; - + POSIX_ENSURE_REF(stuffer->blob.data); + uint8_t *data = stuffer->blob.data + stuffer->write_cursor - length; for (int i = 0; i < length; i++) { S2N_INVARIANT(i <= length); uint8_t shift = (length - i - 1) * CHAR_BIT; diff --git a/tests/cbmc/proofs/s2n_dh_compute_shared_secret_as_client/Makefile b/tests/cbmc/proofs/s2n_dh_compute_shared_secret_as_client/Makefile index ec8ef4fc519..cb7a239897c 100644 --- a/tests/cbmc/proofs/s2n_dh_compute_shared_secret_as_client/Makefile +++ b/tests/cbmc/proofs/s2n_dh_compute_shared_secret_as_client/Makefile @@ -39,7 +39,7 @@ PROJECT_SOURCES += $(SRCDIR)/utils/s2n_mem.c PROJECT_SOURCES += $(SRCDIR)/utils/s2n_result.c PROJECT_SOURCES += $(SRCDIR)/utils/s2n_safety.c -UNWINDSET += s2n_stuffer_write_network_order.4:9 +UNWINDSET += s2n_stuffer_write_network_order.10:9 # We abstract this function because manual inspection demonstrates it is unreachable. REMOVE_FUNCTION_BODY += __CPROVER_file_local_s2n_mem_c_s2n_mem_cleanup_impl diff --git a/tests/cbmc/proofs/s2n_dh_params_to_p_g_Ys/Makefile b/tests/cbmc/proofs/s2n_dh_params_to_p_g_Ys/Makefile index 94ca0745414..33bba982d1f 100644 --- a/tests/cbmc/proofs/s2n_dh_params_to_p_g_Ys/Makefile +++ b/tests/cbmc/proofs/s2n_dh_params_to_p_g_Ys/Makefile @@ -42,6 +42,6 @@ REMOVE_FUNCTION_BODY += s2n_free REMOVE_FUNCTION_BODY += s2n_realloc REMOVE_FUNCTION_BODY += s2n_stuffer_wipe -UNWINDSET += s2n_stuffer_write_network_order.4:9 +UNWINDSET += s2n_stuffer_write_network_order.10:9 include ../Makefile.common diff --git a/tests/cbmc/proofs/s2n_stuffer_write_network_order/Makefile b/tests/cbmc/proofs/s2n_stuffer_write_network_order/Makefile index 4c8f38b8e79..ba093b9041a 100644 --- a/tests/cbmc/proofs/s2n_stuffer_write_network_order/Makefile +++ b/tests/cbmc/proofs/s2n_stuffer_write_network_order/Makefile @@ -38,7 +38,7 @@ REMOVE_FUNCTION_BODY += __CPROVER_file_local_s2n_mem_c_s2n_mem_cleanup_impl REMOVE_FUNCTION_BODY += s2n_blob_slice REMOVE_FUNCTION_BODY += s2n_stuffer_wipe -UNWINDSET += s2n_stuffer_write_network_order.4:9 +UNWINDSET += s2n_stuffer_write_network_order.10:9 UNWINDSET += s2n_stuffer_write_network_order_harness.0:9 include ../Makefile.common diff --git a/tests/cbmc/proofs/s2n_stuffer_write_reservation/Makefile b/tests/cbmc/proofs/s2n_stuffer_write_reservation/Makefile index fe8370b37f7..bc921b1d3ee 100644 --- a/tests/cbmc/proofs/s2n_stuffer_write_reservation/Makefile +++ b/tests/cbmc/proofs/s2n_stuffer_write_reservation/Makefile @@ -48,13 +48,6 @@ REMOVE_FUNCTION_BODY += s2n_stuffer_wipe REMOVE_FUNCTION_BODY += s2n_stuffer_wipe_n REMOVE_FUNCTION_BODY += __CPROVER_file_local_s2n_mem_c_s2n_mem_cleanup_impl -UNWINDSET += s2n_stuffer_write_network_order.0:$(MAX_WRITE_NETWORK_ORDER) -UNWINDSET += s2n_stuffer_write_network_order.1:$(MAX_WRITE_NETWORK_ORDER) -UNWINDSET += s2n_stuffer_write_network_order.2:$(MAX_WRITE_NETWORK_ORDER) -UNWINDSET += s2n_stuffer_write_network_order.3:$(MAX_WRITE_NETWORK_ORDER) -UNWINDSET += s2n_stuffer_write_network_order.4:$(MAX_WRITE_NETWORK_ORDER) -UNWINDSET += s2n_stuffer_write_network_order.5:$(MAX_WRITE_NETWORK_ORDER) -UNWINDSET += s2n_stuffer_write_network_order.6:$(MAX_WRITE_NETWORK_ORDER) -UNWINDSET += s2n_stuffer_write_network_order.7:$(MAX_WRITE_NETWORK_ORDER) +UNWINDSET += s2n_stuffer_write_network_order.10:$(MAX_WRITE_NETWORK_ORDER) include ../Makefile.common diff --git a/tests/cbmc/proofs/s2n_stuffer_write_uint16/Makefile b/tests/cbmc/proofs/s2n_stuffer_write_uint16/Makefile index 73c61bcecad..de513abf4e9 100644 --- a/tests/cbmc/proofs/s2n_stuffer_write_uint16/Makefile +++ b/tests/cbmc/proofs/s2n_stuffer_write_uint16/Makefile @@ -36,6 +36,6 @@ PROJECT_SOURCES += $(SRCDIR)/utils/s2n_result.c REMOVE_FUNCTION_BODY += s2n_blob_slice REMOVE_FUNCTION_BODY += s2n_stuffer_wipe -UNWINDSET += s2n_stuffer_write_network_order.4:$(shell echo $$((1 + $(SIZEOF_UINT16)))) +UNWINDSET += s2n_stuffer_write_network_order.10:$(shell echo $$((1 + $(SIZEOF_UINT16)))) include ../Makefile.common diff --git a/tests/cbmc/proofs/s2n_stuffer_write_uint24/Makefile b/tests/cbmc/proofs/s2n_stuffer_write_uint24/Makefile index acba5cf7b1e..0ee330db573 100644 --- a/tests/cbmc/proofs/s2n_stuffer_write_uint24/Makefile +++ b/tests/cbmc/proofs/s2n_stuffer_write_uint24/Makefile @@ -36,6 +36,6 @@ PROJECT_SOURCES += $(SRCDIR)/utils/s2n_result.c REMOVE_FUNCTION_BODY += s2n_blob_slice REMOVE_FUNCTION_BODY += s2n_stuffer_wipe -UNWINDSET += s2n_stuffer_write_network_order.4:$(shell echo $$((1 + $(SIZEOF_UINT24)))) +UNWINDSET += s2n_stuffer_write_network_order.10:$(shell echo $$((1 + $(SIZEOF_UINT24)))) include ../Makefile.common diff --git a/tests/cbmc/proofs/s2n_stuffer_write_uint32/Makefile b/tests/cbmc/proofs/s2n_stuffer_write_uint32/Makefile index aece6e2f0d0..bec46adf7eb 100644 --- a/tests/cbmc/proofs/s2n_stuffer_write_uint32/Makefile +++ b/tests/cbmc/proofs/s2n_stuffer_write_uint32/Makefile @@ -36,6 +36,6 @@ PROJECT_SOURCES += $(SRCDIR)/utils/s2n_result.c REMOVE_FUNCTION_BODY += s2n_blob_slice REMOVE_FUNCTION_BODY += s2n_stuffer_wipe -UNWINDSET += s2n_stuffer_write_network_order.4:$(shell echo $$((1 + $(SIZEOF_UINT32)))) +UNWINDSET += s2n_stuffer_write_network_order.10:$(shell echo $$((1 + $(SIZEOF_UINT32)))) include ../Makefile.common diff --git a/tests/cbmc/proofs/s2n_stuffer_write_uint64/Makefile b/tests/cbmc/proofs/s2n_stuffer_write_uint64/Makefile index 04f1c6b5aae..8e339851469 100644 --- a/tests/cbmc/proofs/s2n_stuffer_write_uint64/Makefile +++ b/tests/cbmc/proofs/s2n_stuffer_write_uint64/Makefile @@ -36,6 +36,6 @@ PROJECT_SOURCES += $(SRCDIR)/utils/s2n_result.c REMOVE_FUNCTION_BODY += s2n_blob_slice REMOVE_FUNCTION_BODY += s2n_stuffer_wipe -UNWINDSET += s2n_stuffer_write_network_order.4:$(shell echo $$((1 + $(SIZEOF_UINT64)))) +UNWINDSET += s2n_stuffer_write_network_order.10:$(shell echo $$((1 + $(SIZEOF_UINT64)))) include ../Makefile.common diff --git a/tests/cbmc/proofs/s2n_stuffer_write_vector_size/Makefile b/tests/cbmc/proofs/s2n_stuffer_write_vector_size/Makefile index cf3fc56cd15..350684a0681 100644 --- a/tests/cbmc/proofs/s2n_stuffer_write_vector_size/Makefile +++ b/tests/cbmc/proofs/s2n_stuffer_write_vector_size/Makefile @@ -44,13 +44,6 @@ REMOVE_FUNCTION_BODY += s2n_stuffer_resize REMOVE_FUNCTION_BODY += s2n_stuffer_wipe_n REMOVE_FUNCTION_BODY += __CPROVER_file_local_s2n_mem_c_s2n_mem_cleanup_impl -UNWINDSET += s2n_stuffer_write_network_order.0:$(MAX_WRITE_NETWORK_ORDER) -UNWINDSET += s2n_stuffer_write_network_order.1:$(MAX_WRITE_NETWORK_ORDER) -UNWINDSET += s2n_stuffer_write_network_order.2:$(MAX_WRITE_NETWORK_ORDER) -UNWINDSET += s2n_stuffer_write_network_order.3:$(MAX_WRITE_NETWORK_ORDER) -UNWINDSET += s2n_stuffer_write_network_order.4:$(MAX_WRITE_NETWORK_ORDER) -UNWINDSET += s2n_stuffer_write_network_order.5:$(MAX_WRITE_NETWORK_ORDER) -UNWINDSET += s2n_stuffer_write_network_order.6:$(MAX_WRITE_NETWORK_ORDER) -UNWINDSET += s2n_stuffer_write_network_order.7:$(MAX_WRITE_NETWORK_ORDER) +UNWINDSET += s2n_stuffer_write_network_order.10:$(MAX_WRITE_NETWORK_ORDER) include ../Makefile.common diff --git a/tls/s2n_security_rules.c b/tls/s2n_security_rules.c index ebeefe99430..2d1513d3909 100644 --- a/tls/s2n_security_rules.c +++ b/tls/s2n_security_rules.c @@ -219,7 +219,7 @@ S2N_CLEANUP_RESULT s2n_security_rule_result_free(struct s2n_security_rule_result { if (result) { RESULT_GUARD_POSIX(s2n_stuffer_free(&result->output)); + *result = (struct s2n_security_rule_result){ 0 }; } - *result = (struct s2n_security_rule_result){ 0 }; return S2N_RESULT_OK; } diff --git a/utils/s2n_map.c b/utils/s2n_map.c index b1ad6721c09..ee8bafd7c32 100644 --- a/utils/s2n_map.c +++ b/utils/s2n_map.c @@ -32,6 +32,7 @@ static S2N_RESULT s2n_map_slot(const struct s2n_map *map, struct s2n_blob *key, uint32_t *slot) { RESULT_ENSURE_REF(map); + RESULT_ENSURE(map->capacity != 0, S2N_ERR_SAFETY); union { uint8_t u8[32]; uint32_t u32[8];