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

Backport 2.1: More robust timing tests #1223

Merged
merged 12 commits into from
Dec 26, 2017

Conversation

gilles-peskine-arm
Copy link
Contributor

This is a partial backport of #1136 . Included:

  • Fix read from an uninitialized integer object in mbedtls_timing_get_timer on Windows (internal ref: IOTSSL-1822)
  • Improved documentation of the timing API.
  • Fix mbedtls_set_alarm(0) on Unix/POSIX.
  • In the selftest program, support running a subset of the tests with command line arguments.
  • The timing self-test is more tolerant and shorter.

Not included:

  • New unit tests — there were no unit tests for the timing module in 2.1.

Internal ref: IOTSSL-1798

The POSIX/Unix implementation of mbedtls_set_alarm did not set the
mbedtls_timing_alarmed flag when called with 0, which was inconsistent
with what the documentation implied and with the Windows behavior.
mbedtls_timing_get_timer with reset=1 is called both to initialize a
timer object and to reset an already-initialized object. In an
initial call, the content of the data structure is indeterminate, so
the code should not read from it. This could crash if signed overflows
trap, for example.

As a consequence, on reset, we can't return the previously elapsed
time as was previously done on Windows. Return 0 as was done on Unix.
Print some not-very-nice-looking but helpful diagnosis information if
the timing selftest fails. Since the failures tend to be due to heavy
system load that's hard to reproduce, this information is necessary to
understand what's going on.
If given command line arguments, interpret them as test names and only
run those tests.
E.g. "selftest -x timing" runs all the self-tests except timing.
mbedtls_timing_self_test fails annoyingly often when running on a busy
machine such as can be expected of a continous integration system.
Increase the tolerances in the delay test, to reduce the chance of
failures that are only due to missing a deadline on a busy machine.
Increase the duration of the self test, otherwise it tends to fail on
a busy machine even with the recently upped tolerance. But run the
loop only once, it's enough for a simple smoke test.
We don't need to test multiple delays in a self-test.
Save 5s of busy-wait.
@gilles-peskine-arm gilles-peskine-arm added enhancement mbed TLS team needs-review Every commit must be reviewed by at least two team members, labels Dec 20, 2017
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

I'm satistifed this is a faithful backport of the main branch:

  • minus the new test suite which wasn't planned for backporting
  • plus a fix to an error code in selftest.c along the way (the issue wasn't present in development)

Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Withdrawing my approval in the light of the following CI failure: EXIT_FAILURE not defined in selftest.c. Please fix by making it more similar to the development branch.

Include stdlib.h for EXIT_FAILURE.
@gilles-peskine-arm
Copy link
Contributor Author

@mpg In the development branch, there's MBEDTLS_EXIT_FAILURE in the platform abstraction, but that's not in 2.1. I hadn't realized that stdlib.h wasn't included (it was getting included indirectly in the configurations that I'd tested locally) so I added that.

Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

I'm happy with the fix.

Copy link

@mazimkhan mazimkhan left a comment

Choose a reason for hiding this comment

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

LGTM

@mpg mpg added needs-design-approval and removed needs-review Every commit must be reviewed by at least two team members, labels Dec 21, 2017
@mpg mpg merged commit 83cd34a into Mbed-TLS:mbedtls-2.1 Dec 26, 2017
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.

3 participants