From ab82116ed315f546dbe5e44610fbe3d4308a7b00 Mon Sep 17 00:00:00 2001 From: Kevin Walls Date: Mon, 7 Apr 2025 10:04:36 +0100 Subject: [PATCH 1/4] 8353727: HeapDumpPath doesn't expand %p --- src/hotspot/share/services/heapDumper.cpp | 71 ++++++++----------- .../TestHeapDumpOnOutOfMemoryError.java | 26 +++++-- 2 files changed, 50 insertions(+), 47 deletions(-) diff --git a/src/hotspot/share/services/heapDumper.cpp b/src/hotspot/share/services/heapDumper.cpp index b30b042864861..7c66c3b682a3f 100644 --- a/src/hotspot/share/services/heapDumper.cpp +++ b/src/hotspot/share/services/heapDumper.cpp @@ -43,6 +43,7 @@ #include "oops/objArrayOop.inline.hpp" #include "oops/oop.inline.hpp" #include "oops/typeArrayOop.inline.hpp" +#include "runtime/arguments.hpp" #include "runtime/continuationWrapper.inline.hpp" #include "runtime/frame.inline.hpp" #include "runtime/handles.inline.hpp" @@ -2747,11 +2748,10 @@ void HeapDumper::dump_heap() { void HeapDumper::dump_heap(bool oome) { static char base_path[JVM_MAXPATHLEN] = {'\0'}; static uint dump_file_seq = 0; - char* my_path; + char my_path[JVM_MAXPATHLEN]; const int max_digit_chars = 20; - const char* dump_file_name = "java_pid"; - const char* dump_file_ext = HeapDumpGzipLevel > 0 ? ".hprof.gz" : ".hprof"; + const char* dump_file_name = HeapDumpGzipLevel > 0 ? "java_pid%p.hprof.gz" : "java_pid%p.hprof"; // The dump file defaults to java_pid.hprof in the current working // directory. HeapDumpPath= can be used to specify an alternative @@ -2762,56 +2762,42 @@ void HeapDumper::dump_heap(bool oome) { const size_t total_length = (HeapDumpPath == nullptr ? 0 : strlen(HeapDumpPath)) + strlen(os::file_separator()) + max_digit_chars + - strlen(dump_file_name) + strlen(dump_file_ext) + 1; + strlen(dump_file_name) + 1; if (total_length > sizeof(base_path)) { warning("Cannot create heap dump file. HeapDumpPath is too long."); return; } - bool use_default_filename = true; - if (HeapDumpPath == nullptr || HeapDumpPath[0] == '\0') { - // HeapDumpPath= not specified - } else { - strcpy(base_path, HeapDumpPath); - // check if the path is a directory (must exist) - DIR* dir = os::opendir(base_path); - if (dir == nullptr) { - use_default_filename = false; - } else { - // HeapDumpPath specified a directory. We append a file separator - // (if needed). - os::closedir(dir); - size_t fs_len = strlen(os::file_separator()); - if (strlen(base_path) >= fs_len) { - char* end = base_path; - end += (strlen(base_path) - fs_len); - if (strcmp(end, os::file_separator()) != 0) { - strcat(base_path, os::file_separator()); - } + // Set base path (name or directory, default or custom, without seq no), doing %p substitution. + const char *path_src = (HeapDumpPath && HeapDumpPath[0] != '\0') ? HeapDumpPath : dump_file_name; + if (!Arguments::copy_expand_pid(path_src, strlen(path_src), base_path, JVM_MAXPATHLEN)) { + warning("Cannot create heap dump file. HeapDumpPath is too long."); + return; + } + // Check if the path is an existing directory + DIR* dir = os::opendir(base_path); + if (dir != nullptr) { + os::closedir(dir); + // Append a file separator (if needed). + size_t fs_len = strlen(os::file_separator()); + if (strlen(base_path) >= fs_len) { + char* end = base_path; + end += (strlen(base_path) - fs_len); + if (strcmp(end, os::file_separator()) != 0) { + strcat(base_path, os::file_separator()); } } + // Path is a directory. Append the default name, with %p substitution. Use my_path temporarily. + if (!Arguments::copy_expand_pid(dump_file_name, strlen(dump_file_name), my_path, JVM_MAXPATHLEN)) { + warning("Cannot create heap dump file. HeapDumpPath is too long."); + } + const size_t dlen = strlen(base_path); + jio_snprintf(&base_path[dlen], sizeof(base_path) - dlen, "%s", my_path); } - // If HeapDumpPath wasn't a file name then we append the default name - if (use_default_filename) { - const size_t dlen = strlen(base_path); // if heap dump dir specified - jio_snprintf(&base_path[dlen], sizeof(base_path)-dlen, "%s%d%s", - dump_file_name, os::current_process_id(), dump_file_ext); - } - const size_t len = strlen(base_path) + 1; - my_path = (char*)os::malloc(len, mtInternal); - if (my_path == nullptr) { - warning("Cannot create heap dump file. Out of system memory."); - return; - } - strncpy(my_path, base_path, len); + strncpy(my_path, base_path, JVM_MAXPATHLEN); } else { // Append a sequence number id for dumps following the first const size_t len = strlen(base_path) + max_digit_chars + 2; // for '.' and \0 - my_path = (char*)os::malloc(len, mtInternal); - if (my_path == nullptr) { - warning("Cannot create heap dump file. Out of system memory."); - return; - } jio_snprintf(my_path, len, "%s.%d", base_path, dump_file_seq); } dump_file_seq++; // increment seq number for next time we dump @@ -2819,5 +2805,4 @@ void HeapDumper::dump_heap(bool oome) { HeapDumper dumper(false /* no GC before heap dump */, oome /* pass along out-of-memory-error flag */); dumper.dump(my_path, tty, HeapDumpGzipLevel); - os::free(my_path); } diff --git a/test/hotspot/jtreg/runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryError.java b/test/hotspot/jtreg/runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryError.java index fb098bd27fec7..2eb46d7895aa5 100644 --- a/test/hotspot/jtreg/runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryError.java +++ b/test/hotspot/jtreg/runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryError.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018, 2023, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2018, 2025, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -71,6 +71,7 @@ public static void main(String[] args) throws Exception { } } test(args[1]); + System.out.println("PASSED"); } static void test(String type) throws Exception { @@ -96,11 +97,28 @@ static void test(String type) throws Exception { output.stdoutShouldNotBeEmpty(); output.shouldContain("Dumping heap to " + type + ".hprof"); File dump = new File(heapdumpFilename); - Asserts.assertTrue(dump.exists() && dump.isFile(), - "Could not find dump file " + dump.getAbsolutePath()); + Asserts.assertTrue(dump.exists() && dump.isFile(), "Could not find dump file " + dump.getAbsolutePath()); HprofParser.parse(new File(heapdumpFilename)); - System.out.println("PASSED"); + + // Test again using %p pid substitution in HeapDumpPath: + heapdumpFilename = type + ".%p.hprof"; + pb = ProcessTools.createLimitedTestJavaProcessBuilder("-XX:+HeapDumpOnOutOfMemoryError", + "-XX:HeapDumpPath=" + heapdumpFilename, + "-XX:MaxMetaspaceSize=16m", + "-Xmx128m", + Platform.isDebugBuild() ? "-XX:-VerifyDependencies" : "-Dx", + TestHeapDumpOnOutOfMemoryError.class.getName(), type); + + Process proc = pb.start(); + output = new OutputAnalyzer(proc); + output.stdoutShouldNotBeEmpty(); + long pid = proc.pid(); + String actualHeapdumpFilename = type + "." + pid + ".hprof"; + output.shouldContain("Dumping heap to " + actualHeapdumpFilename); + dump = new File(actualHeapdumpFilename); + Asserts.assertTrue(dump.exists() && dump.isFile(), "Could not find dump file " + dump.getAbsolutePath()); + HprofParser.parse(new File(actualHeapdumpFilename)); } } From 4ec2b03c99fcabf7cd23cf6acd6d01edfa23067a Mon Sep 17 00:00:00 2001 From: Kevin Walls Date: Tue, 8 Apr 2025 12:04:52 +0100 Subject: [PATCH 2/4] Chris feedback --- src/hotspot/share/services/heapDumper.cpp | 3 ++- .../ErrorHandling/TestHeapDumpOnOutOfMemoryError.java | 7 ++----- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/hotspot/share/services/heapDumper.cpp b/src/hotspot/share/services/heapDumper.cpp index 7c66c3b682a3f..9d4d791866b7d 100644 --- a/src/hotspot/share/services/heapDumper.cpp +++ b/src/hotspot/share/services/heapDumper.cpp @@ -2769,7 +2769,7 @@ void HeapDumper::dump_heap(bool oome) { } // Set base path (name or directory, default or custom, without seq no), doing %p substitution. - const char *path_src = (HeapDumpPath && HeapDumpPath[0] != '\0') ? HeapDumpPath : dump_file_name; + const char *path_src = (HeapDumpPath != nullptr && HeapDumpPath[0] != '\0') ? HeapDumpPath : dump_file_name; if (!Arguments::copy_expand_pid(path_src, strlen(path_src), base_path, JVM_MAXPATHLEN)) { warning("Cannot create heap dump file. HeapDumpPath is too long."); return; @@ -2790,6 +2790,7 @@ void HeapDumper::dump_heap(bool oome) { // Path is a directory. Append the default name, with %p substitution. Use my_path temporarily. if (!Arguments::copy_expand_pid(dump_file_name, strlen(dump_file_name), my_path, JVM_MAXPATHLEN)) { warning("Cannot create heap dump file. HeapDumpPath is too long."); + return; } const size_t dlen = strlen(base_path); jio_snprintf(&base_path[dlen], sizeof(base_path) - dlen, "%s", my_path); diff --git a/test/hotspot/jtreg/runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryError.java b/test/hotspot/jtreg/runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryError.java index 2eb46d7895aa5..b371c82fb8630 100644 --- a/test/hotspot/jtreg/runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryError.java +++ b/test/hotspot/jtreg/runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryError.java @@ -98,7 +98,6 @@ static void test(String type) throws Exception { output.shouldContain("Dumping heap to " + type + ".hprof"); File dump = new File(heapdumpFilename); Asserts.assertTrue(dump.exists() && dump.isFile(), "Could not find dump file " + dump.getAbsolutePath()); - HprofParser.parse(new File(heapdumpFilename)); // Test again using %p pid substitution in HeapDumpPath: @@ -110,11 +109,9 @@ static void test(String type) throws Exception { Platform.isDebugBuild() ? "-XX:-VerifyDependencies" : "-Dx", TestHeapDumpOnOutOfMemoryError.class.getName(), type); - Process proc = pb.start(); - output = new OutputAnalyzer(proc); + output = new OutputAnalyzer(pb.start()); output.stdoutShouldNotBeEmpty(); - long pid = proc.pid(); - String actualHeapdumpFilename = type + "." + pid + ".hprof"; + String actualHeapdumpFilename = type + "." + output.pid() + ".hprof"; output.shouldContain("Dumping heap to " + actualHeapdumpFilename); dump = new File(actualHeapdumpFilename); Asserts.assertTrue(dump.exists() && dump.isFile(), "Could not find dump file " + dump.getAbsolutePath()); From c32e4ca4e11989864f9e6e48d5c98c45191fcf66 Mon Sep 17 00:00:00 2001 From: Kevin Walls Date: Tue, 8 Apr 2025 12:39:28 +0100 Subject: [PATCH 3/4] length checking update --- src/hotspot/share/services/heapDumper.cpp | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/src/hotspot/share/services/heapDumper.cpp b/src/hotspot/share/services/heapDumper.cpp index 9d4d791866b7d..7a2c8d5296985 100644 --- a/src/hotspot/share/services/heapDumper.cpp +++ b/src/hotspot/share/services/heapDumper.cpp @@ -2750,27 +2750,15 @@ void HeapDumper::dump_heap(bool oome) { static uint dump_file_seq = 0; char my_path[JVM_MAXPATHLEN]; const int max_digit_chars = 20; - const char* dump_file_name = HeapDumpGzipLevel > 0 ? "java_pid%p.hprof.gz" : "java_pid%p.hprof"; // The dump file defaults to java_pid.hprof in the current working // directory. HeapDumpPath= can be used to specify an alternative // dump file name or a directory where dump file is created. if (dump_file_seq == 0) { // first time in, we initialize base_path - // Calculate potentially longest base path and check if we have enough - // allocated statically. - const size_t total_length = - (HeapDumpPath == nullptr ? 0 : strlen(HeapDumpPath)) + - strlen(os::file_separator()) + max_digit_chars + - strlen(dump_file_name) + 1; - if (total_length > sizeof(base_path)) { - warning("Cannot create heap dump file. HeapDumpPath is too long."); - return; - } - // Set base path (name or directory, default or custom, without seq no), doing %p substitution. const char *path_src = (HeapDumpPath != nullptr && HeapDumpPath[0] != '\0') ? HeapDumpPath : dump_file_name; - if (!Arguments::copy_expand_pid(path_src, strlen(path_src), base_path, JVM_MAXPATHLEN)) { + if (!Arguments::copy_expand_pid(path_src, strlen(path_src), base_path, JVM_MAXPATHLEN - max_digit_chars)) { warning("Cannot create heap dump file. HeapDumpPath is too long."); return; } @@ -2778,7 +2766,7 @@ void HeapDumper::dump_heap(bool oome) { DIR* dir = os::opendir(base_path); if (dir != nullptr) { os::closedir(dir); - // Append a file separator (if needed). + // Path is a directory. Append a file separator (if needed). size_t fs_len = strlen(os::file_separator()); if (strlen(base_path) >= fs_len) { char* end = base_path; @@ -2787,8 +2775,8 @@ void HeapDumper::dump_heap(bool oome) { strcat(base_path, os::file_separator()); } } - // Path is a directory. Append the default name, with %p substitution. Use my_path temporarily. - if (!Arguments::copy_expand_pid(dump_file_name, strlen(dump_file_name), my_path, JVM_MAXPATHLEN)) { + // Then add the default name, with %p substitution. Use my_path temporarily. + if (!Arguments::copy_expand_pid(dump_file_name, strlen(dump_file_name), my_path, JVM_MAXPATHLEN - max_digit_chars)) { warning("Cannot create heap dump file. HeapDumpPath is too long."); return; } From 40c67a0c6aed8cb98dd805037db04c6a6ef94fed Mon Sep 17 00:00:00 2001 From: Kevin Walls Date: Wed, 9 Apr 2025 10:30:03 +0100 Subject: [PATCH 4/4] test feedback --- .../TestHeapDumpOnOutOfMemoryError.java | 30 +++++-------------- 1 file changed, 7 insertions(+), 23 deletions(-) diff --git a/test/hotspot/jtreg/runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryError.java b/test/hotspot/jtreg/runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryError.java index b371c82fb8630..66df532983a46 100644 --- a/test/hotspot/jtreg/runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryError.java +++ b/test/hotspot/jtreg/runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryError.java @@ -75,7 +75,8 @@ public static void main(String[] args) throws Exception { } static void test(String type) throws Exception { - String heapdumpFilename = type + ".hprof"; + // Test using %p pid substitution in HeapDumpPath: + String heapdumpFilename = type + ".%p.hprof"; ProcessBuilder pb = ProcessTools.createLimitedTestJavaProcessBuilder("-XX:+HeapDumpOnOutOfMemoryError", "-XX:HeapDumpPath=" + heapdumpFilename, // Note: When trying to provoke a metaspace OOM we may generate a lot of classes. In debug VMs this @@ -95,27 +96,10 @@ static void test(String type) throws Exception { OutputAnalyzer output = new OutputAnalyzer(pb.start()); output.stdoutShouldNotBeEmpty(); - output.shouldContain("Dumping heap to " + type + ".hprof"); - File dump = new File(heapdumpFilename); - Asserts.assertTrue(dump.exists() && dump.isFile(), "Could not find dump file " + dump.getAbsolutePath()); - HprofParser.parse(new File(heapdumpFilename)); - - // Test again using %p pid substitution in HeapDumpPath: - heapdumpFilename = type + ".%p.hprof"; - pb = ProcessTools.createLimitedTestJavaProcessBuilder("-XX:+HeapDumpOnOutOfMemoryError", - "-XX:HeapDumpPath=" + heapdumpFilename, - "-XX:MaxMetaspaceSize=16m", - "-Xmx128m", - Platform.isDebugBuild() ? "-XX:-VerifyDependencies" : "-Dx", - TestHeapDumpOnOutOfMemoryError.class.getName(), type); - - output = new OutputAnalyzer(pb.start()); - output.stdoutShouldNotBeEmpty(); - String actualHeapdumpFilename = type + "." + output.pid() + ".hprof"; - output.shouldContain("Dumping heap to " + actualHeapdumpFilename); - dump = new File(actualHeapdumpFilename); - Asserts.assertTrue(dump.exists() && dump.isFile(), "Could not find dump file " + dump.getAbsolutePath()); - HprofParser.parse(new File(actualHeapdumpFilename)); + String expectedHeapdumpFilename = type + "." + output.pid() + ".hprof"; + output.shouldContain("Dumping heap to " + expectedHeapdumpFilename); + File dump = new File(expectedHeapdumpFilename); + Asserts.assertTrue(dump.exists() && dump.isFile(), "Expected heap dump file " + dump.getAbsolutePath()); + HprofParser.parse(new File(expectedHeapdumpFilename)); } - }