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://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 >
