8353727: HeapDumpPath doesn't expand %p

[kevinjwalls]

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).

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8353727: HeapDumpPath doesn't expand %p (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24482/head:pull/24482
$ git checkout pull/24482

Update a local copy of the PR:
$ git checkout pull/24482
$ git pull https://git.openjdk.org/jdk.git pull/24482/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24482

View PR using the GUI difftool:
$ git pr show -t 24482

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24482.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back kevinw! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@kevinjwalls The following labels will be automatically applied to this pull request:

• hotspot-runtime
• serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (c32e4ca4)
• 00: Full (ab82116e)

[zhengyu123]

@kevinjwalls I have JDK-8349083 to address similar issues.

AFAICT, there are 3 separate code to handle filename expansion and logging has the most complete support, It will be nice to unify them.

[kevinjwalls]

@kevinjwalls I have JDK-8349083 to address similar issues.

AFAICT, there are 3 separate code to handle filename expansion and logging has the most complete support, It will be nice to unify them.

Hi, thanks for the pointer.
Yes, we have some duplication in this area... This change is quite small, and removes one duplicate, the manual "base+pid+extension" creation of the filename in HeapDumper.
I am looking at the other PR, maybe we can make them share more in future...

[kevinjwalls]

Updated.
Additionally, the total_length check at line 2760 is wrong now. But it is also redundant, we use copy_expand_pid to do our length checks on expansion. Use max_digit_chars to reduce buffer length in those copy_expand_pid calls, to leave room for possible later sequence numbers (this is very conservative).

On longer path lengths, worth noting that using MAXPATHLEN (4k) is higher than outputStream::print_cr allows.
This means we can get through all of HeapDumper::dump_heap(bool oome) and call HeapDumper::dump() which uses:
2606 out->print_cr("Dumping heap to %s ...", path);
..and will show a VM warning like "outputStream::do_vsnprintf output truncated"

Again, very very long HeapDumpPaths are not efficient. 8-)

[tstuefe]

Updated. Additionally, the total_length check at line 2760 is wrong now. But it is also redundant, we use copy_expand_pid to do our length checks on expansion. Use max_digit_chars to reduce buffer length in those copy_expand_pid calls, to leave room for possible later sequence numbers (this is very conservative).

On longer path lengths, worth noting that using MAXPATHLEN (4k) is higher than outputStream::print_cr allows. This means we can get through all of HeapDumper::dump_heap(bool oome) and call HeapDumper::dump() which uses: 2606 out->print_cr("Dumping heap to %s ...", path); ..and will show a VM warning like "outputStream::do_vsnprintf output truncated"

Again, very very long HeapDumpPaths are not efficient. 8-)

I keep thinking that such a coding can benefit from using stringStream; it makes most of the associated buffer counting etc obsolete, supports dynamic buffers, or optionally can be laid over a fixed-sized buffer and then handles truncation. It does not yet provide a way to report truncation, but that can be added really easily.

[kevinjwalls]

I keep thinking that such a coding can benefit from using stringStream; it makes most of the associated buffer counting etc obsolete, supports dynamic buffers, or optionally can be laid over a fixed-sized buffer and then handles truncation. It does not yet provide a way to report truncation, but that can be added really easily.

Thanks Thomas - Yes interesting. Would be good to get the %p honoured now, it's been missing for so long. But handling of very long paths would be a good reason to revisit this option and others like it.

[tstuefe]

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.

[kevinjwalls]

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!

[tstuefe]

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