Refactor calculateClockOffset to fix overflow issues

lexicon.txt

@@ -16,6 +16,7 @@ bytestorecv
 bytestorecv
 bytestosend
 bytestosend
+clienttime
 clienttxtime
 clockfreqtolerance
 clockoffsetsec
@@ -41,6 +42,7 @@ expectedinterval
 expectedtxtime
 faqs
 feb
+firstorderdiff
 fracs
 fracsinnetorder
 fracsinnetorder
@@ -149,7 +151,10 @@ serializerequest
 serveraddr
 servernamelen
 serverport
+servertime
 setsystemtimefunc
+signedfirstorderdiffrecv
+signedfirstorderdiffsend
 sizeof
 sntp
 sntpbuffertoosmall
@@ -176,6 +181,7 @@ startingpos
 struct
 sublicense
 testbuffer
+testclockoffsetcalculation
 timebeforeloop
 timeserver
 transmittime

source/core_sntp_serializer.c

@@ -121,6 +121,18 @@
  */
 #define CLOCK_OFFSET_FIRST_ORDER_DIFF_OVERFLOW_BITS_MASK    ( 0xC0000000U )
 
+/**
+ * @brief The bit mask for the most-significant bit in an unsigned 32 bit integer.
+ * This value is used in the intermediate conversion of unsigned integer to signed
+ * integer for the first order difference values during clock-offset calculation.
+ *
+ * @note The conversion from unsigned to signed integer of first order difference
+ * values uses this macro to check whether assignment of the unsigned value to signed
+ * integer will overflow, and accordingly, change the unsigned value to safely perform
+ * the conversion.
+ */
+#define UINT32_MOST_SIGNIFICANT_BIT_BIT_MASK                ( 0x80000000U )
+
 /**
  * @brief Structure representing an SNTP packet header.
  * For more information on SNTP packet format, refer to
@@ -191,6 +203,31 @@ static uint32_t readWordFromNetworkByteOrderMemory( const uint32_t * ptr )
                           ( ( uint32_t ) *( pMemStartByte + 3 ) ) );
 }
 
+/**
+ * @brief Utility to determine whether the passed first order difference value
+ * between server and client timestamps represents that the server and client
+ * are within 34 years of each other to be able to calculate the clock-offset
+ * value according to the NTPv4 specification's on-wire protocol.
+ *
+ * @param[in] firstOrderDiff A first-order difference value between the client
+ * and server.
+ *
+ * @note As the SNTP timestamp value wraps around after ~136 years (exactly at
+ * 7 Feb 2036 6h 28m 16s), the utility logic checks the first order difference
+ * in both polarities (i.e. as (Server - Client) and (Client - Server) time values )
+ * to support the edge case when the two timestamps are in different SNTP eras (for
+ * example, server time is in 2037 and client time is in 2035 ).
+ */
+static bool isEligibleForClockOffsetCalculation( uint32_t firstOrderDiff )
+{
+    /* Note: The (1U + ~firstOrderDiff) expression represents 2's complement or negation of value.
+     * This is done to be compliant with both CBMC and MISRA Rule 10.1.
+     * CBMC flags overflow for (unsigned int = 0U - positive value) whereas
+     * MISRA rule forbids use of unary minus operator on unsigned integers. */
+    return( ( ( firstOrderDiff & CLOCK_OFFSET_FIRST_ORDER_DIFF_OVERFLOW_BITS_MASK ) == 0U ) ||
+            ( ( ( 1U + ~firstOrderDiff ) & CLOCK_OFFSET_FIRST_ORDER_DIFF_OVERFLOW_BITS_MASK ) == 0U ) );
+}
+
 /**
  * @brief Utility to calculate clock offset of system relative to the
  * server using the on-wire protocol specified in the NTPv4 specification.
@@ -259,50 +296,104 @@ static SntpStatus_t calculateClockOffset( const SntpTimestamp_t * pClientTxTime,
     SntpStatus_t status = SntpSuccess;
 
     /* Variable for storing the first-order difference between timestamps. */
-    uint32_t firstOrderDiff = 0;
+    uint32_t firstOrderDiffSend = 0;
+    uint32_t firstOrderDiffRecv = 0;
+    bool sendPolarity, recvPolarity;
 
     assert( pClientTxTime != NULL );
     assert( pServerRxTime != NULL );
     assert( pServerTxTime != NULL );
     assert( pClientRxTime != NULL );
     assert( pClockOffset != NULL );
 
