Another Reset Sequence (#1358)

* proper reset sequence handling, with an added small refactor to the marshalling process

* reset sequence at 0x8000, but only when retrieving a new sequence number; caching common flag values when marshalling packets

src/main/scala/net/psforever/actors/net/MiddlewareActor.scala

@@ -6,7 +6,6 @@ import akka.actor.typed.scaladsl.{ActorContext, Behaviors}
 import akka.io.Udp
 import java.net.InetSocketAddress
 import java.security.{SecureRandom, Security}
-
 import javax.crypto.spec.SecretKeySpec
 import org.bouncycastle.jce.provider.BouncyCastleProvider
 
@@ -26,6 +25,7 @@ import net.psforever.packet.game._
 import net.psforever.packet.crypto._
 import net.psforever.packet.game.{ChangeFireModeMessage, CharacterInfoMessage, KeepAliveMessage, PingMsg}
 import net.psforever.packet.PacketCoding.CryptoCoding
+import net.psforever.packet.reset.ResetSequence
 import net.psforever.util.{Config, DiffieHellman, Md5Mac}
 
 /**
@@ -219,17 +219,13 @@ class MiddlewareActor(
     * Increment the outbound sequence number.
     * The previous sequence number is returned.
     * The fidelity of the sequence field in packets is 16 bits, so wrap back to 0 after 65535.
-    * @return
+    * @return next sequence number
     */
   private def nextSequence: Int = {
-    if (outSequence >= 0xffff) {
-      // TODO resetting the sequence to 0 causes a client crash
-      // but that does not happen when we always send the same number
-      // the solution is most likely to send the proper ResetSequence payload
-      // send(ResetSequence(), None, crypto)
-      // outSequence = -1
-      // return nextSequence
-      outSequence
+    if (outSequence >= 0x8000) {
+      send(ResetSequence(), Some(outSequence), crypto)
+      performResetSequenceReset()
+      0
     } else {
       outSequence += 1
       outSequence
@@ -257,6 +253,16 @@ class MiddlewareActor(
     r
   }
 
+  /**
+   * When the sequence is reset,
+   * the server resets both the expectant packet sequence id and the delivered packet sequence id
+   * and gaslight the client to send packets using refreshed (delivered) id's.
+   */
+  private def performResetSequenceReset(): Unit = {
+    outSequence = 0
+    inSequence = 0
+  }
+
   /**
     * Do not bundle these packets together with other packets
     */
@@ -623,7 +629,7 @@ class MiddlewareActor(
         Behaviors.same
 
       case _: PlanetSideResetSequencePacket =>
-        log.error(s"Unexpected crypto packet: received a PlanetSideResetSequencePacket when it should never happen")
+        performResetSequenceReset()
         Behaviors.same
     }
   }

src/main/scala/net/psforever/packet/PacketCoding.scala

@@ -23,91 +23,97 @@ object PacketCoding {
   final val PLANETSIDE_MIN_PACKET_SIZE = 1
 
   /**
-    * Transform a kind of packet into the sequence of data that represents it.
-    * Wraps around the encoding process for all valid packet container types.
-    * @param packet the packet to encode
-    * @param sequence the packet's sequence number. Must be set for all non ControlPacket packets (but always for encrypted packets).
-    * @param crypto if set, encrypt final payload
-    * @return a `BitVector` translated from the packet's data
-    */
+   * Transform a kind of packet into the sequence of data that represents it.
+   * Wraps around the encoding process for all valid packet container types.
+   * @param packet packet to encode
+   * @param sequenceOpt packet's sequence number;
+   *                    must be set for all packets that are not `ControlPacket`;
+   *                    always for encrypted packets
+   * @param crypto if set, encrypt final payload
+   * @return a `BitVector` translated from the packet's data
+   */
   def marshalPacket(
       packet: PlanetSidePacket,
-      sequence: Option[Int] = None,
+      sequenceOpt: Option[Int] = None,
       crypto: Option[CryptoCoding] = None
   ): Attempt[BitVector] = {
-    val seq = packet match {
-      case _: PlanetSideControlPacket if crypto.isEmpty => BitVector.empty
-      case _: PlanetSideResetSequencePacket             => BitVector.empty
+    packet match {
+      case _: PlanetSideControlPacket if crypto.isEmpty =>
+        marshalPacket(packet, BitVector.empty, crypto)
       case _ =>
-        sequence match {
-          case Some(_sequence) =>
-            uint16L.encode(_sequence) match {
-              case Successful(_seq) => _seq
-              case f @ Failure(_)   => return f
+        sequenceOpt match {
+          case Some(sequence) =>
+            uint16L.encode(sequence) match {
+              case Successful(encodedSequence) => marshalPacket(packet, encodedSequence, crypto)
+              case f: Failure => f
             }
           case None =>
-            return Failure(Err(s"Missing sequence"))
+            Failure(Err(s"can not marshal $packet without a proper sequence id"))
         }
     }
+  }
 
-    val (flags, payload) = packet match {
+  /** Common flags for normal encrypted packets. */
+  private val normalSecuredFlags: BitVector = PlanetSidePacketFlags.codec
+    .encode(PlanetSidePacketFlags(PacketType.Normal, secured = true))
+    .require
+  /** Common flags for normal unencrypted packets. */
+  private val normalNotSecuredFlags: BitVector = PlanetSidePacketFlags.codec
+    .encode(PlanetSidePacketFlags(PacketType.Normal, secured = false))
+    .require
+  /** Common flags for crypto unencrypted packets (that's not illogical). */
+  private val cryptoNotSecuredFlags: BitVector = PlanetSidePacketFlags.codec
+    .encode(PlanetSidePacketFlags(PacketType.Crypto, secured = false))
+    .require
+
+  /**
+   * Transform a kind of packet into the sequence of data that represents it.
+   * @param packet the packet to encode
+   * @param sequence the packet's sequence number. Must be set for all non ControlPacket packets (but always for encrypted packets).
+   * @param crypto if set, encrypt final payload
+   * @return a `BitVector` translated from the packet's data
+   */
+  private def marshalPacket(
+                             packet: PlanetSidePacket,
+                             sequence: BitVector,
+                             crypto: Option[CryptoCoding]
+                           ): Attempt[BitVector] = {
+    packet match {
       case _: PlanetSideGamePacket | _: PlanetSideControlPacket if crypto.isDefined =>
-        encodePacket(packet) match {
-          case Successful(_payload) =>
-            val encryptedPayload = crypto.get.encrypt(_payload.bytes) match {
-              case Successful(p) => p
-              case f: Failure    => return f
-            }
-            (
-              PlanetSidePacketFlags.codec.encode(PlanetSidePacketFlags(PacketType.Normal, secured = true)).require,
-              // encrypted packets need to be aligned to 4 bytes before encryption/decryption
-              // first byte are flags, second is the sequence, and third is the pad
-              hex"00".bits ++ encryptedPayload.bits
-            )
-          case f @ Failure(_) => return f
-        }
+        encodePacket(packet)
+          .flatMap { _payload => crypto.get.encrypt(_payload.bytes) }
+          .flatMap { encryptedPayload =>
+            // these encrypted packets need to be aligned to 4 bytes before encryption/decryption
+            // first byte are flags, second is the sequence, and third is the pad
+            val payload = hex"00".bits ++ encryptedPayload.bits
+            Successful(normalSecuredFlags ++ sequence ++ payload)
+          }
       case packet: PlanetSideGamePacket =>
-        encodePacket(packet) match {
-          case Successful(_payload) =>
-            (
-              PlanetSidePacketFlags.codec.encode(PlanetSidePacketFlags(PacketType.Normal, secured = false)).require,
-              _payload
-            )
-          case f @ Failure(_) => return f
-        }
+        encodePacket(packet)
+          .flatMap { _payload =>
+            Successful(normalNotSecuredFlags ++ sequence ++ _payload)
+          }
       case packet: PlanetSideControlPacket =>
-        encodePacket(packet) match {
-          case Successful(_payload) =>
-            (
-              // control packets don't have flags
-              BitVector.empty,
-              _payload
-            )
-          case f @ Failure(_) => return f
-        }
+        encodePacket(packet)
+          .flatMap { _payload =>
+            Successful(sequence ++ _payload)
+          }
       case packet: PlanetSideCryptoPacket =>
-        encodePacket(packet) match {
-          case Successful(_payload) =>
-            (
-              PlanetSidePacketFlags.codec.encode(PlanetSidePacketFlags(PacketType.Crypto, secured = false)).require,
-              _payload
-            )
-          case f @ Failure(_) => return f
-        }
-      case packet: PlanetSideResetSequencePacket =>
-        encodePacket(packet) match {
-          case Successful(_payload) =>
-            (
-              PlanetSidePacketFlags.codec
-                .encode(PlanetSidePacketFlags(PacketType.ResetSequence, secured = false))
-                .require,
-              _payload
-            )
-          case f @ Failure(_) => return f
-        }
+        encodePacket(packet)
+          .flatMap { _payload =>
+            Successful(cryptoNotSecuredFlags ++ sequence ++ _payload)
+          }
+      case packet: PlanetSideResetSequencePacket if crypto.isDefined =>
+        encodePacket(packet)
+          .flatMap { _payload => crypto.get.encrypt(_payload.bytes) }
+          .flatMap { encryptedPayload =>
+            //uncommon, so it's fine to not cache these flags (above)
+            val flags = PlanetSidePacketFlags.codec.encode(PlanetSidePacketFlags(PacketType.ResetSequence, secured = true)).require
+            Successful(flags ++ sequence ++ encryptedPayload.bits)
+          }
+      case _ =>
+        Failure(Err(s"can not marshal $packet, sequence? $sequence, crypto state? ${crypto.isDefined}"))
     }
-
-    Successful(flags ++ seq ++ payload)
   }
 
   /**
@@ -131,17 +137,16 @@ object PacketCoding {
               case Successful(opcode) => Successful(opcode ++ payload)
               case f @ Failure(_)     => f
             }
-
           case packet: PlanetSideResetSequencePacket =>
             ResetSequenceOpcode.codec.encode(packet.opcode) match {
               case Successful(opcode) => Successful(opcode)
-              case f @ Failure(_)     => f
+              case f: Failure => f
             }
 
           case _ =>
             Failure(Err("packet not supported"))
         }
-      case f @ Failure(_) => f
+      case f: Failure => f
     }
   }
 
@@ -360,6 +365,5 @@ object PacketCoding {
 
       Successful(payloadNoMac)
     }
-
   }
 }