Whatever you are comfortable with, we will have the final patch reviewed on the public list though.
Please keep me in the direct email recipient to make sure I read the email in time. Cheers, Henry > On Nov 4, 2019, at 3:40 PM, Mat Carter <matthew.car...@microsoft.com> wrote: > > Thanks Henry > > I'm working on a test, do you want me to share directly with you (incl. > questions) or continue with comms through the mailing list > > Cheers > Mat > > From: Henry Jen <henry....@oracle.com> > Sent: Monday, November 4, 2019 12:47 PM > 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 > > Hi Mat, > > I’ll sponsor this fix, let me know if you are coming up with test and I’ll > work with you to get your patch committed. > > Cheers, > Henry > > > > On Nov 4, 2019, at 9:21 AM, Henry Jen <henry....@oracle.com> wrote: > > > > 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%7C657f0afad82b4d31670a08d761682b0b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637085026486748197&sdata=19S%2BLGCPaoy0h2YmbKrKfsC21d6B43rITkZhnAKyuHw%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 > > >
