Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: check for s2n_array_len in loop bounds #4802

Merged
merged 2 commits into from
Oct 1, 2024
Merged

Conversation

lrstewart
Copy link
Contributor

@lrstewart lrstewart commented Sep 25, 2024

Description of changes:

Whenever I'm writing a test of the form:

struct {
    /* some variable arguments */
} test_cases[] = { /* some test cases */ };
for (size_t i = 0; i < s2n_array_len(test_cases); i++) {

I'm always worried that I'm going to do "sizeof" instead of "s2n_array_len". Best case, that would cause the test to iterate over test cases that don't exist, probably failing, at a minimum probably accessing invalid memory. But worst case, the test silently doesn't execute some test cases. I've made this mistake before.

So I added a grep_simple_mistakes that only allows sizeof in loops for variables with names indicating that's probably safe. I'd love to try to expand it to all uses of sizeof, but that's a MUCH larger set of mistakes. Loops are also what I'm most concerned about: it's a common pattern, and it's very hard to detect the mistake. Other sizeofs are generally used in assertions.

Testing:

Tests continue to pass. Here's an example of a violation:

Warning: sizeof is only valid for arrays of chars or uint8_ts. Use s2n_array_len for other types, or append "bytes", "data", or "u8" to your variable name for clarity. Warning in /home/lrstewart/Code/s2n-tls/tests/unit/s2n_resume_test.c:
408:        for (size_t i = 0; i < sizeof(ems_state); i++) {

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

@github-actions github-actions bot added the s2n-core team label Sep 25, 2024
for (int i = 0; i < sizeof(server_conn->kex_params.mutually_supported_kem_groups); i++) {
for (int i = 0; i < s2n_array_len(server_conn->kex_params.mutually_supported_kem_groups); i++) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Luckily this and line 720 are the only existing mistakes I found. From my testing, they currently pass without triggering an invalid memory access because they never iterate all the way through the loop. Currently, both always bail on the first kem group.

@lrstewart lrstewart changed the title tests: check for proper sizeof loop bounds ci: check for s2n_array_len in loop bounds Sep 25, 2024
@lrstewart lrstewart marked this pull request as ready for review September 25, 2024 21:53
@lrstewart lrstewart enabled auto-merge (squash) October 1, 2024 00:54
@lrstewart lrstewart merged commit 373bc2d into aws:main Oct 1, 2024
37 checks passed
@lrstewart lrstewart deleted the arraylen_2 branch October 1, 2024 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants