refactor: make s2n_array_len constant

[lrstewart]

Description of changes:

Our grep_simple_mistakes script requires that we use s2n_array_len everywhere instead of (sizeof(array) / sizeof(array[0])), but s2n_array_len is no longer defined as (sizeof(array) / sizeof(array[0])), so it's no longer constant. That means that you can't do like:

struct s2n_blob array1[100] = { 0 };
struct s2n_blob array2[s2n_array_len(array1)] = { 0 };

You could do (sizeof(array) / sizeof(struct s2n_blob)) as a workaround, but that's unnecessary and more brittle. We do it in multiple places though, so I modified our grep_simple_mistakes script to catch it.

The null check is also unnecessary because arrays can't be null. Only pointers can be null, and s2n_array_len doesn't work on pointers anyway. Instead, we should check that what was passed to s2n_array_len is actually an array. Doing that is a little tricky: I got a hacky solution working using __builtin_types_compatible_p and a BUILD_BUG_ON_ZERO-style compiler assert... then I realized that modern compilers will actually just check for you. So that's nice.

Testing:

If you call s2n_array_len with a pointer on more recent compiler versions (see comment on s2n_array_len), you get this error:

/home/ubuntu/s2n-tls/tests/unit/s2n_safety_test.c: In function ‘main’:
/home/ubuntu/s2n-tls/./utils/s2n_safety.h:121:20: warning: division ‘sizeof (uint16_t * {aka short unsigned int *}) / sizeof (uint16_t {aka short unsigned int})’ does not compute the number of array elements [-Wsizeof-pointer-div]
  121 |     (sizeof(array) / sizeof(array[0]) + \

Here's an godbolt to play with: https://godbolt.org/z/9MooK8z43

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[lrstewart]

This now leads to a compiler error:

   /home/runner/work/s2n-tls/s2n-tls/tls/extensions/s2n_server_alpn.c:69:5: error: result of comparison of constant 256 with expression of type 'uint8_t' (aka 'unsigned char') is always true [-Werror,-Wtautological-constant-out-of-range-compare]
     69 |     POSIX_ENSURE_LT(protocol_len, s2n_array_len(conn->application_protocol));
        |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  /home/runner/work/s2n-tls/s2n-tls/utils/s2n_safety_macros.h:261:80: note: expanded from macro 'POSIX_ENSURE_LT'
    261 | #define POSIX_ENSURE_LT(a, b)                                 __S2N_ENSURE((a) < (b), POSIX_BAIL(S2N_ERR_SAFETY))
        |                                                               ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  /home/runner/work/s2n-tls/s2n-tls/utils/s2n_ensure.h:35:15: note: expanded from macro '__S2N_ENSURE'
     35 |         if (!(cond)) {             \
        |               ^~~~
  1 error generated.

I added a test instead.

[camshaft]

hmm... i wonder how we can preserve this check, though. in theory the conn->application_protocol size could be reduced and then we'd have an overwrite. this check would at least avoid that issue.

[lrstewart]

The new unit test I added should actually enforce that. The test I wrote sends an alpn based on the size of conn->application_protocol:

s2n-tls/tests/unit/s2n_server_alpn_extension_test.c

Lines 132 to 135 in c0a43a1

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

But then it checks that the final result matches the known max size, independent of the size of conn->application_protocol:

s2n-tls/tests/unit/s2n_server_alpn_extension_test.c

Lines 143 to 145 in c0a43a1

	const char *client_alpn = s2n_get_application_protocol(client);
	EXPECT_NOT_NULL(client_alpn);
	EXPECT_EQUAL(strlen(client_alpn), max_size);

So if conn->application_protocol shrinks, that test fails.