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

Add faketime test script #7799

Open
wants to merge 8 commits into
base: development
Choose a base branch
from

Conversation

yuhaoth
Copy link
Contributor

@yuhaoth yuhaoth commented Jun 19, 2023

Description

This is for detecting expired CRL/CRT and checking if the generate commands work correctly.

For time being, tests/data_files/Makefile does not work correctly. Components for that will fail

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

  • changelog provided, or not required
  • backport done, or not required
  • tests provided, or not required

@yuhaoth yuhaoth marked this pull request as draft June 19, 2023 08:55
@yuhaoth yuhaoth added the component-test Test framework and CI scripts label Jun 19, 2023
@yuhaoth yuhaoth linked an issue Jun 19, 2023 that may be closed by this pull request
@yuhaoth yuhaoth force-pushed the pr/add-faketime-test-script branch from 86f98df to 7e7e2de Compare June 26, 2023 07:57
@yuhaoth yuhaoth marked this pull request as ready for review July 3, 2023 09:11
@yuhaoth yuhaoth added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review priority-medium Medium priority - this can be reviewed as time permits labels Jul 3, 2023
@yuhaoth yuhaoth force-pushed the pr/add-faketime-test-script branch from 7e7e2de to dbb1786 Compare July 4, 2023 08:02
@@ -380,6 +381,22 @@ check_tools()
done
}

check_faketime() {

for i in /usr/local/lib/faketime/libfaketime.so.1 \
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use the faketime program?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might cause make fail or other fail due to time stamp.
If we do not wrap all commands, make or other program might report fail. And I think this change is the easiest way to avoid those failure


# CFLAGS and LDFLAGS for Asan builds that don't use CMake
# default to -O2, use -Ox _after_ this if you want another level
ASAN_CFLAGS='-O2 -Werror -fsanitize=address,undefined -fno-sanitize-recover=all'
Copy link
Contributor

Choose a reason for hiding this comment

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

Asan or Msan combined with faketime leads do a deadlock when using Clang on Ubuntu 22.04 (and other versions). This is a known issue which has been partially resolved by wolfcw/libfaketime#389 . We can build our own faketime executable with that feature included (there's no release yet), with -DFAIL_PRE_INIT_CALLS' activated at compile time. Then faketime is compatible with Clang's sanitizers.

I tested on Ubuntu 22.04 with clang 14 and faketime 27b9c83a27cf253fcfa05bcbc635e85b36acb1cc built with

make FAKETIME_COMPILE_CFLAGS='-DFAKE_FILE_TIMESTAMPS -DFAKE_RANDOM -DINTERCEPT_SYSCALL -DFAIL_PRE_INIT_CALLS'

There doesn't seem to be a problem with GCC's sanitizers, other than needing to load lib*san.so before libfaketime.so, so we can't use the faketime binary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that I'm not sure whether we want to do that: it makes sense to run the faketime job without Asan, since we only care about functional testing there. But it would be convenient. to be able to at least occasionally run a full all.sh with faketime, so I intend to make a patch to our build/test scripts (to support faketime invocation when running tests) and to the dockerfiles (to build our own faketime instead of using the one from the Linux distribution) that makes it possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try that firstly. I will update this PR with a mbedtls-test PR when it pass my local test.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've been experimenting with that and I have dockerfile patches ready in https://github.com/Mbed-TLS/mbedtls-test/tree/dev/gilles-peskine-arm/faketime-20240116 (this branch also contains groovy patches that were just for this one test run). I also have some build script patches in https://github.com/gilles-peskine-arm/mbedtls/tree/faketime-testing-3.4.1 etc which I find too invasive. CI results: development, 3.4.1, 2.28.4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ASAN_OPTIONS=verify_asan_link_order=0 faketime -f +100d might be better solution. See https://github.com/google/sanitizers/wiki/AddressSanitizerFlags.

I just pass test_default_cmake_gcc_asan, I test it with LD_PRELOAD. And I like your patches. With ASAN_OPTIONS, no invasion needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In your patches, change https://github.com/Mbed-TLS/mbedtls-test/blob/5e1d2ee0d511705178d0db99b54107ed3887300a/vars/gen_jobs.groovy#L550 to
common.all_sh_precommand += "ASAN_OPTIONS=verify_asan_link_order=0 faketime -f +${days_ahead}d " . I think that's enough.

@gilles-peskine-arm gilles-peskine-arm added the needs-adoption stalled PR that someone should pick up and complete label Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-test Test framework and CI scripts needs-adoption stalled PR that someone should pick up and complete needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review priority-medium Medium priority - this can be reviewed as time permits
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add faketime test script
2 participants