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

Fix initialization errors in unit tests #4370

Merged
merged 3 commits into from
Jan 19, 2024
Merged

Conversation

goatgoose
Copy link
Contributor

@goatgoose goatgoose commented Jan 18, 2024

Resolved issues:

Resolves #4204.

Description of changes:

In some environments, some unit tests currently error with s2n not initialized when linked to the shared library of s2n-tls. For example, see the the result of this GH action that builds s2n-tls with a shared library on macOS before this fix was applied.

This error is due to including utils/s2n_mem.c in .c files:

/* Required to override memory callbacks at runtime */
#include "utils/s2n_mem.c"

When s2n_init() is called, initialized in s2m_mem.c is set to true in s2n_mem_init():

s2n-tls/utils/s2n_mem.c

Lines 249 to 253 in 7f84701

int s2n_mem_init(void)
{
POSIX_ENSURE(s2n_mem_init_cb() >= S2N_SUCCESS, S2N_ERR_CANCELLED);
initialized = true;

When s2n_mem.c is included from a .c file, this seems to cause initialized to be set to false when checked by functions such as s2n_free_without_wipe(), which is causing unit tests to incorrectly fail:

s2n-tls/utils/s2n_mem.c

Lines 287 to 291 in 7f84701

int s2n_free_without_wipe(struct s2n_blob *b)
{
POSIX_PRECONDITION(s2n_blob_validate(b));
POSIX_ENSURE(initialized, S2N_ERR_NOT_INITIALIZED);

This PR removes the s2n_mem.c include from .c files, and instead uses new get/override functions to get and set the mem callbacks.

Testing:

I modified the macOS GH action to additionally build and test the s2n-tls shared library, which fails without this fix.

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 18, 2024
@goatgoose goatgoose marked this pull request as ready for review January 18, 2024 22:51
@goatgoose goatgoose requested a review from lrstewart January 18, 2024 22:51
@lrstewart lrstewart requested a review from maddeleine January 18, 2024 23:49
@goatgoose goatgoose enabled auto-merge (squash) January 19, 2024 16:05
@goatgoose goatgoose merged commit 670cb43 into aws:main Jan 19, 2024
30 checks passed
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.

s2n: upgrade 1.3.50 -> 1.3.51 some unittests fail
3 participants