Hi,
Sorry for this late reply!

I only focus on the changes on the tests in sun/security/tools/jarsigner/compatibility. Compatibility.java has been modified significantly, I just had a quick look at it.

sun/security/tools/jarsigner/compatibility/SignTwice.java
--------------------
With my testing, this test usually got timeout in our CI testing system.
The timeout is 480 seconds.


sun/security/tools/jarsigner/compatibility/Compatibility.java
--------------------
 38 import java.io.ByteArrayOutputStream;
 44 import java.io.FilterOutputStream;
 65 import java.util.jar.Attributes;
The above import statements are unnecessary.

 82     private static JdkInfo TEST_JDK_INFO;
100     private static String[] KEY_ALGORITHMS;
111     private static String[] DIGEST_ALGORITHMS;
Now that these variables are not constant anymore, the names would be testJdkInfo, keyAlgorithms and digestAlgorithms.

161         System.out.println("using sigfile " + sigfileName + " for alias " 162                     + alias + " signing " + u + ".jar to " + s + ".jar");
Could it be better to use System.out.printf()?

409     private static String outfile() {
410         return System.getProperty("o");
411     }
Would it be better to define a constant for outfile?

416         String[] jdkList = list("jdkList");
417         if (jdkList.length == 0) {
418             jdkList = new String[] { "TEST_JDK" };
419         }
420
421         List<JdkInfo> jdkInfoList = new ArrayList<>();
422         for (String jdkPath : jdkList) {
423             JdkInfo jdkInfo = "TEST_JDK".equalsIgnoreCase(jdkPath) ?
424                     TEST_JDK_INFO : new JdkInfo(jdkPath);
Not sure understand this update.
Now that the constant TEST_JDK is the current testing JDK path, then new JdkInfo(TEST_JDK) should be equivalent to TEST_JDK_INFO in this update.

435     private static List<String> keyAlgs() throws IOException {
436         if (KEY_ALGORITHMS == null) KEY_ALGORITHMS = list("keyAlgs");
437         if (KEY_ALGORITHMS.length == 0)
438             return Arrays.asList(DEFAULT_KEY_ALGORITHMS);
439         return Arrays.stream(KEY_ALGORITHMS).map(a -> a.split(";")[0])
440                 .collect(Collectors.toList());
441     }
...
467     private static List<String> digestAlgs() throws IOException {
468         if (DIGEST_ALGORITHMS == null) DIGEST_ALGORITHMS = list("digestAlgs");
469         if (DIGEST_ALGORITHMS.length == 0)
470             return Arrays.asList(DEFAULT_DIGEST_ALGORITHMS);
471         return Arrays.asList(DIGEST_ALGORITHMS);
472     }
It looks better that the above methods return String array rather than list. Then, the callings on Arrays.asList() are unnecessary.

793             System.out.println("verifyingStatus: error: exit code != " + expectedExitCode + ": " + outputAnalyzer.getExitValue() + " != " + expectedExitCode);
...
799             System.out.println("verifyingStatus: error: expectedSuccessMessage not found: " + expectedSuccessMessage);
...
810                 System.out.println("verifyingStatus: error: line.matches(" + Test.ERROR + "\" ?\"): " + line);
The above lines are too long.


sun/security/tools/jarsigner/compatibility/JdkUtils.java
--------------------
The year should be updated in the copyright notes.

Best regards,
John Jiang

On 2019/6/21 11:34, Weijun Wang wrote:
BTW, I haven't looked at the changes in 
test/jdk/sun/security/tools/jarsigner/compatibility yet.

John, do you have any comment in this area?

Thanks,
Max

On Jun 21, 2019, at 11:32 AM, Weijun Wang <weijun.w...@oracle.com> wrote:

The tests.

- For all:

We are thinking about removing default value for -keyalg for "keytool -genkeypair" some day, so it 
will be better to add an explicit "-keyalg RSA" or "-keyalg EC" now.

There are several tests printing out the manifest in a sequence of "[[0]=83='S', ...". While it is precise, I 
find it not very easy to check. How about we just print out the string format but for each "\r" we just print 
"\r" literally and keep printing "\n" with a new line,  like this:

Signature-Version: 1.0\r
Created-By: 13-internal (Oracle Corporation)\r
SHA-256-Digest-Manifest: Pxklci7ELW1zzSh2jZ+oz7TPR0WK2uTsAIiHar1m6Eo=\r
SHA-256-Digest-Manifest-Main-Attributes: ETfvdTNx3yN/ClSZz20wR0SM9ta8H7O\r
E7U0F4uZ9JV8=\r
\r
Name: abc\r
SHA-256-Digest: uTZ8UM3iJLPtl8W7+NEC/AC2auI7LP2Gu53ajj6jLgg=\r
\r

If you find running jarsigner slow, is the JarSigner::sign API any better? The 
verify side can also usually be replaced with open the file and read everything 
inside unless you want to grab certain outputs, but those can also be done with 
JarEntry::getSigners.

- DigestDontIgnoreCase.java:

getManifestBytes(). This could reformat the manifest (I understand it's 
irrelevant in this test), you can just return

   jf.getInputStream(jf.getJarEntry(JarFile.MANIFEST_NAME)).readAllBytes()

- EmptyIndividualSectionName.java:

Are you going to do something around testNameEmptyTrusted()?

- FindHeaderEndVsManifestDigesterFindFirstSection.java:

Several TODO and FIXME.

52: typo: backwars-

76: No need to maintain FIXED_8217375 anymore

229: Typo: ManifestSigester

- InsufficientSectionDelimiter.java:

We usually abbreviate "attribute" to "attr" instead of "att".

We usually use Path.of() instead of Paths.get() these days.

- MainAttributesConfused.java:

Have you tried adding a file named "Manifest-Main-Attributes" into a jar and 
see if it would mess up anything?

- PreserveRawManifestEntryAndDigest.java:

This test is running for a long time. The output is also quite big and you can see 
"Output overflow" inside which means if there is a failure it might not show. 
Is it possible to simplify it?

getExpectedJarSignerOutputUpdatedContentNotValidatedBySignerA(). The output 
might change often. If you only focus on several lines, it's better to check 
them with OutputAnalyzer::shouldMatch***. We can always manually construct an 
OutputAnalyzer object.

- DigestInput.java and FindSection.java:

static final boolean FIXED_8217375 = true;

Is this still needed?

- OutputAnalyzer.java:

This is a shared utility class and you changed the behavior. It might be 
correct (start matching *after* the from line) but I cannot be sure if it would 
break anything. I'll ask people about it.

Thanks,
Max




On Jun 11, 2019, at 3:08 PM, Weijun Wang <weijun.w...@oracle.com> wrote:

Hi Philipp,

I'll start reviewing this code change. Since this is a P3 bug fix, we still 
have a month's time (before RDP 2 starts on 7/18) to work on it.

Also, I've included John as a reviewer. He is the author of the 
Compatibility.java test.

Thanks,
Max

On May 23, 2019, at 9:49 AM, Weijun Wang <weijun.w...@oracle.com> wrote:

Hi Philipp,

I've just uploaded your patch to

http://cr.openjdk.java.net/~weijun/8217375/webrev.01/

Reply via email to