RFR: 8353727: HeapDumpPath doesn't expand %p [v2]

[Thomas Stuefe]

On Tue, 8 Apr 2025 11:43:20 GMT, Kevin Walls <kevinw at openjdk.org> wrote:

>> This is a long-standing oversight: HeapDumpPath does not recognise %p for pid expansion.
>> The default filename uses a pid (e.g. java_pid1676937.hprof) but HeapDumpPath does not.
>> It has always done a manual "root plus pid plus extension" on the default filename only, and
>> should move to using Argument::copy_expand_pid() like we do with other such filenames.
>> 
>> 
>> We also assumed the default filename is not a directory (which is very very likely, but doesn't have to be true).
>
> Kevin Walls has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - length checking update
>  - Chris feedback

src/hotspot/share/services/heapDumper.cpp line 2779:

> 2777:       }
> 2778:       // Then add the default name, with %p substitution.  Use my_path temporarily.
> 2779:       if (!Arguments::copy_expand_pid(dump_file_name, strlen(dump_file_name), my_path, JVM_MAXPATHLEN - max_digit_chars)) {

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.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/24482#discussion_r2033167264