More robust timing tests

ChangeLog

@@ -4,11 +4,17 @@ mbed TLS ChangeLog (Sorted per branch, date)
 
 Features
    * Allow comments in test data files.
+   * The selftest program can execute a subset of the tests based on command
+     line arguments.
+   * New unit tests for timing. Improve the self-test to be more robust
+     when run on a heavily-loaded machine.
 
 Bugfix
    * Fix memory leak in mbedtls_ssl_set_hostname() when called multiple times.
      Found by projectgus and jethrogb, #836.
    * Fix usage help in ssl_server2 example. Found and fixed by Bei Lin.
+   * Fix mbedtls_timing_alarm(0) on Unix.
+   * Fix use of uninitialized memory in mbedtls_timing_get_timer when reset=1.
 
 = mbed TLS 2.6.0 branch released 2017-08-10
 

include/mbedtls/timing.h

@@ -1,7 +1,7 @@
 /**
  * \file timing.h
  *
- * \brief Portable interface to the CPU cycle counter
+ * \brief Portable interface to timeouts and to the CPU cycle counter
  *
  *  Copyright (C) 2006-2015, ARM Limited, All Rights Reserved
  *  SPDX-License-Identifier: Apache-2.0
@@ -65,21 +65,36 @@ extern volatile int mbedtls_timing_alarmed;
  * \warning        This is only a best effort! Do not rely on this!
  *                 In particular, it is known to be unreliable on virtual
  *                 machines.
+ *
+ * \note           This value starts at an unspecified origin and
+ *                 may wrap around.
  */
 unsigned long mbedtls_timing_hardclock( void );
 
 /**
  * \brief          Return the elapsed time in milliseconds
  *
  * \param val      points to a timer structure
- * \param reset    if set to 1, the timer is restarted
+ * \param reset    If 0, query the elapsed time. Otherwise (re)start the timer.
+ *
+ * \return         Elapsed time since the previous reset in ms. When
+ *                 restarting, this is always 0.
+ *
+ * \note           To initialize a timer, call this function with reset=1.
+ *
+ *                 Determining the elapsed time and resetting the timer is not
+ *                 atomic on all platforms, so after the sequence
+ *                 `{ get_timer(1); ...; time1 = get_timer(1); ...; time2 =
+ *                 get_timer(0) }` the value time1+time2 is only approximately
+ *                 the delay since the first reset.
  */
 unsigned long mbedtls_timing_get_timer( struct mbedtls_timing_hr_time *val, int reset );
 
 /**
  * \brief          Setup an alarm clock
  *
  * \param seconds  delay before the "mbedtls_timing_alarmed" flag is set
+ *                 (must be >=0)
  *
  * \warning        Only one alarm at a time  is supported. In a threaded
  *                 context, this means one for the whole process, not one per
@@ -91,11 +106,15 @@ void mbedtls_set_alarm( int seconds );
  * \brief          Set a pair of delays to watch
  *                 (See \c mbedtls_timing_get_delay().)
  *
- * \param data     Pointer to timing data
+ * \param data     Pointer to timing data.
  *                 Must point to a valid \c mbedtls_timing_delay_context struct.
  * \param int_ms   First (intermediate) delay in milliseconds.
+ *                 The effect if int_ms > fin_ms is unspecified.
  * \param fin_ms   Second (final) delay in milliseconds.
  *                 Pass 0 to cancel the current delay.
+ *
+ * \note           To set a single delay, either use \c mbedtls_timing_set_timer
+ *                 directly or use this function with int_ms == fin_ms.
  */
 void mbedtls_timing_set_delay( void *data, uint32_t int_ms, uint32_t fin_ms );
 
@@ -106,7 +125,7 @@ void mbedtls_timing_set_delay( void *data, uint32_t int_ms, uint32_t fin_ms );
  * \param data     Pointer to timing data
  *                 Must point to a valid \c mbedtls_timing_delay_context struct.
  *
