On Tue, 8 Apr 2025 13:47:10 GMT, Kevin Walls <[email protected]> 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
