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

[Thomas Stuefe]

On Tue, 8 Apr 2025 13:47:10 GMT, Kevin Walls <kevinw at openjdk.org> wrote:

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

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

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

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