On Fri, 12 Nov 2021 08:25:04 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:

> jmm_GetDiagnosticCommandArgumentsInfo and jmm_GetDiagnosticCommandInfo are 
> used to query the hotspot about diagnostic commands. They provide output 
> arrays for the information:
> 
> 
> void jmm_GetDiagnosticCommandArgumentsInfo(JNIEnv *env,
>           jstring command, dcmdArgInfo* infoArray)
> 
> 
> but array size is implicitly assumed to be known to both caller and callee. 
> Caller and callee negotiate those sizes in prior steps, but things can go 
> wrong. E.g. I recently hunted a bug where `DCmd::number_arguments()` was off 
> - did not reflect the real number of its jcmd parameters - which led to a 
> hidden memory overwriter.
> 
> Thankfully, JDK-8264565 rewrote the dcmd framework to deal with this 
> particular issue (The VM I analyzed was older). Still, it would be good if we 
> had additional safety measures here.
> 
> -------------
> 
> Testing:
> - manual tests with artificially induced error in one dcmd for debug, release
> - GHAs (which include tier1 serviceability jcmd tests which use JMX and 
> exercise these APIs)

Hi Thomas,

Sorry but only half of this makes sense to me.

Cheers,
David

src/hotspot/share/services/management.cpp line 1968:

> 1966: 
> 1967: JVM_ENTRY(void, jmm_GetDiagnosticCommandInfo(JNIEnv *env, jobjectArray 
> cmds,
> 1968:           dcmdInfo* infoArray, jint count))

I do not see the point of the change in this case. This doesn't check for a 
mismatch between an "external" and "internal" value but between two external 
values. If you don't trust the caller to size infoArray correctly then how can 
you trust them to pass in the right "count" ?

src/hotspot/share/services/management.cpp line 2022:

> 2020: 
> 2021: JVM_ENTRY(void, jmm_GetDiagnosticCommandArgumentsInfo(JNIEnv *env,
> 2022:           jstring command, dcmdArgInfo* infoArray, jint count))

This one makes sense because you verify that the external expectation about the 
arg count matches the actual internal representation.

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

PR: https://git.openjdk.java.net/jdk/pull/6363

Reply via email to