From 0eeb67a8baf0b0c5a851064f7d76bb98a4bed781 Mon Sep 17 00:00:00 2001 From: Albert Puig Date: Tue, 30 Dec 2025 19:16:04 +0100 Subject: [PATCH 1/6] Fix SlaveCommandStatistics memory leak with some commands - Update IRRELEVANT pattern to strip bracket parameters - Add MAX_ENTRIES_PER_AGENT=1000 cap with preferringOlder strategy - Add MAX_COMMAND_LENGTH=256 truncation (tunable via system properties) --- .../support/impl/SlaveCommandStatistics.java | 47 ++++++- .../SlaveCommandStatisticsClassifyTest.java | 121 ++++++++++++++++++ 2 files changed, 162 insertions(+), 6 deletions(-) create mode 100644 src/test/java/com/cloudbees/jenkins/support/impl/SlaveCommandStatisticsClassifyTest.java diff --git a/src/main/java/com/cloudbees/jenkins/support/impl/SlaveCommandStatistics.java b/src/main/java/com/cloudbees/jenkins/support/impl/SlaveCommandStatistics.java index 4ef764908..0da03d36c 100644 --- a/src/main/java/com/cloudbees/jenkins/support/impl/SlaveCommandStatistics.java +++ b/src/main/java/com/cloudbees/jenkins/support/impl/SlaveCommandStatistics.java @@ -52,6 +52,8 @@ import java.util.Map; import java.util.Set; import java.util.TreeMap; +import java.util.logging.Level; +import java.util.logging.Logger; import java.util.regex.Pattern; import javax.annotation.Nonnegative; import jenkins.model.Jenkins; @@ -65,6 +67,12 @@ public final class SlaveCommandStatistics extends Component { /*protected*/ static @Nonnegative int MAX_STATS_SIZE = 1000; + /*protected*/ static @Nonnegative int MAX_COMMAND_LENGTH = + Integer.getInteger(SlaveCommandStatistics.class.getName() + ".maxCommandLength", 256); + + /*protected*/ static @Nonnegative int MAX_ENTRIES_PER_AGENT = + Integer.getInteger(SlaveCommandStatistics.class.getName() + ".maxEntriesPerAgent", 1000); + private final Object statLock = new Object(); @GuardedBy("statLock") @@ -116,7 +124,10 @@ public ComponentCategory getCategory() { } } - private static final class Statistics extends Channel.Listener { + @VisibleForTesting + static final class Statistics extends Channel.Listener { + + private static final Logger LOGGER = Logger.getLogger(Statistics.class.getName()); /** Represents a tally of both the number of times some event occurred, and some integral metric associated with each event which should be summed. */ private static final class CountSum { @@ -143,13 +154,27 @@ long sum() { private final Set jars = new LinkedHashSet<>(); + private static void preferringOlder(Map map, String key, long value) { + CountSum cs = map.get(key); + if (cs != null) { + cs.tally(value); + } else if (map.size() < MAX_ENTRIES_PER_AGENT) { + cs = new CountSum(); + cs.tally(value); + map.put(key, cs); + } else { + LOGGER.log(Level.FINE, () -> "Statistics map at capacity (%d), ignoring command type: %s" + .formatted(MAX_ENTRIES_PER_AGENT, key)); + } + } + @Override public void onWrite(Channel channel, Command cmd, long blockSize) { String type = classify(cmd); // Synchronization probably unnecessary for tallying (since each channel processes commands sequentially), // but printing could happen at any time anyway. synchronized (writes) { - writes.computeIfAbsent(type, k -> new CountSum()).tally(blockSize); + preferringOlder(writes, type, blockSize); } } @@ -157,7 +182,7 @@ public void onWrite(Channel channel, Command cmd, long blockSize) { public void onRead(Channel channel, Command cmd, long blockSize) { String type = classify(cmd); synchronized (reads) { - reads.computeIfAbsent(type, k -> new CountSum()).tally(blockSize); + preferringOlder(reads, type, blockSize); } } @@ -165,7 +190,7 @@ public void onRead(Channel channel, Command cmd, long blockSize) { public void onResponse(Channel channel, Request req, Response rsp, long totalTime) { String type = classify(req); synchronized (responses) { - responses.computeIfAbsent(type, k -> new CountSum()).tally(totalTime); + preferringOlder(responses, type, totalTime); } } @@ -176,10 +201,20 @@ public void onJar(Channel channel, File jar) { } } - private static final Pattern IRRELEVANT = Pattern.compile("(@[a-f0-9]+|[(][^)]+[)])+$"); + /** Strips brackets (content limited to 1MB), hash codes, and parentheses at the end of command strings. */ + private static final Pattern IRRELEVANT = Pattern.compile("(\\[.{0,1048576}\\]|@[a-f0-9]+|[(][^)]+[)])+$"); private static String classify(Command cmd) { - return IRRELEVANT.matcher(cmd.toString()).replaceFirst(""); + return classify(cmd.toString()); + } + + @VisibleForTesting + static String classify(String cmdString) { + cmdString = IRRELEVANT.matcher(cmdString).replaceFirst(""); + if (cmdString.length() > MAX_COMMAND_LENGTH) { + cmdString = cmdString.substring(0, MAX_COMMAND_LENGTH) + "..."; + } + return cmdString; } private void print(PrintWriter out) { diff --git a/src/test/java/com/cloudbees/jenkins/support/impl/SlaveCommandStatisticsClassifyTest.java b/src/test/java/com/cloudbees/jenkins/support/impl/SlaveCommandStatisticsClassifyTest.java new file mode 100644 index 000000000..9a04b4c4e --- /dev/null +++ b/src/test/java/com/cloudbees/jenkins/support/impl/SlaveCommandStatisticsClassifyTest.java @@ -0,0 +1,121 @@ +/* + * The MIT License + * + * Copyright 2025 CloudBees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ +package com.cloudbees.jenkins.support.impl; + +import static com.cloudbees.jenkins.support.impl.SlaveCommandStatistics.Statistics.classify; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.*; +import static org.junit.Assert.assertEquals; + +import org.junit.Test; + +public class SlaveCommandStatisticsClassifyTest { + + @Test + public void patternStripsEndBracketsHashAndParentheses() { + assertEquals( + "UserRequest:RemoteLaunchCallable", + classify("UserRequest:RemoteLaunchCallable[cmd=[docker, exec, --env, FOO=bar]]@2b194a2b")); + + assertEquals("UserRequest:Callable", classify("UserRequest:Callable@1a2b3c4d[params]@5e6f7890(Context)")); + + assertEquals( + "UserRequest:RemoteLaunchCallable", + classify( + "UserRequest:RemoteLaunchCallable[cmd=[nohup, sh, -c, (cp script.sh script.sh.copy; sh -xe script.sh.copy)], env=[Ljava.lang.String;@284e13da]@abc123")); + + assertEquals( + "UserRequest:UserRPCRequest:org.jenkinsci.plugins.gitclient.GitClient.addCredentials", + classify( + "UserRequest:UserRPCRequest:org.jenkinsci.plugins.gitclient.GitClient.addCredentials[java.lang.String,com.cloudbees.plugins.credentials.common.StandardCredentials]@abc")); + + assertEquals( + "UserRequest:hudson.FilePath$CallableWith", + classify("UserRequest:hudson.FilePath$CallableWith[workspace=/home/jenkins/workspace]@7f31245a")); + + assertEquals( + "UserRequest:com.cloudbees.jenkins.support.impl.ThreadDumps$GetThreadDump", + classify("UserRequest:com.cloudbees.jenkins.support.impl.ThreadDumps$GetThreadDump@abc123")); + + assertEquals("UserRequest:hudson.EnvVars$GetEnvVars", classify("UserRequest:hudson.EnvVars$GetEnvVars@def456")); + + assertEquals( + "RPCRequest:hudson.FilePath.act", + classify( + "RPCRequest:hudson.FilePath.act[hudson.FilePath$FileCallable,hudson.remoting.VirtualChannel$ACL](123)")); + + assertEquals("RPCRequest:hudson.FilePath.exists", classify("RPCRequest:hudson.FilePath.exists[](456)")); + + // Legacy format (pre-jenkinsci/jenkins#9921) + assertEquals("Response", classify("Response@456abc(hudson.remoting.Channel)")); + assertEquals("UserRequest:RemoteLaunchCallable", classify("UserRequest:RemoteLaunchCallable@67c6eade8")); + + // Invalid hex not matched + assertEquals("Cmd@zebra", classify("Cmd@zebra")); + + // Brackets in middle preserved ($ anchor) + assertEquals("Prefix[arg]:Suffix", classify("Prefix[arg]:Suffix@abc")); + } + + @Test + public void hugeEnvVarsDeduplicateToSameKey() { + String hugeEnv = "group1,group2,group3,%s".formatted("x".repeat(12000)); + String withHugeEnv = + "UserRequest:RemoteLaunchCallable[cmd=[docker, exec, --env, BUILD_USER_GROUPS=%s]]@67c6eade8" + .formatted(hugeEnv); + + String expected = "UserRequest:RemoteLaunchCallable"; + assertEquals(expected, classify(withHugeEnv)); + assertEquals( + expected, classify("UserRequest:RemoteLaunchCallable[cmd=[docker, --env, BUILD_NUMBER=1]]@1a2b3c4d")); + assertEquals( + expected, classify("UserRequest:RemoteLaunchCallable[cmd=[docker, --env, BUILD_NUMBER=2]]@5e6f7890")); + assertEquals( + expected, + classify("UserRequest:RemoteLaunchCallable[cmd=[docker, --env, TIMESTAMP=%d]]@deadbeef" + .formatted(System.currentTimeMillis()))); + + assertThat(classify(withHugeEnv).length(), lessThan(100)); + assertThat(classify(withHugeEnv), not(containsString("BUILD_USER_GROUPS"))); + } + + @Test + public void defenseInDepthTruncationAndBounds() { + // Truncation: MAX_COMMAND_LENGTH = 256 + String veryLongCommand = "UserRequest:%s@1a2b3c4d".formatted("VeryLongClassName".repeat(20)); + String truncated = classify(veryLongCommand); + assertThat(truncated.length(), lessThanOrEqualTo(259)); + assertThat(truncated, endsWith("...")); + + // Pattern bound: over 1MB gracefully degrades + String overLimit = "Cmd[%s]@abc".formatted("x".repeat(1048577)); + assertThat(classify(overLimit).length(), lessThanOrEqualTo(259)); + assertThat(classify(overLimit), startsWith("Cmd[")); + + // ReDoS safety + assertEquals("Test", classify("Test%s]@abc".formatted("[".repeat(1000)))); + assertEquals("Test", classify("Test%s@abc".formatted("[]".repeat(500)))); + assertThat(classify("Test%s@abc".formatted("[".repeat(1000))).length(), lessThanOrEqualTo(259)); + } +} From d2a288a68eae73b5e3ff353ab1a001da623a1018 Mon Sep 17 00:00:00 2001 From: Albert Puig Date: Tue, 30 Dec 2025 22:28:36 +0100 Subject: [PATCH 2/6] use jenkins.util.SystemProperties --- .../jenkins/support/impl/SlaveCommandStatistics.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/cloudbees/jenkins/support/impl/SlaveCommandStatistics.java b/src/main/java/com/cloudbees/jenkins/support/impl/SlaveCommandStatistics.java index 0da03d36c..6e647806b 100644 --- a/src/main/java/com/cloudbees/jenkins/support/impl/SlaveCommandStatistics.java +++ b/src/main/java/com/cloudbees/jenkins/support/impl/SlaveCommandStatistics.java @@ -58,6 +58,7 @@ import javax.annotation.Nonnegative; import jenkins.model.Jenkins; import jenkins.model.NodeListener; +import jenkins.util.SystemProperties; import net.jcip.annotations.GuardedBy; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; @@ -68,10 +69,10 @@ public final class SlaveCommandStatistics extends Component { /*protected*/ static @Nonnegative int MAX_STATS_SIZE = 1000; /*protected*/ static @Nonnegative int MAX_COMMAND_LENGTH = - Integer.getInteger(SlaveCommandStatistics.class.getName() + ".maxCommandLength", 256); + SystemProperties.getInteger(SlaveCommandStatistics.class.getName() + ".maxCommandLength", 256); /*protected*/ static @Nonnegative int MAX_ENTRIES_PER_AGENT = - Integer.getInteger(SlaveCommandStatistics.class.getName() + ".maxEntriesPerAgent", 1000); + SystemProperties.getInteger(SlaveCommandStatistics.class.getName() + ".maxEntriesPerAgent", 1000); private final Object statLock = new Object(); From 33c15999cd3a1e9ca6650e5e83a6e800e16faec1 Mon Sep 17 00:00:00 2001 From: Albert Puig Date: Tue, 30 Dec 2025 22:35:49 +0100 Subject: [PATCH 3/6] remove Co-authored-by: Jesse Glick --- .../support/impl/SlaveCommandStatisticsClassifyTest.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/test/java/com/cloudbees/jenkins/support/impl/SlaveCommandStatisticsClassifyTest.java b/src/test/java/com/cloudbees/jenkins/support/impl/SlaveCommandStatisticsClassifyTest.java index 9a04b4c4e..9dd72309f 100644 --- a/src/test/java/com/cloudbees/jenkins/support/impl/SlaveCommandStatisticsClassifyTest.java +++ b/src/test/java/com/cloudbees/jenkins/support/impl/SlaveCommandStatisticsClassifyTest.java @@ -96,8 +96,6 @@ public void hugeEnvVarsDeduplicateToSameKey() { classify("UserRequest:RemoteLaunchCallable[cmd=[docker, --env, TIMESTAMP=%d]]@deadbeef" .formatted(System.currentTimeMillis()))); - assertThat(classify(withHugeEnv).length(), lessThan(100)); - assertThat(classify(withHugeEnv), not(containsString("BUILD_USER_GROUPS"))); } @Test From 58a4bc951573afe4bf7a5bd206424ebe487c3c4b Mon Sep 17 00:00:00 2001 From: Albert Puig Date: Wed, 31 Dec 2025 15:17:29 +0100 Subject: [PATCH 4/6] fix spotless --- .../jenkins/support/impl/SlaveCommandStatisticsClassifyTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/java/com/cloudbees/jenkins/support/impl/SlaveCommandStatisticsClassifyTest.java b/src/test/java/com/cloudbees/jenkins/support/impl/SlaveCommandStatisticsClassifyTest.java index 9dd72309f..b72f668ee 100644 --- a/src/test/java/com/cloudbees/jenkins/support/impl/SlaveCommandStatisticsClassifyTest.java +++ b/src/test/java/com/cloudbees/jenkins/support/impl/SlaveCommandStatisticsClassifyTest.java @@ -95,7 +95,6 @@ public void hugeEnvVarsDeduplicateToSameKey() { expected, classify("UserRequest:RemoteLaunchCallable[cmd=[docker, --env, TIMESTAMP=%d]]@deadbeef" .formatted(System.currentTimeMillis()))); - } @Test From 411c2a06ec1cdca766efcbc66176210e5101ce78 Mon Sep 17 00:00:00 2001 From: Albert Puig Date: Wed, 7 Jan 2026 10:24:50 +0100 Subject: [PATCH 5/6] build ci From 9a031e6916368fcfccd7c5bf3672c2307b0a24d0 Mon Sep 17 00:00:00 2001 From: Albert Puig Date: Thu, 8 Jan 2026 17:19:06 +0100 Subject: [PATCH 6/6] build ci