diff --git a/actor-tests/src/test/scala/org/apache/pekko/actor/ActorPathSpec.scala b/actor-tests/src/test/scala/org/apache/pekko/actor/ActorPathSpec.scala index a7573979fbf..0fbba1f7ba8 100644 --- a/actor-tests/src/test/scala/org/apache/pekko/actor/ActorPathSpec.scala +++ b/actor-tests/src/test/scala/org/apache/pekko/actor/ActorPathSpec.scala @@ -16,9 +16,10 @@ package org.apache.pekko.actor import java.net.MalformedURLException import org.scalatest.matchers.should.Matchers +import org.scalatest.prop.TableDrivenPropertyChecks import org.scalatest.wordspec.AnyWordSpec -class ActorPathSpec extends AnyWordSpec with Matchers { +class ActorPathSpec extends AnyWordSpec with Matchers with TableDrivenPropertyChecks { "An ActorPath" must { @@ -81,6 +82,57 @@ class ActorPathSpec extends AnyWordSpec with Matchers { "must not be empty") } + "accept valid path elements" in { + val valid = Table( + "name", + "a", + "actor-1", + "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789", + "-_.*$+:@&=,!~';", + "a$" + "b", + "%20", + "%ff%FF%0a", + "a%20b", + "x" * 10000) + forAll(valid) { name => + ActorPath.isValidPathElement(name) should ===(true) + ActorPath.validatePathElement(name) + } + } + + "reject invalid path elements and report the position" in { + def positionOf(name: String): Int = + intercept[InvalidActorNameException](ActorPath.validatePathElement(name)).getMessage match { + case msg if msg.contains("at position: ") => + msg.split("at position: ")(1).takeWhile(_.isDigit).toInt + case msg => fail(s"unexpected message [$msg]") + } + + val invalid = Table( + ("name", "position"), + ("$" + "a", 0), // `$` is reserved for system names at the start + ("a b", 1), + ("a/b", 1), + ("a#b", 1), + ("a?b", 1), + ("a%", 1), // `%` is only valid as `%XX` + ("a%2", 1), + ("a%2g", 1), + ("a%g2", 1), + ("caf\u00e9", 3), // Latin-1 but not ASCII + ("na\u00efve-actor", 2), + ("\u4e2d\u6587", 0), // outside Latin-1 + ("actor-\u4e2d", 6), + ("actor-\ud83d\ude00", 6), // surrogate pair + ("a\u0000b", 1), + ("a\u007fb", 1), + ("x" * 100 + "\u00e9", 100)) + forAll(invalid) { (name, position) => + ActorPath.isValidPathElement(name) should ===(false) + positionOf(name) should ===(position) + } + } + "create correct toStringWithAddress" in { val local = Address("pekko", "mysys") val a = local.copy(host = Some("aaa"), port = Some(7355)) diff --git a/actor/src/main/scala/org/apache/pekko/actor/ActorPath.scala b/actor/src/main/scala/org/apache/pekko/actor/ActorPath.scala index ef43038b546..2ecc4a5cd1d 100644 --- a/actor/src/main/scala/org/apache/pekko/actor/ActorPath.scala +++ b/actor/src/main/scala/org/apache/pekko/actor/ActorPath.scala @@ -14,6 +14,7 @@ package org.apache.pekko.actor import java.lang.{ StringBuilder => JStringBuilder } import java.net.MalformedURLException +import java.nio.charset.StandardCharsets import scala.annotation.nowarn import scala.annotation.tailrec @@ -79,6 +80,26 @@ object ActorPath { private final val ValidPathCode = -1 private final val EmptyPathCode = -2 + private final val ValidCharFlag: Byte = 1 + private final val HexCharFlag: Byte = 2 + + /** + * Lookup table over the 7-bit ASCII range: bit 0 marks a character allowed in a path element, + * bit 1 marks a hex digit (for `%XX` escapes). Everything at or above 0x80 is invalid. + */ + private final val PathCharFlags: Array[Byte] = { + val flags = new Array[Byte](128) + def set(c: Char, flag: Byte): Unit = flags(c) = (flags(c) | flag).toByte + ('a' to 'z').foreach(set(_, ValidCharFlag)) + ('A' to 'Z').foreach(set(_, ValidCharFlag)) + ('0' to '9').foreach(set(_, ValidCharFlag)) + ValidSymbols.foreach(set(_, ValidCharFlag)) + ('a' to 'f').foreach(set(_, HexCharFlag)) + ('A' to 'F').foreach(set(_, HexCharFlag)) + ('0' to '9').foreach(set(_, HexCharFlag)) + flags + } + /** * Validates the given actor path element and throws an [[InvalidActorNameException]] if invalid. * See [[#isValidPathElement]] for a non-throwing version. @@ -127,29 +148,29 @@ object ActorPath { private final def findInvalidPathElementCharPosition(s: String): Int = if (s.isEmpty) EmptyPathCode else { - def isValidChar(c: Char): Boolean = - (c >= 'a' && c <= 'z') || - (c >= 'A' && c <= 'Z') || - (c >= '0' && c <= '9') || - (ValidSymbols.indexOf(c) != -1) - - def isHexChar(c: Char): Boolean = - (c >= 'a' && c <= 'f') || - (c >= 'A' && c <= 'F') || - (c >= '0' && c <= '9') - - val len = s.length - def validate(pos: Int): Int = - if (pos < len) - s.charAt(pos) match { - case c if isValidChar(c) => validate(pos + 1) - case '%' if pos + 2 < len && isHexChar(s.charAt(pos + 1)) && isHexChar(s.charAt(pos + 2)) => - validate(pos + 3) - case _ => pos - } - else ValidPathCode - - if (len > 0 && s.charAt(0) != '$') validate(0) else 0 + // Copy the string out once (an intrinsic array copy for Latin-1 strings) and scan the byte + // array with a table lookup. This avoids `String.charAt` in the loop, whose Latin-1/UTF-16 + // coder branch shares one JVM-wide profile and is easily polluted by unrelated non-ASCII + // strings. Characters outside Latin-1 are encoded as '?', which is invalid, and characters + // in 0x80-0xFF are negative bytes, which are also invalid, so positions are preserved. + val bytes = s.getBytes(StandardCharsets.ISO_8859_1) + val len = bytes.length + val flags = PathCharFlags + + def flagsAt(pos: Int): Int = { + val b = bytes(pos) + if (b >= 0) flags(b) else 0 + } + + @tailrec def validate(pos: Int): Int = + if (pos >= len) ValidPathCode + else if ((flagsAt(pos) & ValidCharFlag) != 0) validate(pos + 1) + else if (bytes(pos) == '%' && pos + 2 < len && + (flagsAt(pos + 1) & HexCharFlag) != 0 && (flagsAt(pos + 2) & HexCharFlag) != 0) + validate(pos + 3) + else pos + + if (bytes(0) != '$') validate(0) else 0 } private[pekko] final val emptyActorPath: immutable.Iterable[String] = List("") diff --git a/bench-jmh/src/main/scala/org/apache/pekko/actor/ActorPathValidationBenchmark.scala b/bench-jmh/src/main/scala/org/apache/pekko/actor/ActorPathValidationBenchmark.scala index 780bbf096cd..976fd1f35ea 100644 --- a/bench-jmh/src/main/scala/org/apache/pekko/actor/ActorPathValidationBenchmark.scala +++ b/bench-jmh/src/main/scala/org/apache/pekko/actor/ActorPathValidationBenchmark.scala @@ -22,6 +22,8 @@ import org.openjdk.jmh.annotations.Fork import org.openjdk.jmh.annotations.Measurement import org.openjdk.jmh.annotations.Mode import org.openjdk.jmh.annotations.OutputTimeUnit +import org.openjdk.jmh.annotations.Param +import org.openjdk.jmh.annotations.Setup import org.openjdk.jmh.annotations.State import org.openjdk.jmh.annotations.Warmup @@ -32,6 +34,21 @@ import org.openjdk.jmh.annotations.Warmup [info] a.a.ActorPathValidationBenchmark.handLoopActor_1 thrpt 20 38.825 3.378 ops/us [info] a.a.ActorPathValidationBenchmark.oldActor_1 thrpt 20 1.585 0.090 ops/us + +`polluted = true` first exercises String.charAt with UTF-16 strings so its Latin-1/UTF-16 coder branch +profile (collected once, JVM-wide, in String.charAt's own bytecode) sees both coders, as it would in +any process that ever handles non-ASCII text. charAt-based scanners then get both coders compiled into +their loop; the byte-table scanner (isValidPathElement) is unaffected: + +[info] Benchmark (polluted) Mode Cnt Score Error Units +[info] ActorPathValidationBenchmark.charAtLoop7000 false thrpt 5 0.197 ± 0.028 ops/us +[info] ActorPathValidationBenchmark.charAtLoop7000 true thrpt 5 0.107 ± 0.017 ops/us +[info] ActorPathValidationBenchmark.charAtLoopActor_1 false thrpt 5 100.954 ± 14.868 ops/us +[info] ActorPathValidationBenchmark.charAtLoopActor_1 true thrpt 5 49.306 ± 8.706 ops/us +[info] ActorPathValidationBenchmark.handLoop7000 false thrpt 5 0.176 ± 0.012 ops/us +[info] ActorPathValidationBenchmark.handLoop7000 true thrpt 5 0.142 ± 0.094 ops/us +[info] ActorPathValidationBenchmark.handLoopActor_1 false thrpt 5 138.841 ± 111.054 ops/us +[info] ActorPathValidationBenchmark.handLoopActor_1 true thrpt 5 162.126 ± 281.298 ops/us */ @Fork(2) @State(JmhScope.Benchmark) @@ -41,21 +58,75 @@ import org.openjdk.jmh.annotations.Warmup @OutputTimeUnit(TimeUnit.MICROSECONDS) class ActorPathValidationBenchmark { + @Param(Array("false", "true")) + var polluted: Boolean = false + final val a = "actor-1" final val s = "687474703a2f2f74686566727569742e636f6d2f26683d37617165716378357926656e" * 100 final val ElementRegex = """(?:[-\w:@&=+,.!~*'_;]|%\p{XDigit}{2})(?:[-\w:@&=+,.!~*'$_;]|%\p{XDigit}{2})*""".r + @Setup + def setup(): Unit = { + if (polluted) { + val utf16 = "årsrapport-ünïcödé-中文-é" + var acc = 0 + var i = 0 + while (i < 2000000) { + acc += (if (charAtLoop(utf16 + i)) 1 else 0) + utf16.charAt(i % utf16.length) + i += 1 + } + if (acc == 42) println("unlikely") + } + } + // @Benchmark // blows up with stack overflow, we know def old7000: Option[List[String]] = ElementRegex.unapplySeq(s) @Benchmark def handLoop7000: Boolean = ActorPath.isValidPathElement(s) + @Benchmark + def charAtLoop7000: Boolean = charAtLoop(s) + @Benchmark def oldActor_1: Option[List[String]] = ElementRegex.unapplySeq(a) @Benchmark def handLoopActor_1: Boolean = ActorPath.isValidPathElement(a) + @Benchmark + def charAtLoopActor_1: Boolean = charAtLoop(a) + + // the String.charAt based validator that isValidPathElement used before the byte-table version + private final val ValidSymbols = """-_.*$+:@&=,!~';""" + + private def charAtLoop(s: String): Boolean = + if (s.isEmpty) false + else { + def isValidChar(c: Char): Boolean = + (c >= 'a' && c <= 'z') || + (c >= 'A' && c <= 'Z') || + (c >= '0' && c <= '9') || + (ValidSymbols.indexOf(c) != -1) + + def isHexChar(c: Char): Boolean = + (c >= 'a' && c <= 'f') || + (c >= 'A' && c <= 'F') || + (c >= '0' && c <= '9') + + val len = s.length + def validate(pos: Int): Int = + if (pos < len) + s.charAt(pos) match { + case c if isValidChar(c) => validate(pos + 1) + case '%' if pos + 2 < len && isHexChar(s.charAt(pos + 1)) && isHexChar(s.charAt(pos + 2)) => + validate(pos + 3) + case _ => pos + } + else -1 + + (if (len > 0 && s.charAt(0) != '$') validate(0) else 0) == -1 + } + }