Collision-resistent maps in header parsing (akka#2276)

akka-http-core/src/main/scala/akka/http/impl/model/parser/AcceptHeader.scala

@@ -4,6 +4,8 @@
 
 package akka.http.impl.model.parser
 
+import scala.collection.immutable.TreeMap
+
 import akka.parboiled2.Parser
 import akka.http.scaladsl.model.headers._
 import akka.http.scaladsl.model.{ MediaRange, MediaRanges }
@@ -22,11 +24,11 @@ private[parser] trait AcceptHeader { this: Parser with CommonRules with CommonAc
       if (sub == "*") {
         val mainLower = main.toRootLowerCase
         MediaRanges.getForKey(mainLower) match {
-          case Some(registered) ⇒ if (params.isEmpty) registered else registered.withParams(params.toMap)
-          case None             ⇒ MediaRange.custom(mainLower, params.toMap)
+          case Some(registered) ⇒ if (params.isEmpty) registered else registered.withParams(TreeMap(params: _*))
+          case None             ⇒ MediaRange.custom(mainLower, TreeMap(params: _*))
         }
       } else {
-        val (p, q) = MediaRange.splitOffQValue(params.toMap)
+        val (p, q) = MediaRange.splitOffQValue(TreeMap(params: _*))
         MediaRange(getMediaType(main, sub, p contains "charset", p), q)
       }
     }

akka-http-core/src/main/scala/akka/http/impl/model/parser/CommonRules.scala

@@ -5,6 +5,8 @@
 package akka.http.impl.model.parser
 
 import scala.collection.immutable
+import scala.collection.immutable.TreeMap
+
 import akka.http.scaladsl.model._
 import akka.http.scaladsl.model.headers._
 import akka.parboiled2._
@@ -178,7 +180,7 @@ private[parser] trait CommonRules { this: Parser with StringBuilding ⇒
         case (token, Nil) ⇒ HttpChallenge(scheme, None, Map("" → token))
         case (_, params) ⇒ {
           val (realms, otherParams) = params.partition(_._1 equalsIgnoreCase "realm")
-          HttpChallenge(scheme, realms.headOption.map(_._2), otherParams.toMap)
+          HttpChallenge(scheme, realms.headOption.map(_._2), TreeMap(otherParams: _*))
         }
       }
     }
