From b60ca787b0c75cda2c4c8faf645849c6f6d974b4 Mon Sep 17 00:00:00 2001 From: andsel Date: Thu, 20 Mar 2025 11:06:42 +0100 Subject: [PATCH 1/6] Moved creation of RubyString inside generateString, this avoid to keep alive a reference to a 2GB Java String --- logstash-core/build.gradle | 3 ++- .../BufferedTokenizerExtWithSizeLimitTest.java | 14 +++++++------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/logstash-core/build.gradle b/logstash-core/build.gradle index 2b3bb6e668..01c8fbc22a 100644 --- a/logstash-core/build.gradle +++ b/logstash-core/build.gradle @@ -124,7 +124,8 @@ tasks.register("javaTests", Test) { exclude '/org/logstash/plugins/factory/PluginFactoryExtTest.class' exclude '/org/logstash/execution/ObservedExecutionTest.class' - maxHeapSize = "12g" + maxHeapSize = "10g" + jvmArgs '-XX:+HeapDumpOnOutOfMemoryError' jacoco { enabled = true diff --git a/logstash-core/src/test/java/org/logstash/common/BufferedTokenizerExtWithSizeLimitTest.java b/logstash-core/src/test/java/org/logstash/common/BufferedTokenizerExtWithSizeLimitTest.java index 16181bb6f1..d38c182754 100644 --- a/logstash-core/src/test/java/org/logstash/common/BufferedTokenizerExtWithSizeLimitTest.java +++ b/logstash-core/src/test/java/org/logstash/common/BufferedTokenizerExtWithSizeLimitTest.java @@ -117,25 +117,25 @@ public void giveMultipleSegmentsThatGeneratesMultipleBufferFullErrorsThenIsAbleT @Test public void givenMaliciousInputExtractDoesntOverflow() { assertEquals("Xmx must equals to what's defined in the Gradle's javaTests task", - 12L * GB, Runtime.getRuntime().maxMemory()); + 10L * GB, Runtime.getRuntime().maxMemory()); // re-init the tokenizer with big sizeLimit initSUTWithSizeLimit((int) (2L * GB) - 3); // Integer.MAX_VALUE is 2 * GB - String bigFirstPiece = generateString("a", Integer.MAX_VALUE - 1024); - sut.extract(context, RubyUtil.RUBY.newString(bigFirstPiece)); + RubyString bigFirstPiece = generateString("a", Integer.MAX_VALUE - 1024); + sut.extract(context, bigFirstPiece); // add another small fragment to trigger int overflow // sizeLimit is (2^32-1)-3 first segment length is (2^32-1) - 1024 second is 1024 +2 - // so the combined length of first and second is > sizeLimit and should throw an expection + // so the combined length of first and second is > sizeLimit and should throw an exception // but because of overflow it's negative and happens to be < sizeLimit Exception thrownException = assertThrows(IllegalStateException.class, () -> { - sut.extract(context, RubyUtil.RUBY.newString(generateString("a", 1024 + 2))); + sut.extract(context, generateString("a", 1024 + 2)); }); assertThat(thrownException.getMessage(), containsString("input buffer full")); } - private String generateString(String fill, int size) { - return fill.repeat(size); + private RubyString generateString(String fill, int size) { + return RubyUtil.RUBY.newString(fill.repeat(size)); } } \ No newline at end of file From 6b698725d1e10f2de5bddc0520f0d4bf9528e00d Mon Sep 17 00:00:00 2001 From: andsel Date: Thu, 20 Mar 2025 11:21:36 +0100 Subject: [PATCH 2/6] Ignore test if not enough physical memory is available --- ...BufferedTokenizerExtWithSizeLimitTest.java | 38 ++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/logstash-core/src/test/java/org/logstash/common/BufferedTokenizerExtWithSizeLimitTest.java b/logstash-core/src/test/java/org/logstash/common/BufferedTokenizerExtWithSizeLimitTest.java index d38c182754..98a5bb5ac6 100644 --- a/logstash-core/src/test/java/org/logstash/common/BufferedTokenizerExtWithSizeLimitTest.java +++ b/logstash-core/src/test/java/org/logstash/common/BufferedTokenizerExtWithSizeLimitTest.java @@ -28,11 +28,18 @@ import org.logstash.RubyTestBase; import org.logstash.RubyUtil; +import javax.management.Attribute; +import javax.management.InstanceNotFoundException; +import javax.management.ReflectionException; +import java.lang.management.ManagementFactory; +import java.lang.management.OperatingSystemMXBean; import java.util.List; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.junit.Assert.*; +import static org.junit.Assume.assumeThat; +import static org.junit.Assume.assumeTrue; import static org.logstash.RubyUtil.RUBY; @SuppressWarnings("unchecked") @@ -116,8 +123,11 @@ public void giveMultipleSegmentsThatGeneratesMultipleBufferFullErrorsThenIsAbleT @Test public void givenMaliciousInputExtractDoesntOverflow() { + long expectedNeedHeapMemory = 10L * GB; + assumeTrue("Skip the test because VM hasn't enough physical memory", hasEnoughPhysicalMemory(expectedNeedHeapMemory)); + assertEquals("Xmx must equals to what's defined in the Gradle's javaTests task", - 10L * GB, Runtime.getRuntime().maxMemory()); + expectedNeedHeapMemory, Runtime.getRuntime().maxMemory()); // re-init the tokenizer with big sizeLimit initSUTWithSizeLimit((int) (2L * GB) - 3); @@ -138,4 +148,30 @@ public void givenMaliciousInputExtractDoesntOverflow() { private RubyString generateString(String fill, int size) { return RubyUtil.RUBY.newString(fill.repeat(size)); } + + private boolean hasEnoughPhysicalMemory(long requiredPhysicalMemory) { + long physicalMemory; + try { + physicalMemory = readPhysicalMemorySize(); + } catch (InstanceNotFoundException | ReflectionException e) { + System.out.println("Can't read attribute JMX OS bean"); + return false; + } catch (IllegalStateException e) { + System.out.println(e.getMessage()); + return false; + } + return physicalMemory > requiredPhysicalMemory; + } + + private long readPhysicalMemorySize() throws ReflectionException, InstanceNotFoundException { + OperatingSystemMXBean op = ManagementFactory.getOperatingSystemMXBean(); + + List attributes = ManagementFactory.getPlatformMBeanServer() + .getAttributes(op.getObjectName(), new String[]{"TotalPhysicalMemorySize"} ).asList(); + if (attributes.isEmpty()) { + throw new IllegalStateException("Attribute TotalPhysicalMemorySize is not available from JMX OS bean"); + } + Attribute a = attributes.get(0); + return (long) (Long) a.getValue(); + } } \ No newline at end of file From f54c199f069b2c9d14017bbefd3f71e248a80db8 Mon Sep 17 00:00:00 2001 From: andsel Date: Thu, 20 Mar 2025 11:24:54 +0100 Subject: [PATCH 3/6] Cleanup --- logstash-core/build.gradle | 1 - .../common/BufferedTokenizerExtWithSizeLimitTest.java | 6 +++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/logstash-core/build.gradle b/logstash-core/build.gradle index 01c8fbc22a..ff8c7eb10a 100644 --- a/logstash-core/build.gradle +++ b/logstash-core/build.gradle @@ -125,7 +125,6 @@ tasks.register("javaTests", Test) { exclude '/org/logstash/execution/ObservedExecutionTest.class' maxHeapSize = "10g" - jvmArgs '-XX:+HeapDumpOnOutOfMemoryError' jacoco { enabled = true diff --git a/logstash-core/src/test/java/org/logstash/common/BufferedTokenizerExtWithSizeLimitTest.java b/logstash-core/src/test/java/org/logstash/common/BufferedTokenizerExtWithSizeLimitTest.java index 98a5bb5ac6..eeaddd4f48 100644 --- a/logstash-core/src/test/java/org/logstash/common/BufferedTokenizerExtWithSizeLimitTest.java +++ b/logstash-core/src/test/java/org/logstash/common/BufferedTokenizerExtWithSizeLimitTest.java @@ -37,8 +37,8 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; -import static org.junit.Assert.*; -import static org.junit.Assume.assumeThat; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; import static org.junit.Assume.assumeTrue; import static org.logstash.RubyUtil.RUBY; @@ -122,7 +122,7 @@ public void giveMultipleSegmentsThatGeneratesMultipleBufferFullErrorsThenIsAbleT } @Test - public void givenMaliciousInputExtractDoesntOverflow() { + public void givenTooLongInputExtractDoesntOverflow() { long expectedNeedHeapMemory = 10L * GB; assumeTrue("Skip the test because VM hasn't enough physical memory", hasEnoughPhysicalMemory(expectedNeedHeapMemory)); From 930a3971d2cc6f16e477dc6dd6d4221964073936 Mon Sep 17 00:00:00 2001 From: andsel Date: Thu, 20 Mar 2025 17:01:18 +0100 Subject: [PATCH 4/6] Println physicalVM memory --- .../logstash/common/BufferedTokenizerExtWithSizeLimitTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/logstash-core/src/test/java/org/logstash/common/BufferedTokenizerExtWithSizeLimitTest.java b/logstash-core/src/test/java/org/logstash/common/BufferedTokenizerExtWithSizeLimitTest.java index eeaddd4f48..85e4f128ed 100644 --- a/logstash-core/src/test/java/org/logstash/common/BufferedTokenizerExtWithSizeLimitTest.java +++ b/logstash-core/src/test/java/org/logstash/common/BufferedTokenizerExtWithSizeLimitTest.java @@ -160,6 +160,7 @@ private boolean hasEnoughPhysicalMemory(long requiredPhysicalMemory) { System.out.println(e.getMessage()); return false; } + System.out.println("Physical memory on the VM is: " + physicalMemory + " bytes"); return physicalMemory > requiredPhysicalMemory; } From cff2dba6af92d0235b8c3325a8bb0878a36f7ec6 Mon Sep 17 00:00:00 2001 From: andsel Date: Mon, 24 Mar 2025 14:57:20 +0100 Subject: [PATCH 5/6] Ignore the test if JDK is not at least 21 that has proven to be able to execute it --- .../src/main/java/org/logstash/util/JavaVersion.java | 1 + .../BufferedTokenizerExtWithSizeLimitTest.java | 12 ++++++++++++ 2 files changed, 13 insertions(+) diff --git a/logstash-core/src/main/java/org/logstash/util/JavaVersion.java b/logstash-core/src/main/java/org/logstash/util/JavaVersion.java index c1d619e985..7bf1ac85b5 100644 --- a/logstash-core/src/main/java/org/logstash/util/JavaVersion.java +++ b/logstash-core/src/main/java/org/logstash/util/JavaVersion.java @@ -31,6 +31,7 @@ public class JavaVersion implements Comparable { public static final JavaVersion CURRENT = parse(System.getProperty("java.specification.version")); public static final JavaVersion JAVA_17 = parse("17"); + public static final JavaVersion JAVA_21 = parse("21"); private final List version; diff --git a/logstash-core/src/test/java/org/logstash/common/BufferedTokenizerExtWithSizeLimitTest.java b/logstash-core/src/test/java/org/logstash/common/BufferedTokenizerExtWithSizeLimitTest.java index 85e4f128ed..9af76ca649 100644 --- a/logstash-core/src/test/java/org/logstash/common/BufferedTokenizerExtWithSizeLimitTest.java +++ b/logstash-core/src/test/java/org/logstash/common/BufferedTokenizerExtWithSizeLimitTest.java @@ -19,6 +19,7 @@ package org.logstash.common; +import org.hamcrest.Matchers; import org.jruby.RubyArray; import org.jruby.RubyString; import org.jruby.runtime.ThreadContext; @@ -27,6 +28,7 @@ import org.junit.Test; import org.logstash.RubyTestBase; import org.logstash.RubyUtil; +import org.logstash.util.JavaVersion; import javax.management.Attribute; import javax.management.InstanceNotFoundException; @@ -39,6 +41,7 @@ import static org.hamcrest.Matchers.containsString; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertThrows; +import static org.junit.Assume.assumeThat; import static org.junit.Assume.assumeTrue; import static org.logstash.RubyUtil.RUBY; @@ -123,6 +126,15 @@ public void giveMultipleSegmentsThatGeneratesMultipleBufferFullErrorsThenIsAbleT @Test public void givenTooLongInputExtractDoesntOverflow() { + // This test has proven to go OOM on JDK 11 and JDK 17, also if physical memory is 16GB. + // JDK 21 successfully executes the test due to internal changes or efficiency of G1GC (the default GC). + // Tested also others GC on JDK 11 without any success: + // - ZGC + // - Parallel GC + // - CMS + // remove this code when the minimal JDK version for Logstash is JDK 21 or greater. + + assumeThat("Expect at least JDK 21", JavaVersion.CURRENT, Matchers.greaterThanOrEqualTo(JavaVersion.JAVA_21)); long expectedNeedHeapMemory = 10L * GB; assumeTrue("Skip the test because VM hasn't enough physical memory", hasEnoughPhysicalMemory(expectedNeedHeapMemory)); From 4b3f827966cf28e0e033bcd704e2e65bbe771980 Mon Sep 17 00:00:00 2001 From: andsel Date: Tue, 25 Mar 2025 08:51:56 +0100 Subject: [PATCH 6/6] Added link to the motivation why 10GB is needed to run the int overflow test --- logstash-core/build.gradle | 1 + .../logstash/common/BufferedTokenizerExtWithSizeLimitTest.java | 1 + 2 files changed, 2 insertions(+) diff --git a/logstash-core/build.gradle b/logstash-core/build.gradle index ff8c7eb10a..4ae67b0c2d 100644 --- a/logstash-core/build.gradle +++ b/logstash-core/build.gradle @@ -124,6 +124,7 @@ tasks.register("javaTests", Test) { exclude '/org/logstash/plugins/factory/PluginFactoryExtTest.class' exclude '/org/logstash/execution/ObservedExecutionTest.class' + // 10GB is needed by the BufferedTokenizerExtWithSizeLimitTest.givenTooLongInputExtractDoesntOverflow test maxHeapSize = "10g" jacoco { diff --git a/logstash-core/src/test/java/org/logstash/common/BufferedTokenizerExtWithSizeLimitTest.java b/logstash-core/src/test/java/org/logstash/common/BufferedTokenizerExtWithSizeLimitTest.java index 9af76ca649..c25ee01b41 100644 --- a/logstash-core/src/test/java/org/logstash/common/BufferedTokenizerExtWithSizeLimitTest.java +++ b/logstash-core/src/test/java/org/logstash/common/BufferedTokenizerExtWithSizeLimitTest.java @@ -136,6 +136,7 @@ public void givenTooLongInputExtractDoesntOverflow() { assumeThat("Expect at least JDK 21", JavaVersion.CURRENT, Matchers.greaterThanOrEqualTo(JavaVersion.JAVA_21)); long expectedNeedHeapMemory = 10L * GB; + // To understand the motivation of 10GB heap please read https://github.com/elastic/logstash/pull/17373#issuecomment-2750378212 assumeTrue("Skip the test because VM hasn't enough physical memory", hasEnoughPhysicalMemory(expectedNeedHeapMemory)); assertEquals("Xmx must equals to what's defined in the Gradle's javaTests task",