From 7a7b9ed7fe4a10bca155b0877c3e731f9d343b92 Mon Sep 17 00:00:00 2001 From: Kevin Walls Date: Wed, 9 Apr 2025 14:49:04 +0000 Subject: [PATCH] 8353727: HeapDumpPath doesn't expand %p Reviewed-by: stuefe, lmesnik --- src/hotspot/share/services/heapDumper.cpp | 76 ++++++------------- .../TestHeapDumpOnOutOfMemoryError.java | 19 +++-- 2 files changed, 34 insertions(+), 61 deletions(-) diff --git a/src/hotspot/share/services/heapDumper.cpp b/src/hotspot/share/services/heapDumper.cpp index b30b0428648..7a2c8d52969 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,71 +2748,45 @@ 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 // 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) + strlen(dump_file_ext) + 1; - if (total_length > sizeof(base_path)) { + // 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 - max_digit_chars)) { 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()); - } + // Check if the path is an existing directory + DIR* dir = os::opendir(base_path); + if (dir != nullptr) { + os::closedir(dir); + // 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; + end += (strlen(base_path) - fs_len); + if (strcmp(end, os::file_separator()) != 0) { + strcat(base_path, os::file_separator()); } } + // 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; + } + 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 +2794,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 fb098bd27fe..66df532983a 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,10 +71,12 @@ public class TestHeapDumpOnOutOfMemoryError { } } test(args[1]); + System.out.println("PASSED"); } 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 @@ -94,13 +96,10 @@ public class TestHeapDumpOnOutOfMemoryError { 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)); - System.out.println("PASSED"); + 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)); } - }