@@ -228,7 +230,7 @@ private[parser] trait CommonRules { this: Parser with StringBuilding ⇒
   def `generic-credentials` = rule {
     `challenge-or-credentials` ~> ((scheme, tokenAndParams) ⇒ {
       val (token, params) = tokenAndParams
-      GenericHttpCredentials(scheme, token, params.toMap)
+      GenericHttpCredentials(scheme, token, TreeMap(params: _*))
     })
   }
 
@@ -403,7 +405,7 @@ private[parser] trait CommonRules { this: Parser with StringBuilding ⇒
       | `transfer-extension`)
 
   def `transfer-extension` = rule {
-    token ~ zeroOrMore(ws(';') ~ `transfer-parameter`) ~> (_.toMap) ~> (TransferEncodings.Extension(_, _))
+    token ~ zeroOrMore(ws(';') ~ `transfer-parameter`) ~> (p ⇒ TreeMap(p: _*)) ~> (TransferEncodings.Extension(_, _))
   }
 
   def `transfer-parameter` = rule { token ~ ws('=') ~ word ~> (_ → _) }

akka-http-core/src/main/scala/akka/http/impl/model/parser/ContentDispositionHeader.scala

@@ -4,14 +4,16 @@
 
 package akka.http.impl.model.parser
 
+import scala.collection.immutable.TreeMap
+
 import akka.parboiled2.Parser
 import akka.http.scaladsl.model.headers._
 
 private[parser] trait ContentDispositionHeader { this: Parser with CommonRules with CommonActions ⇒
 
   // http://tools.ietf.org/html/rfc6266#section-4.1
   def `content-disposition` = rule {
-    `disposition-type` ~ zeroOrMore(ws(';') ~ `disposition-parm`) ~ EOI ~> (_.toMap) ~> (`Content-Disposition`(_, _))
+    `disposition-type` ~ zeroOrMore(ws(';') ~ `disposition-parm`) ~ EOI ~> (p ⇒ TreeMap(p: _*)) ~> (`Content-Disposition`(_, _))
   }
 
   def `disposition-type` = rule(

akka-http-core/src/main/scala/akka/http/impl/model/parser/ContentTypeHeader.scala

@@ -5,6 +5,8 @@
 package akka.http.impl.model.parser
 
 import scala.annotation.tailrec
+import scala.collection.immutable.TreeMap
+
 import akka.parboiled2.Parser
 import akka.http.scaladsl.model._
 
@@ -35,7 +37,7 @@ private[parser] trait ContentTypeHeader { this: Parser with CommonRules with Com
         contentType(main, sub, tail, Some(getCharset(value)), builder)
 
       case Seq(kvp, tail @ _*) ⇒
-        val b = if (builder eq null) Map.newBuilder[String, String] else builder
+        val b = if (builder eq null) TreeMap.newBuilder[String, String] else builder
         b += kvp
         contentType(main, sub, tail, charset, b)
     }

akka-http-core/src/main/scala/akka/http/impl/model/parser/LinkHeader.scala

@@ -5,6 +5,8 @@
 package akka.http.impl.model.parser
 
 import scala.annotation.tailrec
+import scala.collection.immutable.TreeMap
+
 import akka.parboiled2.Parser
 import akka.http.scaladsl.model.{ ParsingException, IllegalUriException }
 import akka.http.scaladsl.model.headers._
@@ -62,7 +64,7 @@ private[parser] trait LinkHeader { this: Parser with CommonRules with CommonActi
     }
   }
 
-  def `link-media-type` = rule { `media-type` ~> ((mt, st, pm) ⇒ getMediaType(mt, st, pm contains "charset", pm.toMap)) }
+  def `link-media-type` = rule { `media-type` ~> ((mt, st, pm) ⇒ getMediaType(mt, st, pm contains "charset", TreeMap(pm: _*))) }
 
   // filter out subsequent `rel`, `media`, `title`, `type` and `type*` params
   @tailrec private def sanitize(params: Seq[LinkParam], result: Seq[LinkParam] = Nil, seenRel: Boolean = false,

akka-http-core/src/test/scala/akka/http/HashCodeCollider.scala

@@ -0,0 +1,30 @@
+/*
+ * Copyright (C) 2018 Lightbend Inc. <https://www.lightbend.com>
+ */
+
+package akka.http
+
+/**
+ * Helper that creates strings that all share the same hashCode == 0.
+ *
+ * Adapted from MIT-licensed code by Andriy Plokhotnyuk
+ * at https://github.com/plokhotnyuk/jsoniter-scala/blob/26b5ecdd4f8c2ab7e97bd8106cefdda4c1e701ce/jsoniter-scala-benchmark/src/main/scala/com/github/plokhotnyuk/jsoniter_scala/macros/HashCodeCollider.scala#L6.
+ */
+object HashCodeColliderr {
+  val visibleChars = (33 until 127).filterNot(c ⇒ c == '\\' || c == '"')
+  def asciiChars: Iterator[Int] = visibleChars.toIterator
+  def asciiCharsAndHash(previousHash: Int): Iterator[(Int, Int)] = visibleChars.toIterator.map(c ⇒ c -> (previousHash + c) * 31)
+
+  /** Creates an iterator of Strings that all have hashCode == 0 */
+  def zeroHashCodeIterator(): Iterator[String] =
+    for {
+      (i0, h0) ← asciiCharsAndHash(0)
+      (i1, h1) ← asciiCharsAndHash(h0)
+      (i2, h2) ← asciiCharsAndHash(h1) if (((h2 + 32) * 923521) ^ ((h2 + 127) * 923521)) < 0
+      (i3, h3) ← asciiCharsAndHash(h2) if (((h3 + 32) * 29791) ^ ((h3 + 127) * 29791)) < 0
+      (i4, h4) ← asciiCharsAndHash(h3) if (((h4 + 32) * 961) ^ ((h4 + 127) * 961)) < 0
+      (i5, h5) ← asciiCharsAndHash(h4) if (((h5 + 32) * 31) ^ ((h5 + 127) * 31)) < 0
+      (i6, h6) ← asciiCharsAndHash(h5) if ((h6 + 32) ^ (h6 + 127)) < 0
+      (i7, h7) ← asciiCharsAndHash(h6) if h6 + i7 == 0
+    } yield new String(Array(i0, i1, i2, i3, i4, i5, i6, i7).map(_.toChar))
+}

akka-http-core/src/test/scala/akka/http/impl/engine/parsing/HttpHeaderParserSpec.scala

@@ -14,6 +14,7 @@ import scala.util.Random
 import org.scalatest.{ BeforeAndAfterAll, Matchers, WordSpec }
 import akka.util.ByteString
 import akka.actor.ActorSystem
+import akka.http.HashCodeColliderr
 import akka.http.scaladsl.model.{ ErrorInfo, HttpHeader }
 import akka.http.scaladsl.model.headers._
 import akka.http.impl.model.parser.CharacterClasses
@@ -272,10 +273,53 @@ abstract class HttpHeaderParserSpec(mode: String, newLine: String) extends WordS
         parseLine(s"Content-Type: abc:123${newLine}x")
       }
     }
+    "not show bad performance characteristics when parameter names' hashCodes collide" in new TestSetup(
+      parserSettings = createParserSettings(system).withMaxHeaderValueLength(500 * 1024)
+    ) {
+      // This is actually quite rare since it needs upping the max header value length
+      val numKeys = 10000
+      val value = "null"
+
+      val regularKeys = Iterator.from(1).map(i ⇒ s"key_$i").take(numKeys)
+      private val zeroHashStrings: Iterator[String] = HashCodeColliderr.zeroHashCodeIterator()
+      val collidingKeys = zeroHashStrings
+        .filter(_.forall(CharacterClasses.tchar))
+        .take(numKeys)
+
+      def createHeader(keys: Iterator[String]): String = "Accept: text/plain" + keys.mkString(";", "=x;", "=x") + newLine + "x"
+
+      val regularHeader = createHeader(regularKeys)
+      val collidingHeader = createHeader(collidingKeys)
+      zeroHashStrings.next().hashCode should be(0)
+
+      val regularTime = nanoBench {
+        val (_, accept: Accept) = parseLine(regularHeader)
+        accept.mediaRanges.head.getParams.size should be(numKeys)
+      }
+      val collidingTime = nanoBench {
+        val (_, accept: Accept) = parseLine(collidingHeader)
+        accept.mediaRanges.head.getParams.size should be(numKeys)
+      }
+
+      collidingTime / regularTime should be < 2L // speed must be in same order of magnitude
+    }
   }
 
   override def afterAll() = TestKit.shutdownActorSystem(system)
 
