From d4b6371749fa0e34e6e881418d60153fd2e8fe63 Mon Sep 17 00:00:00 2001 From: Matt Farmer Date: Sun, 31 May 2026 15:11:19 -0400 Subject: [PATCH 1/2] test: add tests for latent bugs identified by ai --- .../liftweb/actor/LatentCollectBugSpec.scala | 57 ++++++++++++++ .../net/liftweb/util/LatentBugsSpec.scala | 78 +++++++++++++++++++ 2 files changed, 135 insertions(+) create mode 100644 core/actor/src/test/scala/net/liftweb/actor/LatentCollectBugSpec.scala create mode 100644 core/util/src/test/scala/net/liftweb/util/LatentBugsSpec.scala diff --git a/core/actor/src/test/scala/net/liftweb/actor/LatentCollectBugSpec.scala b/core/actor/src/test/scala/net/liftweb/actor/LatentCollectBugSpec.scala new file mode 100644 index 000000000..ef40628ae --- /dev/null +++ b/core/actor/src/test/scala/net/liftweb/actor/LatentCollectBugSpec.scala @@ -0,0 +1,57 @@ +/* + * Copyright 2009-2026 Lift Committers and Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package net.liftweb +package actor + +import org.specs2.mutable.Specification + +import common._ + +/** + * Validates Bug 3: LAFuture.collect over-allocates its accumulator + * (`for (i <- 0 to len)` at LAFuture.scala:390 creates len+1 slots) and uses + * `insert` rather than `update` at the success site, so collected values are + * ordered by completion order rather than by input order. + * + * This example is expected to FAIL while the bug is present. + */ +class LatentCollectBugSpec extends Specification { + "LAFuture.collect Specification".title + + // A scheduler that runs callbacks inline on the calling thread, so that the + // order of satisfy() calls deterministically drives collect()'s accumulation. + private val inline = new LAScheduler { + def execute(f: () => Unit): Unit = f() + } + + "LAFuture.collect" should { + "preserve input order regardless of completion order" in { + val f1 = new LAFuture[String](inline) + val f2 = new LAFuture[String](inline) + val f3 = new LAFuture[String](inline) + + val collected = LAFuture.collect(f1, f2, f3) + + // Complete out of order: middle, then first, then last. + f2.satisfy("b") + f1.satisfy("a") + f3.satisfy("c") + + collected.get(5000L) must_== Full(List("a", "b", "c")) + } + } +} diff --git a/core/util/src/test/scala/net/liftweb/util/LatentBugsSpec.scala b/core/util/src/test/scala/net/liftweb/util/LatentBugsSpec.scala new file mode 100644 index 000000000..f6d9f6baa --- /dev/null +++ b/core/util/src/test/scala/net/liftweb/util/LatentBugsSpec.scala @@ -0,0 +1,78 @@ +/* + * Copyright 2006-2026 Lift Committers and Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package net.liftweb +package util + +import org.specs2.mutable.Specification + +/** + * Each example below is expected to FAIL while the corresponding latent bug is + * present, and to pass once the bug is fixed. + */ +class LatentBugsSpec extends Specification { + "Latent bug validation".title + + "Bug 1: SecurityHelpers.randomLong / randomInt" should { + // SecurityHelpers.scala:49,52 compute `math.abs(random.nextLong) % mod`. + // SecureRandom.nextLong can return Long.MinValue, and math.abs(Long.MinValue) + // overflows back to Long.MinValue (still negative), so the result of the + // modulo can be negative -- violating the documented "modulo a number" + // contract. Reproduced deterministically with the worst-case RNG output, + // which is exactly the value the production expression mishandles. + "never yield a negative value, even when the RNG returns Long.MinValue" in { + val worstCaseNextLong: Long = Long.MinValue + (math.abs(worstCaseNextLong) % 7L) must be_>=(0L) + } + "never yield a negative value, even when the RNG returns Int.MinValue" in { + val worstCaseNextInt: Int = Int.MinValue + (math.abs(worstCaseNextInt) % 7) must be_>=(0) + } + } + + "Bug 2: CurrencyZone.get / round (EU / German locale)" should { + // CurrencyZone.scala:147 uses replaceAll (which takes a *regex*) with the + // locale grouping separator. For EU (Locale.GERMANY) the grouping separator + // is '.', so the regex matches every character and wipes out the number. + "keep the digits of a formatted EU amount instead of erasing them" in { + EU("1234.56").get must beMatching(".*1234.*") + } + "not throw when rounding a EU amount" in { + // round(p) = make(BigDecimal(get(p))); with the bug get(p) == "" and + // BigDecimal("") throws NumberFormatException. + EU("1234.56").round(2) must not(throwA[NumberFormatException]) + } + } + + "Bug 4: TimeSpan.format" should { + import Helpers._ + // TimeHelpers.scala:264 caps the final "week" unit with a divisor of 10000, + // so weeks are reported modulo 10000 and whole 10000-week blocks disappear. + "report all weeks for a duration of exactly 10000 weeks" in { + TimeSpan.format(weeks(10000L)) must_== "10000 weeks" + } + } + + "Bug 5: StringHelpers.splitNameValuePairs" should { + import StringHelpers._ + // StringHelpers.scala:54-62 reads pair(1) without checking that a value was + // actually present, so a malformed entry throws IndexOutOfBoundsException + // instead of being handled gracefully. + "not throw on an entry that has no '=' value" in { + splitNameValuePairs("foo") must not(throwAn[IndexOutOfBoundsException]) + } + } +} From 54894df2bff161b084259a866c6562a8a836b114 Mon Sep 17 00:00:00 2001 From: Matt Farmer Date: Sun, 31 May 2026 18:37:56 -0400 Subject: [PATCH 2/2] fix: resolve five latent bugs in util and actor modules Baseline tests passed for all real code on Scala 2.13.18 and 3.3.7; the only failures were latent-bug regression specs. Fix the underlying bugs and repair the specs so the full suite is green on both Scala versions. - SecurityHelpers.randomLong/randomInt: math.abs(MinValue) overflows and stays negative, so the modulo could be negative. Reduce via math.floorMod through a new nonNegativeMod helper. - CurrencyZone.get: replaceAll treated the grouping separator as a regex (German '.' erased every digit). Strip it literally, normalise the decimal separator to '.', and drop residual non-numeric characters so round()'s BigDecimal parse no longer throws for EU locales. - TimeHelpers TimeSpan.format: week scale divisor of 10000 wrapped weeks; use Long.MaxValue so the largest unit never caps. - StringHelpers.splitNameValuePairs: reading pair(1) threw IndexOutOfBounds for entries with no '='; pattern-match and handle the missing-value case. - LAFuture.collect: over-allocated the accumulator (0 to len) and used insert (which shifts) instead of update, returning results in completion order. Use 0 until len + update to preserve input order. Specs exercise the real APIs and use cross-version beEqualTo. Co-Authored-By: Claude Opus 4.8 --- .../scala/net/liftweb/actor/LAFuture.scala | 8 +++---- .../liftweb/actor/LatentCollectBugSpec.scala | 2 +- .../scala/net/liftweb/util/CurrencyZone.scala | 17 ++++++++++++--- .../net/liftweb/util/SecurityHelpers.scala | 12 +++++++++-- .../net/liftweb/util/StringHelpers.scala | 9 +++++--- .../scala/net/liftweb/util/TimeHelpers.scala | 2 +- .../net/liftweb/util/LatentBugsSpec.scala | 21 +++++++++---------- 7 files changed, 46 insertions(+), 25 deletions(-) diff --git a/core/actor/src/main/scala/net/liftweb/actor/LAFuture.scala b/core/actor/src/main/scala/net/liftweb/actor/LAFuture.scala index 0d3fa5713..ff7ce812f 100644 --- a/core/actor/src/main/scala/net/liftweb/actor/LAFuture.scala +++ b/core/actor/src/main/scala/net/liftweb/actor/LAFuture.scala @@ -386,8 +386,8 @@ object LAFuture { val sync = new Object val len = futures.length val accumulator = new ArrayBuffer[Box[T]](len) - // pad array so inserts at random places are possible - for (i <- 0 to len) { accumulator.insert(i, Empty) } + // pad array so values can be placed at their input index + for (i <- 0 until len) { accumulator.insert(i, Empty) } var gotCnt = 0 futures.toList.zipWithIndex.foreach { @@ -435,7 +435,7 @@ object LAFuture { def collect[T](future: LAFuture[T]*): LAFuture[List[T]] = { collect[T, List[T]]( onFutureSucceeded = { (value, result, values, index) => - values.insert(index, Full(value)) + values.update(index, Full(value)) }, onFutureFailed = { (valueBox, result, values, index) => result.fail(valueBox) }, onAllFuturesCompleted = { (result, values) => result.satisfy(values.toList.flatten) }, @@ -455,7 +455,7 @@ object LAFuture { onFutureSucceeded = { (value, result, values, index) => value match { case Full(realValue) => - values.insert(index, Full(Full(realValue))) + values.update(index, Full(Full(realValue))) case other: EmptyBox => result.satisfy(other) } diff --git a/core/actor/src/test/scala/net/liftweb/actor/LatentCollectBugSpec.scala b/core/actor/src/test/scala/net/liftweb/actor/LatentCollectBugSpec.scala index ef40628ae..d4379a9d0 100644 --- a/core/actor/src/test/scala/net/liftweb/actor/LatentCollectBugSpec.scala +++ b/core/actor/src/test/scala/net/liftweb/actor/LatentCollectBugSpec.scala @@ -51,7 +51,7 @@ class LatentCollectBugSpec extends Specification { f1.satisfy("a") f3.satisfy("c") - collected.get(5000L) must_== Full(List("a", "b", "c")) + collected.get(5000L) must beEqualTo(Full(List("a", "b", "c"))) } } } diff --git a/core/util/src/main/scala/net/liftweb/util/CurrencyZone.scala b/core/util/src/main/scala/net/liftweb/util/CurrencyZone.scala index ade97de5d..bd1ae6f71 100644 --- a/core/util/src/main/scala/net/liftweb/util/CurrencyZone.scala +++ b/core/util/src/main/scala/net/liftweb/util/CurrencyZone.scala @@ -142,9 +142,20 @@ abstract class CurrencyZone { def get(numberOfFractionDigits: Int): String = { val nf = NumberFormat.getNumberInstance(_locale) val df = nf.asInstanceOf[DecimalFormat] - val groupingSeparator = df.getDecimalFormatSymbols.getGroupingSeparator - - format("", numberOfFractionDigits).replaceAll(groupingSeparator.toString+"", "") + val symbols = df.getDecimalFormatSymbols + val groupingSeparator = symbols.getGroupingSeparator + val decimalSeparator = symbols.getDecimalSeparator + + // Strip the grouping separator (as a literal, not a regex), + // normalise the locale-specific decimal separator to '.', and drop + // any remaining non-numeric characters (e.g. the non-breaking space + // German currency formatting leaves behind), so the result is always + // a valid BigDecimal string -- even for locales such as + // Locale.GERMANY where the separators are '.' and ','. + format("", numberOfFractionDigits) + .replace(groupingSeparator.toString, "") + .replace(decimalSeparator.toString, ".") + .replaceAll("[^0-9.+-]", "") } } diff --git a/core/util/src/main/scala/net/liftweb/util/SecurityHelpers.scala b/core/util/src/main/scala/net/liftweb/util/SecurityHelpers.scala index 32d15a634..8e19f68c6 100644 --- a/core/util/src/main/scala/net/liftweb/util/SecurityHelpers.scala +++ b/core/util/src/main/scala/net/liftweb/util/SecurityHelpers.scala @@ -45,11 +45,19 @@ trait SecurityHelpers { private def withRandom[T](f: SecureRandom => T): T = _random.synchronized(f(_random)) + /** + * Reduce a (possibly negative) value into the range `[0, mod)`. Unlike + * `math.abs(value) % mod`, this is safe when `value` is `MinValue`, where + * `math.abs` overflows back to a negative number. + */ + private[util] def nonNegativeMod(value: Long, mod: Long): Long = math.floorMod(value, mod) + private[util] def nonNegativeMod(value: Int, mod: Int): Int = math.floorMod(value, mod) + /** return a random Long modulo a number */ - def randomLong(mod: Long): Long = withRandom(random => math.abs(random.nextLong) % mod) + def randomLong(mod: Long): Long = withRandom(random => nonNegativeMod(random.nextLong, mod)) /** return a random int modulo a number */ - def randomInt(mod: Int): Int = withRandom(random => math.abs(random.nextInt) % mod) + def randomInt(mod: Int): Int = withRandom(random => nonNegativeMod(random.nextInt, mod)) /** * return true only 'percent' times when asked repeatedly. diff --git a/core/util/src/main/scala/net/liftweb/util/StringHelpers.scala b/core/util/src/main/scala/net/liftweb/util/StringHelpers.scala index b58fb28a8..50dda3d19 100644 --- a/core/util/src/main/scala/net/liftweb/util/StringHelpers.scala +++ b/core/util/src/main/scala/net/liftweb/util/StringHelpers.scala @@ -52,9 +52,12 @@ trait StringHelpers { * The result is a Map[String, String] */ def splitNameValuePairs(props: String): Map[String, String] = { - val list = props.split(",").toList.map(in => { - val pair = in.roboSplit("=") - (pair(0), unquote(pair(1))) + val list = props.split(",").toList.flatMap(in => { + in.roboSplit("=") match { + case name :: value :: _ => Some((name, unquote(value))) + case name :: Nil if name.nonEmpty => Some((name, "")) + case _ => None + } }) val map: Map[String, String] = Map.empty diff --git a/core/util/src/main/scala/net/liftweb/util/TimeHelpers.scala b/core/util/src/main/scala/net/liftweb/util/TimeHelpers.scala index f4dd4e1b5..d2beada62 100644 --- a/core/util/src/main/scala/net/liftweb/util/TimeHelpers.scala +++ b/core/util/src/main/scala/net/liftweb/util/TimeHelpers.scala @@ -261,7 +261,7 @@ trait TimeHelpers { self: ControlHelpers => */ object TimeSpan { /** time units and values used when converting a total number of millis to those units (see the format function) */ - val scales = List((1000L, "milli"), (60L, "second"), (60L, "minute"), (24L, "hour"), (7L, "day"), (10000L, "week")) + val scales = List((1000L, "milli"), (60L, "second"), (60L, "minute"), (24L, "hour"), (7L, "day"), (Long.MaxValue, "week")) /** explicit constructor for a TimeSpan */ def apply(in: Long) = new TimeSpan(in) diff --git a/core/util/src/test/scala/net/liftweb/util/LatentBugsSpec.scala b/core/util/src/test/scala/net/liftweb/util/LatentBugsSpec.scala index f6d9f6baa..a896e343e 100644 --- a/core/util/src/test/scala/net/liftweb/util/LatentBugsSpec.scala +++ b/core/util/src/test/scala/net/liftweb/util/LatentBugsSpec.scala @@ -27,19 +27,18 @@ class LatentBugsSpec extends Specification { "Latent bug validation".title "Bug 1: SecurityHelpers.randomLong / randomInt" should { - // SecurityHelpers.scala:49,52 compute `math.abs(random.nextLong) % mod`. - // SecureRandom.nextLong can return Long.MinValue, and math.abs(Long.MinValue) - // overflows back to Long.MinValue (still negative), so the result of the - // modulo can be negative -- violating the documented "modulo a number" - // contract. Reproduced deterministically with the worst-case RNG output, - // which is exactly the value the production expression mishandles. + // randomLong / randomInt reduce the raw RNG output into [0, mod) via + // Helpers.nonNegativeMod, the exact reduction those methods apply. + // SecureRandom.nextLong can return Long.MinValue, and the old + // `math.abs(value) % mod` overflowed (math.abs(Long.MinValue) is still + // negative), yielding a negative result and violating the documented + // "modulo a number" contract. Exercised deterministically with the + // worst-case RNG output. "never yield a negative value, even when the RNG returns Long.MinValue" in { - val worstCaseNextLong: Long = Long.MinValue - (math.abs(worstCaseNextLong) % 7L) must be_>=(0L) + Helpers.nonNegativeMod(Long.MinValue, 7L) must be_>=(0L) } "never yield a negative value, even when the RNG returns Int.MinValue" in { - val worstCaseNextInt: Int = Int.MinValue - (math.abs(worstCaseNextInt) % 7) must be_>=(0) + Helpers.nonNegativeMod(Int.MinValue, 7) must be_>=(0) } } @@ -62,7 +61,7 @@ class LatentBugsSpec extends Specification { // TimeHelpers.scala:264 caps the final "week" unit with a divisor of 10000, // so weeks are reported modulo 10000 and whole 10000-week blocks disappear. "report all weeks for a duration of exactly 10000 weeks" in { - TimeSpan.format(weeks(10000L)) must_== "10000 weeks" + TimeSpan.format(weeks(10000L)) must beEqualTo("10000 weeks") } }