On Sat, 20 Jul 2024 07:38:25 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
>> 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.
I see the prevalent way to deal with runtime parse errors is to throw a java
exception. That exception later is caught in the command processing loop at the
entrance of the attach listener thread.
So, @SoniaZaldana, I would do this here too - when in Rome...
But is this not unnecessarily complex? It requires the AttachListener to be a
java thread when in fact it does need no java - we just misuse java exception
handling as a way to pass error information up the stack, with the simple
ultimate goal of writing error information into the outputStream to be sent to
the caller. We might just as well pass the outputStream* to the parse_xxx
functions as third argument, and write directly and return some error code.
This would make the attach listener thread a lot less dependent on Java and
more robust - at least for jcmds that don't need Java (which jcmds need java?).
After all, the attach listener is supposed to be super robust and always work
even if the JVM misbehaves. @dholmes-ora @lmesnik what do you guys think,
should we change that? (obviously in a different RFE)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1685304522
