On Wed, 30 Sep 2020 04:59:20 GMT, Yumin Qi <mi...@openjdk.org> wrote:
>> This patch is reorganized after 8252725, which is separated from this patch >> to refactor jlink glugin code. The previous >> webrev with hg can be found at: >> http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725 >> integrated, the >> regeneration of holder classes is simply to call the new added >> GenerateJLIClassesHelper.cdsGenerateHolderClasses >> function. Tests: tier1-4 > > Yumin Qi has updated the pull request incrementally with one additional > commit since the last revision: > > Remove trailing word of line which is not used in holder class > regeneration. There is a trailing LF (Line Feed) so trim > white spaces from both front and end of the line or it will fail method > type validation. Changes requested by iklam (Reviewer). test/hotspot/jtreg/runtime/cds/appcds/DumpClassListWithLF.java line 70: > 68: "Hello", > 69: "@lambda-form-invoker [LF_RESOLVE] > java.lang.invoke.DirectMethodHandle$Holder invokeNothing L7_L > (anyword)", 70: "@lambda-form-invoker [LF_RESOLVE] > java.lang.invoke.DirectMethodHandle$Holder > invokeNothing LL_I anyword"), We shouldn't allow the classlist to contain arbitrary data. These two cases should generate an error. src/hotspot/share/classfile/lambdaFormInvokers.cpp line 52: > 50: > 51: // trim white spaces from front and end of string. > 52: char* trim(char* s) { I think this creates unnecessary dependency between the C code and the Java code. The C code assumes that the Java code has appended something like "(salvaged)" into the output, and tries to get rid of that in a non-obvious way. It's better to modify the Java code from static void traceSpeciesType(String cn, Class<?> salvage) { if (TRACE_RESOLVE || CDS.isDumpLoadedClassList()) { String traceSP = SPECIES_RESOLVE + " " + cn + (salvage != null ? " (salvaged)" : " (generated)"); if (TRACE_RESOLVE) { System.out.println(traceSP); } CDS.logTraceResolve(traceSP); } } to if (TRACE_RESOLVE || CDS.isDumpLoadedClassList()) { String traceSP = SPECIES_RESOLVE + " " + cn; if (TRACE_RESOLVE) { System.out.println(traceSP + (salvage != null ? " (salvaged)" : " (generated)")); } CDS.logTraceResolve(traceSP); } test/hotspot/jtreg/runtime/cds/appcds/DumpClassListWithLF.java line 85: > 83: appJar, classlist( > 84: "Hello", > 85: "@lambda-form-invoker [LF_XYRESOLVE] > java.lang.invoke.DirectMethodHandle$Holder invokeStatic L7_L > (any)", We should not allow incorrect input. This should generate an error. test/hotspot/jtreg/runtime/cds/appcds/DumpClassListWithLF.java line 78: > 76: "Hello", > 77: "@lambda-form-invoker [LF_RESOLVE] > my.nonexist.package.MyNonExistClassName$holder invokeStatic > L7_L", 78: "@lambda-form-invoker [LF_RESOLVE] > my.nonexist.package.MyNonExistClassName$holder > invokeStatic LL_I"), I think it's dangerous to allow arbitrary class names here. InvokerBytecodeGenerator doesn't check the classname. This will make it possible to overwrite the contents of arbitrary classes. We should have a check here and allow only the specific holder classes that are supported. ------------- PR: https://git.openjdk.java.net/jdk/pull/193
