Skip to content

8353727: HeapDumpPath doesn't expand %p #24482

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kevinjwalls wants to merge 5 commits into from
Closed

8353727: HeapDumpPath doesn't expand %p #24482

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ab82116
8353727: HeapDumpPath doesn't expand %p
kevinjwalls Apr 7, 2025
4ec2b03
Chris feedback
kevinjwalls Apr 8, 2025
c32e4ca
length checking update
kevinjwalls Apr 8, 2025
40c67a0
test feedback
kevinjwalls Apr 9, 2025
09f6654
Merge remote-tracking branch 'upstream/master' into HeapDumpPath_pid
kevinjwalls Apr 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 25 additions & 51 deletions src/hotspot/share/services/heapDumper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -2747,77 +2748,50 @@ 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<pid>.hprof in the current working
// directory. HeapDumpPath=<file> 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=<file> 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)) {
Copy link
Member

@tstuefe tstuefe Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC there is a pre-existing bug, and if I am right one you should fix: this calculation assumes that there is only a single %p. There may be multiple. Many. E.g. as a malicious attempt to cause a buffer overflow.

This is what I meant with stringStream. stringStream offers protection against stuff like that without the manual buffer counting headaches. I would give Arguments a method like this:

print_expand_pid(outputStream* sink, const char* input);

and in there print to sink, with print or putc. This would never truncate. Then use it like this:

outputStream st(caller buffer, caller buffer size)
if (have HeapDumpPath) {
  Arguments::print_expand_pid(st, HeapDumpPath);
  if (st->was_truncated()) return with warning
  // now st->base() ist der expanded heap path. Test if its a directory etc
}
// append file name
  Arguments::print_expand_pid(st, dump_file_name);
  if (st->was_truncated()) return with warning

Just a rough sketch. And fine for followup PRs, though I think it may make your life easier if you do it now.

Copy link
Contributor Author

@kevinjwalls kevinjwalls Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thankfully copy_expand_pid does handle multiple %p replacements. It seems good to use that to check the buffer length, partly for that reason, as just knowing a max number of digits wasn't so flexible if many %p were present.

Thanks for the other ideas!

Copy link
Member

@tstuefe tstuefe Apr 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay, it checks for overflow. Okay, please disregard half of what I have written :)

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

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);
}
19 changes: 9 additions & 10 deletions test/hotspot/jtreg/runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryError.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -71,10 +71,12 @@ public static void main(String[] args) throws Exception {
}
}
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
Expand All @@ -94,13 +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));
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));
}

}