Fix multiple bugs in NTP packet serialization

tidal-music · Oct 20, 2023 · b791b67 · b791b67
1 parent 1cc70e6
commit b791b67
Show file tree

Hide file tree

Showing 7 changed files with 32 additions and 26 deletions.
diff --git a/library/src/commonMain/kotlin/com/tidal/networktime/internal/NtpExchanger.kt b/library/src/commonMain/kotlin/com/tidal/networktime/internal/NtpExchanger.kt
@@ -18,13 +18,13 @@ internal class NtpExchanger(
   ): NtpExchangeResult? {
     val ntpUdpSocketOperations = NtpUdpSocketOperations()
     val requestPacket = NtpPacket(
-      versionNumber = ntpVersion.toByte(),
-      mode = NTP_MODE_CLIENT.toByte(),
+      versionNumber = ntpVersion.toInt(),
+      mode = NTP_MODE_CLIENT,
     )
     return try {
       ntpUdpSocketOperations.prepareSocket(queryTimeout.inWholeMilliseconds)
       val requestTime = referenceClock.referenceEpochTime
-      val buffer = ntpPacketSerializer(requestPacket.copy(originateEpochTimestamp = requestTime))
+      val buffer = ntpPacketSerializer(requestPacket.copy(transmitEpochTimestamp = requestTime))
       ntpUdpSocketOperations.exchangePacketInPlace(
         buffer,
         address,

diff --git a/library/src/commonMain/kotlin/com/tidal/networktime/internal/NtpPacket.kt b/library/src/commonMain/kotlin/com/tidal/networktime/internal/NtpPacket.kt
@@ -4,19 +4,19 @@ import kotlin.time.Duration
 import kotlin.time.Duration.Companion.days
 
 internal data class NtpPacket(
-  val leapIndicator: Byte = 0,
-  val versionNumber: Byte = 0,
-  val mode: Byte = 0,
+  val leapIndicator: Int = 0,
+  val versionNumber: Int = 0,
+  val mode: Int = 0,
   val stratum: Byte = 0,
   val poll: Byte = 0,
   val precision: Byte = 0,
-  val rootDelay: Duration = Duration.ZERO,
-  val rootDispersion: Duration = Duration.ZERO,
+  val rootDelay: Duration = Duration.INFINITE,
+  val rootDispersion: Duration = Duration.INFINITE,
   val referenceIdentifier: Int = 0,
-  val referenceEpochTimestamp: Duration = Duration.ZERO,
-  val originateEpochTimestamp: Duration = Duration.ZERO,
-  val receiveEpochTimestamp: Duration = Duration.ZERO,
-  val transmitEpochTimestamp: Duration = Duration.ZERO,
+  val referenceEpochTimestamp: Duration = Duration.INFINITE,
+  val originateEpochTimestamp: Duration = Duration.INFINITE,
+  val receiveEpochTimestamp: Duration = Duration.INFINITE,
+  val transmitEpochTimestamp: Duration = Duration.INFINITE,
 ) {
   init {
     // Check sizes of fields whose type does not match their corresponding size in the actual packet

diff --git a/library/src/commonMain/kotlin/com/tidal/networktime/internal/NtpPacketDeserializer.kt b/library/src/commonMain/kotlin/com/tidal/networktime/internal/NtpPacketDeserializer.kt
@@ -9,9 +9,9 @@ internal class NtpPacketDeserializer {
   operator fun invoke(bytes: ByteArray): NtpPacket {
     var index = 0
     return NtpPacket(
-      ((bytes[index++].toInt() shl 8) + bytes[index++]).toByte(),
-      ((bytes[index++].toInt() shl 16) + (bytes[index++].toInt() shl 24) + bytes[index++]).toByte(),
-      ((bytes[index++].toInt() shl 16) + (bytes[index++].toInt() shl 24) + bytes[index++]).toByte(),
+      (bytes[index++].toInt() shl 8) + bytes[index++],
+      (bytes[index++].toInt() shl 16) + (bytes[index++].toInt() shl 24) + bytes[index++],
+      (bytes[index++].toInt() shl 16) + (bytes[index++].toInt() shl 24) + bytes[index++],
       bytes[index++],
       bytes[index++],
       bytes[index++],

diff --git a/library/src/commonMain/kotlin/com/tidal/networktime/internal/NtpPacketSerializer.kt b/library/src/commonMain/kotlin/com/tidal/networktime/internal/NtpPacketSerializer.kt
@@ -1,12 +1,12 @@
 package com.tidal.networktime.internal
 
+import kotlin.random.Random
 import kotlin.time.Duration
 
-internal class NtpPacketSerializer {
+internal class NtpPacketSerializer(private val random: Random) {
   operator fun invoke(ntpPacket: NtpPacket) = with(ntpPacket) {
     byteArrayOf(
-      ((leapIndicator.toInt() and 0b11) or (versionNumber.toInt() and 0b110)).toByte(),
-      ((versionNumber.toInt() and 0b1) or (mode.toInt() and 0b111)).toByte(),
+      ((leapIndicator shl 6) or (versionNumber shl 3) or mode).toByte(),
       stratum,
       poll,
       precision,
@@ -25,6 +25,9 @@ internal class NtpPacketSerializer {
 
   private val Duration.asIntervalToNtpInterval: ByteArray
     get() {
+      if (this == Duration.INFINITE) {
+        return ByteArray(4)
+      }
       val wholeSeconds = inWholeSeconds
       val fraction = (inWholeMilliseconds - wholeSeconds * 1_000) * (1 shl 16) / 1_000
       return byteArrayOf(
@@ -37,6 +40,9 @@ internal class NtpPacketSerializer {
 
   private val Duration.asEpochTimestampToNtpEpochTimestamp: ByteArray
     get() {
+      if (this == Duration.INFINITE) {
+        return ByteArray(8)
+      }
       val seconds = (this + NtpPacket.NTP_EPOCH_OFFSET_WITH_EPOCH).inWholeSeconds
       val fraction = (inWholeMilliseconds - inWholeSeconds * 1_000) *
         0b100000000000000000000000000000000 /
@@ -49,7 +55,7 @@ internal class NtpPacketSerializer {
         (fraction shr 24).toByte(),
         (fraction shr 16).toByte(),
         (fraction shr 8).toByte(),
-        fraction.toByte(),
+        random.nextBytes(1).single(),
       )
     }
 }
diff --git a/library/src/commonMain/kotlin/com/tidal/networktime/internal/SyncPeriodic.kt b/library/src/commonMain/kotlin/com/tidal/networktime/internal/SyncPeriodic.kt
@@ -16,11 +16,12 @@ internal class SyncPeriodic(
     HttpClientFactory()() { install(HttpTimeout) },
     DnsOverHttpsResponseParser(),
   ),
+  random: Random = Random.Default,
   private val ntpExchanger: NtpExchanger = NtpExchanger(
     referenceClock,
-    NtpPacketSerializer(),
+    NtpPacketSerializer(random),
     NtpPacketDeserializer(),
-    Random.Default,
+    random,
   ),
 ) {
   suspend operator fun invoke() {

diff --git a/library/src/commonMain/kotlin/com/tidal/networktime/internal/SyncSingular.kt b/library/src/commonMain/kotlin/com/tidal/networktime/internal/SyncSingular.kt
@@ -26,11 +26,11 @@ internal class SyncSingular(
       .sortedBy { it.clockOffset }
       .run {
         if (isEmpty()) {
-          null
+          return
         } else {
           this[size / 2]
         }
-      } ?: return
+      }
     mutableState.synchronizationResult = SynchronizationResult(
       selectedResult.run { timeMeasured + clockOffset },
       referenceClock.referenceEpochTime,
@@ -47,7 +47,7 @@ internal class SyncSingular(
       },
       lookupTimeout,
     ).map { address ->
-      (1..queriesPerResolvedAddress).map {
+      (1..queriesPerResolvedAddress).mapNotNull {
         val ret = ntpExchanger(
           address,
           queryTimeout,
@@ -62,7 +62,7 @@ internal class SyncSingular(
         )
         delay(waitBetweenResolvedAddressQueries)
         ret
-      }.filterNotNull()
+      }
         .takeIf { it.isNotEmpty() }
         ?.minBy { it.roundTripDelay }
     }

diff --git a/samples/shared/src/commonMain/kotlin/root/MainViewModel.kt b/samples/shared/src/commonMain/kotlin/root/MainViewModel.kt
@@ -1,6 +1,5 @@
 package root
 
-import com.tidal.networktime.DnsLookupStrategy
 import com.tidal.networktime.NTPServer
 import com.tidal.networktime.SNTPClient
 import kotlinx.coroutines.GlobalScope