This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 0b37efa3852681f7268b343d2c1d39aa883f4066 Author: opencode <[email protected]> AuthorDate: Sun Oct 11 22:13:23 2026 +0200 Reject bucket durations that overflow the shift based time bucket counter TimeBucketCounter computes its bucket size as duration * 1000 in int arithmetic and rounds it up to the next power of 2. For durations in the range above 1073741 seconds the product exceeds 2^30, nextPowerOf2 returns Integer.MIN_VALUE, the bit-count loop yields zero bits and the base class constructor rejected the resulting zero duration with the misleading message "The duration (in seconds) must be a positive integer, was [0]". Worse, for durations of 2147484 seconds and above the millisecond product wraps to a small positive value: the counter was silently constructed and enforced a one or two second bucket instead of the configured multi-week window, rejecting legitimate traffic. Reject durations larger than 1073741 seconds, the largest value for which the actual duration in milliseconds, rounded up to the next power of 2, still fits in an int, with a dedicated error message. The check is placed in determineShiftBitsOfDuration() so that it runs before the super() constructor argument and the ratio computation. Configurations that need longer windows have always been able to select ExactRateLimiter, whose counter divides exactly and is not limited by the shift arithmetic. Add unit tests covering the maximum accepted duration, a duration just above the maximum and one that previously passed silently due to wrap-around. --- .../apache/catalina/util/LocalStrings.properties | 1 + .../apache/catalina/util/TimeBucketCounter.java | 22 +++++++++++++++++ .../catalina/util/TestTimeBucketCounter.java | 28 ++++++++++++++++++++++ 3 files changed, 51 insertions(+) diff --git a/java/org/apache/catalina/util/LocalStrings.properties b/java/org/apache/catalina/util/LocalStrings.properties index 63eaed8748..7f59be4eb6 100644 --- a/java/org/apache/catalina/util/LocalStrings.properties +++ b/java/org/apache/catalina/util/LocalStrings.properties @@ -52,5 +52,6 @@ sessionIdGeneratorBase.random=Exception initializing random number generator of sessionIdGeneratorBase.randomAlgorithm=Exception initializing random number generator using algorithm [{0}] sessionIdGeneratorBase.randomProvider=Exception initializing random number generator using provider [{0}] +timebucket.durationTooLarge=The duration (in seconds) must not exceed [{1}], was [{0}] timebucket.invalidDuration=The duration (in seconds) must be a positive integer, was [{0}] timebucket.maintenance.error=Error processing periodic maintenance diff --git a/java/org/apache/catalina/util/TimeBucketCounter.java b/java/org/apache/catalina/util/TimeBucketCounter.java index 4f54ade92f..4282840d83 100644 --- a/java/org/apache/catalina/util/TimeBucketCounter.java +++ b/java/org/apache/catalina/util/TimeBucketCounter.java @@ -18,11 +18,20 @@ package org.apache.catalina.util; import java.util.concurrent.ScheduledExecutorService; +import org.apache.tomcat.util.res.StringManager; + /** * A fast counter that optimizes efficiency at the expense of approximate bucket indexing. */ public class TimeBucketCounter extends TimeBucketCounterBase { + private static final StringManager sm = StringManager.getManager(TimeBucketCounter.class); + + // The largest configured duration, in seconds, for which the actual duration in milliseconds rounded up to the + // next power of 2 still fits in an int, as required by the int based bit shift arithmetic of this + // implementation. Longer durations should use ExactTimeBucketCounter, which divides exactly. + private static final int MAX_DURATION_SECONDS = (int) ((1L << 30) / 1000); + // Milliseconds bucket size as a Power of 2 for bit shift math, e.g. 16 for 65_536ms which is about 1:05 minute. private final int numBits; @@ -35,6 +44,10 @@ public class TimeBucketCounter extends TimeBucketCounterBase { * * @param bucketDuration the bucket duration in seconds * @param executorService the executor service for maintenance tasks + * + * @throws IllegalArgumentException if bucketDuration is not positive, or is larger than the maximum supported + * duration of the shift based arithmetic (see + * {@link #determineShiftBitsOfDuration(int)}) */ public TimeBucketCounter(int bucketDuration, ScheduledExecutorService executorService) { super(getActualDuration(bucketDuration), executorService); @@ -83,8 +96,17 @@ public class TimeBucketCounter extends TimeBucketCounterBase { * @param duration bucket duration in seconds * * @return bits to be shifted + * + * @throws IllegalArgumentException if the duration in milliseconds, rounded up to the next power of 2, would not + * fit in an int */ protected static int determineShiftBitsOfDuration(int duration) { + if (duration > MAX_DURATION_SECONDS) { + // duration * 1000, rounded up to the next power of 2, would overflow int. Reject it with a clear + // message rather than computing a meaningless number of bits. + throw new IllegalArgumentException(sm.getString("timebucket.durationTooLarge", Integer.valueOf(duration), + Integer.valueOf(MAX_DURATION_SECONDS))); + } int bits = 0; int pof2 = nextPowerOf2(duration * 1000); int bitCheck = pof2; diff --git a/test/org/apache/catalina/util/TestTimeBucketCounter.java b/test/org/apache/catalina/util/TestTimeBucketCounter.java index 43f505eb43..4a2d66c996 100644 --- a/test/org/apache/catalina/util/TestTimeBucketCounter.java +++ b/test/org/apache/catalina/util/TestTimeBucketCounter.java @@ -49,6 +49,34 @@ public class TestTimeBucketCounter { Assert.assertEquals(1.092, tbc.getRatio(), DELTA); } + @Test + public void testDurationAtMaximum() { + // 1073741 seconds is the largest duration whose millisecond form, rounded up to the next power of 2 + // (2^30), still fits in an int + TimeBucketCounter tbc = new TimeBucketCounter(1073741, + new java.util.concurrent.ScheduledThreadPoolExecutor(1)); + Assert.assertEquals(30, tbc.getNumBits()); + } + + @Test + public void testDurationTooLarge() { + // Previously this failed with the misleading "must be a positive integer, was [0]" message because the + // overflowed duration produced zero shift bits + IllegalArgumentException iae = Assert.assertThrows(IllegalArgumentException.class, () -> new TimeBucketCounter( + 1073742, new java.util.concurrent.ScheduledThreadPoolExecutor(1))); + // MessageFormat applies locale grouping to the numbers, e.g. "1,073,742" in en_US + Assert.assertTrue("Error message should mention the rejected duration: " + iae.getMessage(), + iae.getMessage().replace(",", "").replace(".", "").contains("1073742")); + } + + @Test + public void testDurationTooLargeAfterWrapAround() { + // Previously this was silently accepted: 4294968 * 1000 wraps to 704 in int arithmetic, producing a + // 1 second bucket with a ratio of 1.455 instead of the configured ~49.7 day window + Assert.assertThrows(IllegalArgumentException.class, () -> new TimeBucketCounter(4294968, + new java.util.concurrent.ScheduledThreadPoolExecutor(1))); + } + @Test public void testGetMillisUntilNextBucket() throws InterruptedException { long millis; --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