+  def nanoBench(block: ⇒ Unit): Long = {
+    // great microbenchmark (the comment must be kept, otherwise it's not true)
+    val f = block _
+
+    // warmup
+    (1 to 10).foreach(_ ⇒ f())
+
+    val start = System.nanoTime()
+    f()
+    val end = System.nanoTime()
+    end - start
+  }
+
   def check(pair: (String, String)) = {
     val (expected, actual) = pair
     actual shouldEqual expected.stripMarginWithNewline("\n")

akka-http-core/src/test/scala/akka/http/impl/model/parser/HttpHeaderSpec.scala

@@ -141,12 +141,12 @@ class HttpHeaderSpec extends FreeSpec with Matchers {
       "Authorization: Fancy QWxhZGRpbjpvcGVuIHNlc2FtZQ==" =!=
         Authorization(GenericHttpCredentials("Fancy", "QWxhZGRpbjpvcGVuIHNlc2FtZQ=="))
       """Authorization: Fancy yes="n:o", nonce=42""" =!=
-        Authorization(GenericHttpCredentials("Fancy", Map("yes" → "n:o", "nonce" → "42"))).renderedTo(
-          """Fancy yes="n:o",nonce=42""")
-      """Authorization: Fancy yes=no,nonce="4\\2"""" =!=
-        Authorization(GenericHttpCredentials("Fancy", Map("yes" → "no", "nonce" → """4\2""")))
-      """Authorization: Other yes=no,empty=""""" =!=
-        Authorization(GenericHttpCredentials("Other", Map("yes" → "no", "empty" → "")))
+        Authorization(GenericHttpCredentials("Fancy", Map("nonce" → "42", "yes" → "n:o"))).renderedTo(
+          """Fancy nonce=42,yes="n:o"""")
+      """Authorization: Fancy nonce="4\\2",yes=no""" =!=
+        Authorization(GenericHttpCredentials("Fancy", Map("nonce" → """4\2""", "yes" → "no")))
+      """Authorization: Other empty="",yes=no""" =!=
+        Authorization(GenericHttpCredentials("Other", Map("empty" → "", "yes" → "no")))
       "Authorization: Basic Qm9iOg==" =!=
         Authorization(BasicHttpCredentials("Bob", ""))
       """Authorization: Digest name=Bob""" =!=
@@ -191,8 +191,8 @@ class HttpHeaderSpec extends FreeSpec with Matchers {
 
     "Content-Disposition" in {
       "Content-Disposition: form-data" =!= `Content-Disposition`(ContentDispositionTypes.`form-data`)
-      "Content-Disposition: attachment; name=\"field1\"; filename=\"file/txt\"" =!=
-        `Content-Disposition`(ContentDispositionTypes.attachment, Map("name" → "field1", "filename" → "file/txt"))
+      "Content-Disposition: attachment; filename=\"file/txt\"; name=\"field1\"" =!=
+        `Content-Disposition`(ContentDispositionTypes.attachment, Map("filename" → "file/txt", "name" → "field1"))
       "Content-Disposition: attachment; name=\"field1\"; other=\"\"" =!=
         `Content-Disposition`(ContentDispositionTypes.attachment, Map("name" → "field1", "other" → ""))
     }
@@ -407,8 +407,8 @@ class HttpHeaderSpec extends FreeSpec with Matchers {
     }
 
     "Proxy-Authorization" in {
-      """Proxy-Authorization: Fancy yes=no,nonce="4\\2"""" =!=
-        `Proxy-Authorization`(GenericHttpCredentials("Fancy", Map("yes" → "no", "nonce" → """4\2""")))
+      """Proxy-Authorization: Fancy nonce="4\\2",yes=no""" =!=
+        `Proxy-Authorization`(GenericHttpCredentials("Fancy", Map("nonce" → """4\2""", "yes" → "no")))
       "Proxy-Authorization: Fancy QWxhZGRpbjpvcGVuIHNlc2FtZQ==" =!=
         `Proxy-Authorization`(GenericHttpCredentials("Fancy", "QWxhZGRpbjpvcGVuIHNlc2FtZQ=="))
     }
@@ -594,13 +594,15 @@ class HttpHeaderSpec extends FreeSpec with Matchers {
                            nonce=dcd98b7102dd2f0e8b11d0f600bfb0c093,
                            opaque=5ccc069c403ebaf9f0171e9517f40e41""".stripMarginWithNewline("\r\n") =!=
         `WWW-Authenticate`(HttpChallenge("Digest", "testrealm@host.com", Map(
-          "qop" → "auth,auth-int",
-          "nonce" → "dcd98b7102dd2f0e8b11d0f600bfb0c093", "opaque" → "5ccc069c403ebaf9f0171e9517f40e41"))).renderedTo(
-          "Digest realm=\"testrealm@host.com\",qop=\"auth,auth-int\",nonce=dcd98b7102dd2f0e8b11d0f600bfb0c093,opaque=5ccc069c403ebaf9f0171e9517f40e41")
+          "nonce" → "dcd98b7102dd2f0e8b11d0f600bfb0c093",
+          "opaque" → "5ccc069c403ebaf9f0171e9517f40e41",
+          "qop" → "auth,auth-int"
+        ))).renderedTo(
+          "Digest realm=\"testrealm@host.com\",nonce=dcd98b7102dd2f0e8b11d0f600bfb0c093,opaque=5ccc069c403ebaf9f0171e9517f40e41,qop=\"auth,auth-int\"")
       "WWW-Authenticate: Basic realm=\"WallyWorld\",attr=\"val>ue\", Fancy realm=\"yeah\"" =!=
         `WWW-Authenticate`(HttpChallenge("Basic", Some("WallyWorld"), Map("attr" → "val>ue")), HttpChallenge("Fancy", Some("yeah")))
-      "WWW-Authenticate: Basic attr=value,another=\"val>ue\"" =!=
-        `WWW-Authenticate`(HttpChallenge("Basic", None, Map("attr" → "value", "another" → "val>ue")))
+      "WWW-Authenticate: Basic another=\"val>ue\",attr=value" =!=
+        `WWW-Authenticate`(HttpChallenge("Basic", None, Map("another" → "val>ue", "attr" → "value")))
       "WWW-Authenticate: Basic attr=value" =!=
         `WWW-Authenticate`(HttpChallenge("Basic", None, Map("attr" → "value")))
       """WWW-Authenticate: Fancy realm="Secure Area",nonce=42""" =!=