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

build: add lint for null checks #4397

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

jmayclin
Copy link
Contributor

Description of changes:

s2n-tls has strict conventions for checking the nullness of pointers returned from fallible methods. See comment here: #4369 (comment). We rely on developers and reviewers to remember to correctly check all of these functions.

Rather than having this be a manual check, we can automate it's enforcement. This PR adds a simple python script that will automatically error when there is a place where nullness isn't checked.

Example output from the tool

File: ./tests/unit/s2n_send_multirecord_test.c:148: nullness of conn was not checked
    148 DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_CLIENT),
    149 s2n_connection_ptr_free);
    150 EXPECT_SUCCESS(s2n_connection_set_config(conn, config));
    151
    152 /* Uninitialized buffer */
You should add `EXPECT_NOT_NULL(conn);` after the call to s2n_connection_new

File: ./tests/unit/s2n_ecdsa_test.c:227: nullness of config was not checked
    227 DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(),
    228 s2n_config_ptr_free);
    229 EXPECT_SUCCESS(s2n_config_set_cipher_preferences(config, "test_all"));
    230 EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(config, chain_and_key));
    231 EXPECT_SUCCESS(s2n_config_set_verification_ca_location(config,
You should add `EXPECT_NOT_NULL(config);` after the call to s2n_config_new

File: ./tests/unit/s2n_ecdsa_test.c:236: nullness of client was not checked
    236 DEFER_CLEANUP(struct s2n_connection *client = s2n_connection_new(S2N_CLIENT),
    237 s2n_connection_ptr_free);
    238 EXPECT_SUCCESS(s2n_connection_set_config(client, config));
    239
    240 DEFER_CLEANUP(struct s2n_connection *server = s2n_connection_new(S2N_SERVER),
You should add `EXPECT_NOT_NULL(client);` after the call to s2n_connection_new

This tool also includes an option to automatically --fix some of the violations. In the following PR I will apply the autofixes and also add a CI step enforcing this lint. In the meantime you can see the results here. 431 null checks were added.

Testing:

I applied the script and then manually spot checked some of the resulting files (the results can be seen in the linked branch). I also made sure that the resulting build compiled and passed all tests.

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 Jan 31, 2024
@jmayclin jmayclin marked this pull request as ready for review February 1, 2024 17:21
@jmayclin jmayclin requested a review from dougch as a code owner February 1, 2024 17:21
Copy link
Contributor

@dougch dougch left a comment

Choose a reason for hiding this comment

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

Neat!

for root, dirs, files in os.walk("."):
for file in files:
# skip all files that aren't c
# skip s2nc.c because it has different null handling conventions
Copy link
Contributor

Choose a reason for hiding this comment

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

Future issue/pr?

codebuild/bin/enforce_null_checks.py Show resolved Hide resolved
@jmayclin
Copy link
Contributor Author

jmayclin commented Feb 1, 2024

Some concerns have been raised that this PR is not a beneficial direction to move in, so hold off on merging this PR until s2n-tls maintainers have consensus on that.

jmayclin and others added 2 commits February 1, 2024 12:32
Co-authored-by: Doug Chapman <54039637+dougch@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants