Thanks for confirmation and for updating the bug Henry. Please let me know if there's anything else related to this issue I can help with; tests and/or verification etc.
In the meantime, I'll take a look at the existing ArgsFile tests for my own education. Cheers Mat ________________________________ From: Henry Jen <henry....@oracle.com> Sent: Monday, November 4, 2019 9:21 AM To: Mat Carter <matthew.car...@microsoft.com> Cc: core-libs-dev@openjdk.java.net <core-libs-dev@openjdk.java.net> Subject: Re: JDK-8231863 bug fix candidate 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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8231863&data=02%7C01%7CMatthew.Carter%40microsoft.com%7C9b5c79a7e6554d6ee8ac08d7614b7614%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637084959537829868&sdata=Phjbn2LibUnrnkg%2BsJhciIIXCIFi086pK0dBoleXJr8%3D&reserved=0), > 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
