The fix is on the spot, good catch. As the test, test/jdk/tools/launcher/ArgsFileTest.java is testing the file expansions, we can add a test case there to have an argument file not have newline at the end, and check that we get correct arguments.
Cheers, Henry > On Nov 1, 2019, at 7:06 AM, Mat Carter <matthew.car...@microsoft.com> wrote: > > Hello core-libs-dev > > I'm a VM engineer at Microsoft, now that we've signed the OCA (news of which > was shared by Bruno Borges in the discuss mailing list) I wanted to pick up > an issue in order to gain familiarity with the change process > > After reproducing the error reported in "JDK-8231863: Crash if classpath is > read from @argument file and the main gets option argument" > (https://bugs.openjdk.java.net/browse/JDK-8231863), I've identified the root > cause and a candidate fix. > > As such I'd like to pick this issue up, assuming its current state of > unassigned still holds what's the process of having it assigned to me or > having it somehow reserved as I don't want to cause unnecessary duplicated > work. > > As well as validating the fix in the debugger on WIndows and Linux, I've run > the AdoptOpenJDK sanity tests on Mac, Windows and Linux for both tip and 11u. > > I'm currently figuring out where/how to write regression tests for this case; > input from this mailing list would be welcomed (examples of tests for similar > bugs etc) > > This error occurs on all platforms (tip + 11u) but only results in a crash on > Windows in java_md.c due to that platforms dependency on > JLI_GetAppArgIndex(). Note that while the error is 100% reproducible, the > crash only occurs <10%. The following change fixes that problem by passing > in the final token fragment from the argument file into the state machine via > checkArg() > > --- a/src/java.base/share/native/libjli/args.c > +++ b/src/java.base/share/native/libjli/args.c > @@ -337,7 +337,9 @@ static JLI_List readArgFile(FILE *file) { > // remaining partial token > if (ctx.state == IN_TOKEN || ctx.state == IN_QUOTE) { > if (ctx.parts->size != 0) { > - JLI_List_add(rv, JLI_List_combine(ctx.parts)); > + token = JLI_List_combine(ctx.parts); > + checkArg(token); > + JLI_List_add(rv, token); > } > } > JLI_List_free(ctx.parts); > > The fix tackles the memory corruption indirectly. Further preventative > changes could be made in java_md.c/CreateApplicationArgs to avoid future > memory corruptions (caused by JLI_GetAppArgIndex() returning -1 in this case) > ; but I felt that that handling those error cases ran secondary to addressing > the bug in the argument file parsing code. > > Cheers > Mat
