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

style: fix declarations without initial value #4404

Merged
merged 3 commits into from
Mar 15, 2024

Conversation

jmayclin
Copy link
Contributor

@jmayclin jmayclin commented Feb 5, 2024

Description of changes:

s2n-tls expects that all variable declarations set an initial value. This commit fixes violations of that rules across our codebase. This commit is an automated fix generated using clang-tidy.

The following script was used to generate the fixes

# remove the existing build directory
rm build -rf
# generate a compile command data base so clang-tidy knows the 
# structure of the project and it's included files
cmake . -Bbuild \
      -DCMAKE_C_COMPILER=/usr/bin/clang \
      -DCMAKE_BUILD_TYPE=RelWithDebInfo \
      -DCMAKE_EXPORT_COMPILE_COMMANDS=ON
# run clang tidy, and have it fix the violations it can
run-clang-tidy . -p build/ -checks=cppcoreguidelines-init-variables -fix

More documentation on the init-variables check can be found here.

Call-outs:

I plan to add this as a check in the CI, but didn't want to add that into this PR so that this PR would include the minimum manual changes for it to pass CI.

Testing:

This is an automated fix with no expected behavior change. I have a murky recollection that most modern compilers will zero-init by default, but regardless all tests passing in CI should confirm that this is not a behavior change.

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 Feb 5, 2024
@jmayclin jmayclin marked this pull request as ready for review February 5, 2024 20:34
@jmayclin
Copy link
Contributor Author

jmayclin commented Feb 5, 2024

CppCheck is failing because of the following errors.

[tests/unit/s2n_self_talk_broken_pipe_test.c:95]: (style:unreadVariable) Variable 'pid' is assigned a value that is never used.
[tests/unit/s2n_self_talk_min_protocol_version_test.c:67]: (style:unreadVariable) Variable 'pid' is assigned a value that is never used.
[tests/unit/s2n_self_talk_tls12_test.c:105]: (style:unreadVariable) Variable 'pid' is assigned a value that is never used.
[tests/unit/s2n_tls12_handshake_test.c:495]: (style:unreadVariable) Variable 'handshake_type_original' is assigned a value that is never used.
[tests/unit/s2n_tls13_handshake_state_machine_test.c:659]: (style:unreadVariable) Variable 'handshake_type' is assigned a value that is never used.

These errors happen because we tend to push variables up to the next scope when dealing with loops. For example, this is s2n_self_talk_min_protocol_version_test.c

    s2n_blocked_status blocked;
    int status = 0;
    pid_t pid = 0;
    char cert_chain_pem[S2N_MAX_TEST_PEM_SIZE];
    char private_key_pem[S2N_MAX_TEST_PEM_SIZE];

    for (uint8_t version = S2N_TLS12; version <= S2N_TLS13; version++) {

        pid = fork();
        if (pid == 0) {

        }

Rather than writing a cppcheck suppression for this case, I think we should just move the pid_t declaration inside the for loop. If reviewers agree I'll go ahead and do that in a separate PR to keep this one entirely automated. On a side note, I'm guessing that this is only causing issues in this refactor because we currently suppress the variableScope warning.

// Message: (style:variableScope) The scope of the variable 'text' can be reduced.
// Reason: Don't error for being able to reduce scope of variables in tests
variableScope:tests/unit/*

Now that the manual cbc patch has been fixed, this commit is 100%
autogenerated.
@jmayclin jmayclin enabled auto-merge (squash) March 15, 2024 01:25
@jmayclin jmayclin merged commit 90a8487 into aws:main Mar 15, 2024
31 checks passed
@jmayclin jmayclin deleted the fix-initial-value branch July 1, 2024 07:25
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.

3 participants