refactor: clang-tidy null deref and undefined mod (#4436)

aws · Mar 14, 2024 · f14659d · f14659d
1 parent b1bb5dc
commit f14659d
Show file tree

Hide file tree

Showing 13 changed files with 22 additions and 27 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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];