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

[Kevin Walls]

On Tue, 8 Apr 2025 13:13:25 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

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

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!

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

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