Hi, Need a quick review of the webrev[1] for JDK-8210810[2], it’s pretty much what Bo contributed, just add some trailing text “aaa\” to verify the integrity of escape sequence is handled properly.
I had reviewed the change and tested, but we need a “R”eviewer. Cheers, Henry [1] http://cr.openjdk.java.net/~henryjen/jdk/8210810/webrev/ [2] https://bugs.openjdk.java.net/browse/JDK-8210810 > On Sep 26, 2018, at 6:45 PM, Bo Zhang <[email protected]> wrote: > > Thank you Henry. Feel free to modify it. > > Regards, > Bo > >> On 27 Sep 2018, at 09:39, Henry Jen <[email protected]> wrote: >> >> Thanks for the fix. I had looked into it, it’s a nice catch, glad you find >> it. >> >> The patch looks good, and test is sufficient, I have tested it with jdk.jdk >> repo. I will help you to push the fix. The only minor improvement I like to >> see is to verify content after the escape character, which I took the >> liberty to do it. Webrev coming soon. >> >> Cheers, >> Henry >> >>> On Sep 26, 2018, at 4:03 PM, Bo Zhang <[email protected]> wrote: >>> >>> Can anyone please take a look? >>> >>> Thank you. >>> >>>> On 25 Sep 2018, at 14:27, Bo Zhang <[email protected]> wrote: >>>> >>>> Hi all, >>>> >>>> This is my first time to try to submit a patch. Please forgive me if I did >>>> anything inappropriate. I’ve signed the OCA with the name “Bo Zhang”. >>>> >>>> Bug description: >>>> >>>> 8210810 <https://bugs.openjdk.java.net/browse/JDK-8210810>: Incorrect >>>> argument file parser behavior in certain cases >>>> >>>> Previously, if a quoted backslash character appears at argument file >>>> parser buffer boundary (i.e. 4096th/8192nd/... character of an argument >>>> file is backslash '\'), the segment before the backslash character will >>>> be added into argument twice due to a missing anchor position reset >>>> operation. >>>> >>>> For example, you have an argument file: >>>> >>>> -cp "...a\\b" >>>> >>>> and the 4096th and 4097th characters happen to be both '\', the result >>>> generated by the argument file parser would be: >>>> >>>> -cp "...aa\\b” >>>> >>>> This patch fixes the issue by resetting anchor position correctly >>>> in this case. >>>> >>>> I’m a little surprised that this issue has existed for several years >>>> unnoticed. >>>> >>>> >>>> Patch: >>>> >>>> # HG changeset patch >>>> # User Bo Zhang <[email protected] <mailto:[email protected]>> >>>> # Date 1537835887 -28800 >>>> # Tue Sep 25 08:38:07 2018 +0800 >>>> # Node ID 34f2803ad62cde8c82e3dfcf0b66e80276bd7352 >>>> # Parent 16f0deae8fa6ef85f7230308e1fe7556bd007c39 >>>> 8210810: Incorrect argument file parser behavior in certain cases >>>> >>>> Previously, if a quoted backslash character appears at argument file >>>> parser buffer boundary (i.e. 4096th/8192nd/... character of an argument >>>> file is backslash '\'), the segment before the backslash character will >>>> be added into argument twice due to a missing anchor position reset >>>> operation. >>>> >>>> For example, you have an argument file: >>>> >>>> -cp "...a\\b" >>>> >>>> and the 4096th and 4097th characters happen to be both '\', the result >>>> generated by the argument file parser would be: >>>> >>>> -cp "...aa\\b" >>>> >>>> This patch fixes the issue by resetting anchor position correctly >>>> in this case. >>>> >>>> diff --git a/src/java.base/share/native/libjli/args.c >>>> b/src/java.base/share/native/libjli/args.c >>>> --- a/src/java.base/share/native/libjli/args.c >>>> +++ b/src/java.base/share/native/libjli/args.c >>>> @@ -262,6 +262,8 @@ >>>> continue; >>>> } >>>> JLI_List_addSubstring(pctx->parts, anchor, nextc - anchor); >>>> + // anchor after backslash character >>>> + anchor = nextc + 1; >>>> pctx->state = IN_ESCAPE; >>>> break; >>>> case '\'': >>>> diff --git a/test/jdk/tools/launcher/Test8210810.java >>>> b/test/jdk/tools/launcher/Test8210810.java >>>> new file mode 100644 >>>> --- /dev/null >>>> +++ b/test/jdk/tools/launcher/Test8210810.java >>>> @@ -0,0 +1,107 @@ >>>> +/* >>>> + * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved. >>>> + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. >>>> + * >>>> + * This code is free software; you can redistribute it and/or modify it >>>> + * under the terms of the GNU General Public License version 2 only, as >>>> + * published by the Free Software Foundation. >>>> + * >>>> + * This code is distributed in the hope that it will be useful, but >>>> WITHOUT >>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >>>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License >>>> + * version 2 for more details (a copy is included in the LICENSE file that >>>> + * accompanied this code). >>>> + * >>>> + * You should have received a copy of the GNU General Public License >>>> version >>>> + * 2 along with this work; if not, write to the Free Software Foundation, >>>> + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. >>>> + * >>>> + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA >>>> + * or visit www.oracle.com <http://www.oracle.com/> if you need >>>> additional information or have any >>>> + * questions. >>>> + */ >>>> + >>>> +/* >>>> + * @test >>>> + * @bug 8210810 >>>> + * @summary Incorrect argument file parser behavior when backslash >>>> appears at buffer boundary >>>> + * @modules java.base >>>> + * @author Bo Zhang >>>> + * @build TestHelper >>>> + * @run main Test8210810 >>>> + */ >>>> + >>>> +/* >>>> + * 8210810: Previously, if a quoted backslash character appears at >>>> argument file >>>> + * parser buffer boundary (i.e. 4096th/8192nd/... character of an argument >>>> + * file is backslash '\'), the segment before the backslash character will >>>> + * be added into argument twice due to a missing anchor position reset >>>> + * operation. This test creates a special argument file which can >>>> reproduce this issue. >>>> + */ >>>> +import java.io.File; >>>> +import java.nio.file.Files; >>>> +import java.io.IOException; >>>> +import java.util.Arrays; >>>> + >>>> +public class Test8210810 extends TestHelper { >>>> + // Buffer size in args.c readArgFile() method >>>> + private static final int ARG_FILE_PARSER_BUF_SIZE = 4096; >>>> + public static void main(String... args) throws Exception { >>>> + new Test8210810().canParseArgFileCorrectly(); >>>> + } >>>> + >>>> + private void canParseArgFileCorrectly() throws IOException { >>>> + File testJar = createTestClassWhichPrintsClasspath(); >>>> + File argFile = createArgFileWithSpecificClasspath(); >>>> + TestResult result = doExec(javaCmd, "@argFile", "Foo"); >>>> + >>>> + long zCountInArgFile = countZ(new >>>> String(Files.readAllBytes(argFile.toPath()))); >>>> + long zCountReadByParser = countZ(result.testOutput.get(0)); >>>> + if(zCountInArgFile != zCountReadByParser) { >>>> + throw new RuntimeException("Classpath parsed by parser is >>>> different from argument file!"); >>>> + } >>>> + >>>> + testJar.delete(); >>>> + argFile.delete(); >>>> + } >>>> + >>>> + private long countZ(String str) { >>>> + return str.chars().filter(ch -> ch == 'z').count(); >>>> + } >>>> + >>>> + private File createTestClassWhichPrintsClasspath() throws IOException >>>> { >>>> + File testJar = new File("test.jar"); >>>> + testJar.delete(); >>>> + >>>> + StringBuilder tsrc = new StringBuilder(); >>>> + tsrc.append("public static void main(String... args) {\n"); >>>> + tsrc.append(" System.out.println(\"Classpath:\" + >>>> System.getProperty(\"java.class.path\"));\n"); >>>> + tsrc.append("}\n"); >>>> + createJar(testJar, new File("Foo"), tsrc.toString()); >>>> + return testJar; >>>> + } >>>> + >>>> + // Create a file whose 4096th and 4097th characters are both '\' >>>> (backslash) >>>> + // File content: >>>> + // |-cp "test.jar:zzzzz...zzz\\"| >>>> + private File createArgFileWithSpecificClasspath() throws IOException { >>>> + File argFile = new File("argFile"); >>>> + argFile.delete(); >>>> + >>>> + StringBuilder arg = new StringBuilder("-cp \"test.jar"); >>>> + arg.append(File.pathSeparator); >>>> + >>>> + int fillingCharactersCount = ARG_FILE_PARSER_BUF_SIZE - 1 - >>>> arg.length(); >>>> + >>>> + for(int i = 0; i < fillingCharactersCount; ++i) { >>>> + arg.append('z'); >>>> + } >>>> + >>>> + arg.append('\\'); // 4096th character in the arg file >>>> + arg.append('\\'); // 4097th character in the arg file >>>> + arg.append('"'); >>>> + >>>> + createAFile(argFile, Arrays.asList(arg.toString())); >>>> + return argFile; >>>> + } >>>> +} >>>> >>>> Unit test: >>>> >>>> Included in the patch. It creates a special argument file whose 4096th and >>>> 4097th characters are both ‘\’. Then it compares the classpath read in the >>>> VM and the classpath in the argument file to verify they’re identical. >>>> >>>> <8210810.patch> >
