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 >
