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 1.3: More robust timing tests #1224

Merged
merged 4 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)
  • Fix mbedtls_set_alarm(0) on Unix/POSIX.
  • The timing self-test is more tolerant and shorter.

I omitted the rest because 1.3 is on its way out and I don't think the rest of the original PR is worth it.

Internal ref: IOTSSL-1798

The POSIX/Unix implementation of set_alarm did not set the
alarmed flag when called with 0, which was inconsistent
with what the documentation implied and with the Windows behavior.
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.
We don't need to test multiple delays in a self-test.
Save 10s 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 satisfied this is a faithful commit of the changes in the main branch that are relevant to 1.3.

@mpg
Copy link
Contributor

mpg commented Dec 21, 2017

CI failed due to a build failure on windows/mingw which is an issue in the CI, unrelated to this PR - not an issue for merging.

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
Copy link
Contributor

mpg commented Dec 21, 2017 via email

@mpg mpg merged commit 8833e86 into Mbed-TLS:mbedtls-1.3 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