refactor: make s2n_array_len constant (#4801)

Author: lrstewart

codebuild/bin/grep_simple_mistakes.sh

@@ -93,11 +93,10 @@ done
 #############################################
 S2N_FILES_ARRAY_SIZING_RETURN=$(find "$PWD" -type f -name "s2n*.c" -path "*")
 for file in $S2N_FILES_ARRAY_SIZING_RETURN; do
-  RESULT_ARR_DIV=`grep -Ern 'sizeof\((.*)\) \/ sizeof\(\1\[0\]\)' $file`
-
+  RESULT_ARR_DIV=`grep -Ern 'sizeof\(.*?\) / sizeof\(' $file`
   if [ "${#RESULT_ARR_DIV}" != "0" ]; then
     FAILED=1
-    printf "\e[1;34mUsage of 'sizeof(array) / sizeof(array[0])' check failed. Use s2n_array_len(array) instead in $file:\e[0m\n$RESULT_ARR_DIV\n\n"
+    printf "\e[1;34mUsage of 'sizeof(array) / sizeof(T)' check failed. Use s2n_array_len instead in $file:\e[0m\n$RESULT_ARR_DIV\n\n"
   fi
 done
 

tests/unit/s2n_certificate_parsing_test.c

@@ -77,7 +77,7 @@ int main(int argc, char **argv)
         CERTIFICATE_2,
         CERTIFICATE_3,
     };
-    struct s2n_blob expected_certs[sizeof(expected_cert_strs) / sizeof(char *)] = { 0 };
+    struct s2n_blob expected_certs[s2n_array_len(expected_cert_strs)] = { 0 };
     EXPECT_EQUAL(s2n_array_len(expected_certs), s2n_array_len(expected_cert_strs));
 
     for (size_t i = 0; i < s2n_array_len(expected_certs); i++) {

tests/unit/s2n_safety_test.c

@@ -445,6 +445,18 @@ int main(int argc, char **argv)
         }
     }
 
+    /* Test: s2n_array_len */
+    {
+        /* Must return correct length */
+        uint16_t test_data[10] = { 0 };
+        EXPECT_EQUAL(s2n_array_len(test_data), 10);
+        EXPECT_NOT_EQUAL(s2n_array_len(test_data), sizeof(test_data));
+
+        /* Must be usable as an array size / constant expression */
+        uint16_t test_data_dup[s2n_array_len(test_data)] = { 0 };
+        EXPECT_EQUAL(sizeof(test_data), sizeof(test_data_dup));
+    }
+
     END_TEST();
     return 0;
 }

tests/unit/s2n_server_alpn_extension_test.c

@@ -120,6 +120,32 @@ int main(int argc, char **argv)
         EXPECT_SUCCESS(s2n_connection_free(server_conn));
     };
 
+    /* Send and receive maximum-sized alpn */
+    {
+        const uint8_t max_size = UINT8_MAX;
+
+        DEFER_CLEANUP(struct s2n_connection *client = s2n_connection_new(S2N_CLIENT),
+                s2n_connection_ptr_free);
+        EXPECT_NOT_NULL(client);
+        EXPECT_NULL(s2n_get_application_protocol(client));
+
+        DEFER_CLEANUP(struct s2n_connection *server = s2n_connection_new(S2N_SERVER),
+                s2n_connection_ptr_free);
+        EXPECT_NOT_NULL(server);
+        memset(server->application_protocol, 'a', sizeof(server->application_protocol) - 1);
+
+        DEFER_CLEANUP(struct s2n_stuffer stuffer = { 0 }, s2n_stuffer_free);
+        EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&stuffer, 0));
+        EXPECT_SUCCESS(s2n_server_alpn_extension.send(server, &stuffer));
+        EXPECT_SUCCESS(s2n_server_alpn_extension.recv(client, &stuffer));
+        EXPECT_EQUAL(s2n_stuffer_data_available(&stuffer), 0);
+
+        const char *client_alpn = s2n_get_application_protocol(client);
+        EXPECT_NOT_NULL(client_alpn);
+        EXPECT_EQUAL(strlen(client_alpn), max_size);
+        EXPECT_STRING_EQUAL(client_alpn, server->application_protocol);
+    };
+
     END_TEST();
     return 0;
 }

tls/extensions/s2n_server_alpn.c

@@ -66,7 +66,6 @@ static int s2n_alpn_recv(struct s2n_connection *conn, struct s2n_stuffer *extens
 
     uint8_t protocol_len = 0;
     POSIX_GUARD(s2n_stuffer_read_uint8(extension, &protocol_len));
-    POSIX_ENSURE_LT(protocol_len, s2n_array_len(conn->application_protocol));
 
     uint8_t *protocol = s2n_stuffer_raw_read(extension, protocol_len);
     POSIX_ENSURE_REF(protocol);

tls/s2n_cipher_suites.c

@@ -1068,7 +1068,7 @@ int s2n_cipher_suites_init(void)
 /* Reset any selected record algorithms */
 S2N_RESULT s2n_cipher_suites_cleanup(void)
 {
-    const int num_cipher_suites = sizeof(s2n_all_cipher_suites) / sizeof(struct s2n_cipher_suite *);
+    const int num_cipher_suites = s2n_array_len(s2n_all_cipher_suites);
     for (int i = 0; i < num_cipher_suites; i++) {
         struct s2n_cipher_suite *cur_suite = s2n_all_cipher_suites[i];
         cur_suite->available = 0;

utils/s2n_safety.h

@@ -97,7 +97,14 @@ S2N_CLEANUP_RESULT s2n_connection_apply_error_blinding(struct s2n_connection** c
     }                                           \
     struct __useless_struct_to_allow_trailing_semicolon__
 
-#define s2n_array_len(array) ((array != NULL) ? (sizeof(array) / sizeof(array[0])) : 0)
+/* This method works for ARRAYS, not for POINTERS.
+ * Calling sizeof on an array declared in the current function correctly returns
+ * the total size of the array. But once the array is passed to another function,
+ * it behaves like a pointer. Calling sizeof on a pointer only returns the size
+ * of the pointer address itself (so usually 8).
+ * Newer compilers (gcc >= 8.1, clang >= 8.0) will warn if the argument is a pointer.
+ */
+#define s2n_array_len(array) (sizeof(array) / sizeof(array[0]))
 
 int s2n_mul_overflow(uint32_t a, uint32_t b, uint32_t* out);