RFR: 8334492: DiagnosticCommands (jcmd) should accept %p in output filenames and substitute PID [v5]

[Thomas Stuefe]

On Fri, 19 Jul 2024 20:00:28 GMT, Leonid Mesnik <lmesnik at openjdk.org> wrote:

>> Sonia Zaldana Calles has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Missing copyright header update
>
> src/hotspot/share/services/diagnosticArgument.cpp line 366:
> 
>> 364:     _value._name = NEW_C_HEAP_ARRAY(char, JVM_MAXPATHLEN, mtInternal);
>> 365:     if (!Arguments::copy_expand_pid(str, len, _value._name, JVM_MAXPATHLEN)) {
>> 366:       fatal("Invalid file path: %s", str);
> 
> As I understand the 'copy_expand_pid' might fail if very long line is used. This cause jvm crash.,
> So there is possibility that user might crash jvm accidentally invoking jcmd command. 
> It doesn't look safe, I believe it would be better to throw Exception like for any other invalid command, see
> " THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(),"
> 
> The 'fatal" owuld make sense only if failing of 'copy_expand_pid' means some unrecoverable jvm bug.

Yes. In this file, other commands use `fatal` only where reading the hard-coded default values - in the various `init_...` functions. Hard-coded values should be valid, obviously, otherwise the JVM developer messed up. Other values are passed in by the end user via jcmd and should not crash the JVM.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1685299871