- * \return         -1 if cancelled (fin_ms = 0)
+ * \return         -1 if cancelled (fin_ms = 0),
  *                  0 if none of the delays are passed,
  *                  1 if only the intermediate delay is passed,
  *                  2 if the final delay is passed.

library/timing.c

@@ -244,21 +244,23 @@ volatile int mbedtls_timing_alarmed = 0;
 
 unsigned long mbedtls_timing_get_timer( struct mbedtls_timing_hr_time *val, int reset )
 {
-    unsigned long delta;
-    LARGE_INTEGER offset, hfreq;
     struct _hr_time *t = (struct _hr_time *) val;
 
-    QueryPerformanceCounter(  &offset );
-    QueryPerformanceFrequency( &hfreq );
-
-    delta = (unsigned long)( ( 1000 *
-        ( offset.QuadPart - t->start.QuadPart ) ) /
-           hfreq.QuadPart );
-
     if( reset )
+    {
         QueryPerformanceCounter( &t->start );
-
-    return( delta );
+        return( 0 );
+    }
+    else
+    {
+        unsigned long delta;
+        LARGE_INTEGER now, hfreq;
+        QueryPerformanceCounter(  &now );
+        QueryPerformanceFrequency( &hfreq );
+        delta = (unsigned long)( ( now.QuadPart - t->start.QuadPart ) * 1000ul
+                                 / hfreq.QuadPart );
+        return( delta );
+    }
 }
 
 /* It's OK to use a global because alarm() is supposed to be global anyway */
@@ -285,23 +287,22 @@ void mbedtls_set_alarm( int seconds )
 
 unsigned long mbedtls_timing_get_timer( struct mbedtls_timing_hr_time *val, int reset )
 {
-    unsigned long delta;
-    struct timeval offset;
     struct _hr_time *t = (struct _hr_time *) val;
 
-    gettimeofday( &offset, NULL );
-
     if( reset )
     {
-        t->start.tv_sec  = offset.tv_sec;
-        t->start.tv_usec = offset.tv_usec;
+        gettimeofday( &t->start, NULL );
         return( 0 );
     }
-
-    delta = ( offset.tv_sec  - t->start.tv_sec  ) * 1000
-          + ( offset.tv_usec - t->start.tv_usec ) / 1000;
-
-    return( delta );
+    else
+    {
+        unsigned long delta;
+        struct timeval now;
+        gettimeofday( &now, NULL );
+        delta = ( now.tv_sec  - t->start.tv_sec  ) * 1000ul
+              + ( now.tv_usec - t->start.tv_usec ) / 1000;
+        return( delta );
+    }
 }
 
 static void sighandler( int signum )
@@ -315,6 +316,12 @@ void mbedtls_set_alarm( int seconds )
     mbedtls_timing_alarmed = 0;
     signal( SIGALRM, sighandler );
     alarm( seconds );
+    if( seconds == 0 )
+    {
+        /* alarm(0) cancelled any previous pending alarm, but the
+           handler won't fire, so raise the flag straight away. */
+        mbedtls_timing_alarmed = 1;
+    }
 }
 
 #endif /* _WIN32 && !EFIX64 && !EFI32 */
@@ -378,13 +385,21 @@ static void busy_msleep( unsigned long msec )
     (void) j;
 }
 