-    /* Calculate a sample first order difference value between the
-     * server and system timestamps. */
-    firstOrderDiff = pClientRxTime->seconds - pServerTxTime->seconds;
+    /* On-wire protocol formula's polarity for first order difference in the send network
+     * path is T2 (Server Rx) - T1 (Client TX) .*/
+    recvPolarity = ( pServerTxTime->seconds >= pClientRxTime->seconds ) ? true : false;
+
+    /* On-wire protocol formula's polarity for first order difference in the receive network
+     * path is T3 (Server TX) - T4 (Client RX) .*/
+    sendPolarity = ( pServerRxTime->seconds >= pClientTxTime->seconds ) ? true : false;
+
+    /* Calculate first order difference values between the server and system timestamps
+     * to determine whether they are within 34 years of each other. */
+
+    /* To avoid overflow issue, we will store only the positive first order difference
+     * values in the unsigned integer now, and later store the values with correct polarity
+     * (according to the formula) later. */
+    firstOrderDiffSend = sendPolarity ? ( pServerRxTime->seconds - pClientTxTime->seconds ) :
+                         ( pClientTxTime->seconds - pServerRxTime->seconds );
+    firstOrderDiffRecv = recvPolarity ? ( pServerTxTime->seconds - pClientRxTime->seconds ) :
+                         ( pClientRxTime->seconds - pServerTxTime->seconds );
 
-    /* Determine from the first order difference if the system time is within
+    /* Determine from the first order differences if the system time is within
      * 34 years of server time to be able to calculate clock offset.
-     *
-     * Note: As the SNTP timestamp value wraps around after ~136 years (exactly at
-     * 7 Feb 2036 6h 28m 16s), the conditional logic checks first order difference
-     * in both polarities (i.e. as (Server - Client) and (Client - Server) time values )
-     * to support the edge case when the two timestamps are in different SNTP eras (for
-     * example, server time is in 2037 and client time is in 2035 ).
      */
