On Tue, 8 Apr 2025 13:13:25 GMT, Thomas Stuefe <stu...@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
