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

test: fix session-ticket, non-blocking-io tests on 32 bit #3969

Merged
merged 2 commits into from
May 6, 2023

Conversation

jmayclin
Copy link
Contributor

This test fixes two session tests that were failing on 32 bit platforms. Session_ticket was failing because a time_t overflowed when converting from seconds to nanoseconds.

Non-blocking was failing because of different integer comparison logic on 32 bit vs 64 bit time_t.

Resolved issues:

Makes progress on #3948

Description of changes:

s2n_self_talk_nonblocking_test

This test was failing because of a broken macro. The macro had always been broken, but became differently broken on a 32 bit platform.

#define LESS_THAN_ELAPSED_SECONDS(start_time, max_time) ((start_time - time(0)) < max_time)

This macro is a little bit backwards, because it is small_number - big_number, which results in the left side of the comparison always being negative.

The left side of the comparison is time_t types and the right side (max_time) is uint32_t.

On a 64 bit platform, time_t is an int64_t which can represent the entire range of uint32_t. During comparison uint32_t is safely upsized so that time_t:-1 < uint32_t:300 evaluates to true.

On a 32 bit platform, time_t is normally an int32_t. Comparison between int32_t and uint_32t results in the int32_t getting "upgraded" to an unsigned integer type, so the negative number becomes a very big number, resulting in the condition evaluating to false. This leads the client to an early exit, which results in the test failing.

s2n_session_ticket_test

There was a current time unix_seconds time_t that we tried to convert to nanoseconds before upsizing to 64 bits. The fix was to cast to an int64_t before converting to nanoseconds.

Testing:

Tested locally with the following

cmake . \
	-Bbuild \
	-DCMAKE_BUILD_TYPE=DEBUG \
	-DCMAKE_TOOLCHAIN_FILE=cmake/toolchains/32-bit.toolchain

cmake --build ./build -j $(nproc)

CTEST_PARALLEL_LEVEL=$(nproc) make -C build test

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

This test fixes two session tests that were failing on 32 bit platforms.
Session_ticket was failing because a time_t overflowed when converting
from seconds to nanoseconds.

Non-blocking was failing because of different integer comparison logic
on 32 bit vs 64 bit time_t.
@jmayclin jmayclin requested review from goatgoose and lrstewart April 28, 2023 21:51
@github-actions github-actions bot added the s2n-core team label Apr 28, 2023
@jmayclin
Copy link
Contributor Author

jmayclin commented May 1, 2023

This PR has gotten a bit large to review as a single unit, so I'll be closing it and splitting up the changes into smaller PRs. Edit: wrong PR, this is already the refactored PR. 😄

@jmayclin jmayclin closed this May 1, 2023
@jmayclin jmayclin reopened this May 1, 2023
@jmayclin
Copy link
Contributor Author

jmayclin commented May 5, 2023

Waiting for #3977 to rebase and merge this, since it will validate the 32 bit fixes in CI.

@jmayclin jmayclin enabled auto-merge (squash) May 6, 2023 00:39
@jmayclin jmayclin disabled auto-merge May 6, 2023 00:39
@jmayclin jmayclin enabled auto-merge (squash) May 6, 2023 00:39
@jmayclin jmayclin changed the title test: fix session_ticket, non-blocking test on 32 bit test: fix session-ticket, non-blocking-io tests on 32 bit May 6, 2023
@jmayclin jmayclin merged commit 9b7b1f3 into aws:main May 6, 2023
@jmayclin
Copy link
Contributor Author

jmayclin commented May 6, 2023

Hmm, looks like the Nix Integration tests failed despite all of the normal integration tests passing? Generally I've only seen the nix integration tests gives false positives (the tests reported a failure but nothing was wrong), so I don't feel any immediate need to revert this. But we should look into this next week and confirm.

@jmayclin
Copy link
Contributor Author

jmayclin commented May 6, 2023

Even in the case that the nix tests are actually failing, I don't think we'd revert this commit. This commit's impact is very strictly limited to unit tests files, and I have high certainty that it does not touch any of the code that gets exercised in the Integration Tests.

dougch pushed a commit to dougch/s2n-tls that referenced this pull request May 31, 2023
@jmayclin jmayclin deleted the st-nb-test-failures branch December 22, 2023 01:59
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