On Fri, 19 Jul 2024 20:00:28 GMT, Leonid Mesnik <[email protected]> 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