-#define FAIL    do                      \
-{                                       \
-    if( verbose != 0 )                  \
-        mbedtls_printf( "failed\n" );   \
-                                        \
-    return( 1 );                        \
-} while( 0 )
+#define FAIL    do                                                      \
+    {                                                                   \
+        if( verbose != 0 )                                              \
+        {                                                               \
+            mbedtls_printf( "failed at line %d\n", __LINE__ );          \
+            mbedtls_printf( " cycles=%lu ratio=%lu millisecs=%lu secs=%lu hardfail=%d a=%lu b=%lu\n", \
+                            cycles, ratio, millisecs, secs, hardfail,   \
+                            (unsigned long) a, (unsigned long) b );     \
+            mbedtls_printf( " elapsed(hires)=%lu elapsed(ctx)=%lu status(ctx)=%d\n", \
+                            mbedtls_timing_get_timer( &hires, 0 ),      \
+                            mbedtls_timing_get_timer( &ctx.timer, 0 ),  \
+                            mbedtls_timing_get_delay( &ctx ) );         \
+        }                                                               \
+        return( 1 );                                                    \
+    } while( 0 )
 
 /*
  * Checkup routine
@@ -394,22 +409,22 @@ static void busy_msleep( unsigned long msec )
  */
 int mbedtls_timing_self_test( int verbose )
 {
-    unsigned long cycles, ratio;
-    unsigned long millisecs, secs;
-    int hardfail;
+    unsigned long cycles = 0, ratio = 0;
+    unsigned long millisecs = 0, secs = 0;
+    int hardfail = 0;
     struct mbedtls_timing_hr_time hires;
-    uint32_t a, b;
+    uint32_t a = 0, b = 0;
     mbedtls_timing_delay_context ctx;
 
     if( verbose != 0 )
         mbedtls_printf( "  TIMING tests note: will take some time!\n" );
 
-
     if( verbose != 0 )
         mbedtls_printf( "  TIMING test #1 (set_alarm / get_timer): " );
 
-    for( secs = 1; secs <= 3; secs++ )
     {
+        secs = 1;
+
         (void) mbedtls_timing_get_timer( &hires, 1 );
 
         mbedtls_set_alarm( (int) secs );
@@ -421,12 +436,7 @@ int mbedtls_timing_self_test( int verbose )
         /* For some reason on Windows it looks like alarm has an extra delay
          * (maybe related to creating a new thread). Allow some room here. */
         if( millisecs < 800 * secs || millisecs > 1200 * secs + 300 )
-        {
-            if( verbose != 0 )
-                mbedtls_printf( "failed\n" );
-
-            return( 1 );
-        }
+            FAIL;
     }
 
     if( verbose != 0 )
@@ -435,28 +445,22 @@ int mbedtls_timing_self_test( int verbose )
     if( verbose != 0 )
         mbedtls_printf( "  TIMING test #2 (set/get_delay        ): " );
 
-    for( a = 200; a <= 400; a += 200 )
     {
-        for( b = 200; b <= 400; b += 200 )
-        {
-            mbedtls_timing_set_delay( &ctx, a, a + b );
+        a = 800;
+        b = 400;
+        mbedtls_timing_set_delay( &ctx, a, a + b );          /* T = 0 */
 
-            busy_msleep( a - a / 8 );
-            if( mbedtls_timing_get_delay( &ctx ) != 0 )
-                FAIL;
+        busy_msleep( a - a / 4 );                      /* T = a - a/4 */
+        if( mbedtls_timing_get_delay( &ctx ) != 0 )
+            FAIL;
 
-            busy_msleep( a / 4 );
-            if( mbedtls_timing_get_delay( &ctx ) != 1 )
-                FAIL;
+        busy_msleep( a / 4 + b / 4 );                  /* T = a + b/4 */
+        if( mbedtls_timing_get_delay( &ctx ) != 1 )
+            FAIL;
 
-            busy_msleep( b - a / 8 - b / 8 );
-            if( mbedtls_timing_get_delay( &ctx ) != 1 )
-                FAIL;
-
-            busy_msleep( b / 4 );
-            if( mbedtls_timing_get_delay( &ctx ) != 2 )
-                FAIL;
-        }
+        busy_msleep( b );                          /* T = a + b + b/4 */
+        if( mbedtls_timing_get_delay( &ctx ) != 2 )
+            FAIL;
     }
 
     mbedtls_timing_set_delay( &ctx, 0, 0 );
@@ -475,7 +479,6 @@ int mbedtls_timing_self_test( int verbose )
      * On a 4Ghz 32-bit machine the cycle counter wraps about once per second;
      * since the whole test is about 10ms, it shouldn't happen twice in a row.
      */
-    hardfail = 0;
 
 hard_test:
     if( hardfail > 1 )