-    if( ( ( firstOrderDiff & CLOCK_OFFSET_FIRST_ORDER_DIFF_OVERFLOW_BITS_MASK )
-          == 0U ) ||
-        ( ( ( 0U - firstOrderDiff ) & CLOCK_OFFSET_FIRST_ORDER_DIFF_OVERFLOW_BITS_MASK )
-          == 0U ) )
+    if( isEligibleForClockOffsetCalculation( firstOrderDiffSend ) &&
+        isEligibleForClockOffsetCalculation( firstOrderDiffRecv ) )
     {
-        /* Calculate the clock-offset as system time is within 34 years window
-         * of server time. */
-        uint32_t firstOrderDiffSend;
-        uint32_t firstOrderDiffRecv;
-        uint32_t sumOfFirstOrderDiffs;
+        /* Now that we have validated that system and server times are within 34 years
+         * of each other, we will prepare for the clock-offset calculation. To calculate
+         * clock-offset, the first order difference values need to be stored as signed integers
+         * in the correct polarity of differences according to the formula. */
+        int32_t signedFirstOrderDiffSend = 0;
+        int32_t signedFirstOrderDiffRecv = 0;
+
+        /* To avoid overflow issues in assigning the unsigned values of first order differences
+         * to signed integers, we will inverse the values if they represent a large value that cannot be
+         * represented in a signed integer. */
+        if( ( firstOrderDiffSend & UINT32_MOST_SIGNIFICANT_BIT_BIT_MASK )
+            == UINT32_MOST_SIGNIFICANT_BIT_BIT_MASK )
 
-        /* Perform ( T2 - T1 ) offset calculation of SNTP Request packet path. */
-        firstOrderDiffSend = pServerRxTime->seconds - pClientTxTime->seconds;
+        {
+            /* Negate the unsigned integer by performing 2's complement operation.
+             * Note: This is done to be compliant with both CBMC and MISRA Rule 10.1.
+             * CBMC flags overflow for (unsigned int = 0U - positive value) whereas
+             * MISRA rule forbids use of unary minus operator on unsigned integers. */
+            firstOrderDiffSend = 1U + ~firstOrderDiffSend;
+            sendPolarity = !sendPolarity;
+        }
+
+        if( ( firstOrderDiffRecv & UINT32_MOST_SIGNIFICANT_BIT_BIT_MASK )
+            == UINT32_MOST_SIGNIFICANT_BIT_BIT_MASK )
+
+        {
+            /* Negate the unsigned integer by performing 2's complement operation.
+             * Note: This is done to be compliant with both CBMC and MISRA Rule 10.1.
+             * CBMC flags overflow for (unsigned int = 0U - positive value) whereas
+             * MISRA rule forbids use of unary minus operator on unsigned integers. */
+            firstOrderDiffRecv = 1U + ~firstOrderDiffRecv;
+            recvPolarity = !recvPolarity;
+        }
+
+        /* Now we can safely store the first order differences as signed integer values. */
+        signedFirstOrderDiffSend = ( int32_t ) firstOrderDiffSend;
+        signedFirstOrderDiffRecv = ( int32_t ) firstOrderDiffRecv;
 
-        /* Perform ( T3 - T4 ) offset calculation of SNTP Response packet path. */
-        firstOrderDiffRecv = 0U - firstOrderDiff;
+        /* To calculate the clock-offset value, we will correct the signed first order difference
+        * values to match the subtraction polarity direction of "Server Time" - "Client Time". */
+        if( sendPolarity == false )
+        {
+            signedFirstOrderDiffSend = 0 - signedFirstOrderDiffSend;
+        }
 
-        /* Perform second order calculation of using average of the above offsets. */
-        sumOfFirstOrderDiffs = firstOrderDiffSend + firstOrderDiffRecv;
+        if( recvPolarity == false )
+        {
+            signedFirstOrderDiffRecv = 0 - signedFirstOrderDiffRecv;
+        }
+
+        /* We are now sure that each of the first order differences represents the values in
+         * the correct direction of polarities, i.e.
+         * signedFirstOrderDiffSend represents (T2 - T1)
+         *                AND
+         *  signedFirstOrderDiffRecv represents (T3 - T4)
+         *
+         * We are now safe to complete the calculation of the clock-offset as the average
+         * of the signed first order difference values.
+         */
 
         /* Use division instead of a bit shift to guarantee sign extension
          * regardless of compiler implementation. */
-        *pClockOffset = ( ( int32_t ) sumOfFirstOrderDiffs / 2 );
+        *pClockOffset = ( ( signedFirstOrderDiffSend + signedFirstOrderDiffRecv ) / 2 );
     }
     else
     {

test/unit-test/core_sntp_serializer_utest.c

@@ -441,7 +441,7 @@ void test_DeserializeResponse_KoD_packets( void )
  * SNTP server response, and determine that the clock offset cannot be calculated
  * when the client clock is beyond 34 years from server.
  */
-void test_DeserializeResponse_AcceptedResponse_Overflow_Case( void )
+void test_DeserializeResponse_AcceptedResponse_Overflow_Cases( void )
 {
     SntpTimestamp_t clientTime = TEST_TIMESTAMP;
 
@@ -465,6 +465,22 @@ void test_DeserializeResponse_AcceptedResponse_Overflow_Case( void )
                                 &serverTime, &clientTime,
                                 SntpClockOffsetOverflow,
                                 SNTP_CLOCK_OFFSET_OVERFLOW );
+
+    /* Now test the contrived case when only the send network path
+     * represents timestamps that overflow but the receive network path
+     * has timestamps that do not overflow. */
+    testClockOffsetCalculation( &clientTime, &serverTime, /* Send Path times are 40 years apart */
+                                &serverTime, &serverTime, /* Receive path times are the same. */
+                                SntpClockOffsetOverflow,
+                                SNTP_CLOCK_OFFSET_OVERFLOW );
+
+    /* Now test the contrived case when only the receive network path
+     * represents timestamps that overflow but the send network path
+     * has timestamps that do not overflow. */
+    testClockOffsetCalculation( &clientTime, &clientTime, /* Send Path times are the same, i.e. don't overflow */
+                                &serverTime, &clientTime, /* Receive path times are 40 years apart. */
+                                SntpClockOffsetOverflow,
+                                SNTP_CLOCK_OFFSET_OVERFLOW );
 }
 
 /**