[8u] RFR 8236645: JDK 8u231 introduces a regression with incompatible handling of XML messages

[Elliott Baron]

Hi Severin,

On 2020-08-19 2:02 p.m., Severin Gehwolf wrote:
> Hi Elliott,
> 
> On Wed, 2020-08-19 at 12:54 -0400, Elliott Baron wrote:
>> Hi,
>>
>> I'd like to request a review of 8236645 for 8u. 8236645 is a bug fix for
>> Oracle JDK 8 that I have re-implemented for OpenJDK 8u.
>>
>> Original fix:
>> https://bugs.openjdk.java.net/browse/JDK-8236645
>> CSR:
>> https://bugs.openjdk.java.net/browse/JDK-8237765
>>
>> This fixes a regression introduced by 8177334 and 8217878 by introducing
>> a new system property
>> "com.sun.org.apache.xml.internal.security.lineFeedOnly", which gives
>> Base64 encodings in XML digital signatures the same line-ending format
>> as before these changes were made. The CSR outlined the behaviour of
>> this system property and how it was originally implemented.
>>
>> I added a new test file for this change, based on test cases in
>> javax/xml/crypto/dsig/GenerationTests. This allows us to run the test
>> with the property enabled and disabled, and also doesn't interfere with
>> future backports affecting GenerationTests.
>>
>> 8u webrev:
>> https://cr.openjdk.java.net/~ebaron/jdk8u/JDK-8236645/webrev.00/
> 
> The CSR states:
> """
> This new property has no effect on default behavior nor when
> com.sun.org.apache.xml.internal.security.ignoreLineBreaks property is
> set.
> """
> 
> If I read your patch right, then it would not work as specified when
> both com.sun.org.apache.xml.internal.security.ignoreLineBreaks *and*
> com.sun.org.apache.xml.internal.security.lineFeedOnly are both set:
> 
>   451     public static String encodeToString(byte[] bytes) {
>   452         if (lineFeedOnly) {
>   453             return LF_ENCODER.encodeToString(bytes);
>   454         }
>   455         if (ignoreLineBreaks) {
>   456             return Base64.getEncoder().encodeToString(bytes);
>   457         }
>   458         return Base64.getMimeEncoder().encodeToString(bytes);
>   459     }
> 
> Lines 452-454 should move after line 457.

Good catch! Thank you for pointing this out.

> 
> test/javax/xml/crypto/dsig/LineFeedOnlyTest.java
> 
>    63 /* @test id=LF
>    64  * @bug 8236645
>    65  * @summary Test "lineFeedOnly" property prevents CR in Base64 encoded output
>    66  * @run main/othervm/timeout=300 -Dcom.sun.org.apache.xml.internal.security.lineFeedOnly=true LineFeedOnlyTest
>    67  */
>    68
>    69 /* @test id=CRLF
>    70  * @bug 8236645
>    71  * @summary Test "lineFeedOnly" property prevents CR in Base64 encoded output
>    72  * @run main/othervm/timeout=300 -Dcom.sun.org.apache.xml.internal.security.lineFeedOnly=false LineFeedOnlyTest
>    73  */
> 
> Couldn't we simplify this by using just multiple @run tags?
> 
> How about this?
> 
> /* @test
>   * @bug 8236645
>   * @summary Test "lineFeedOnly" property prevents CR in Base64 encoded output
>   * @run main/othervm/timeout=300 -Dcom.sun.org.apache.xml.internal.security.lineFeedOnly=false LineFeedOnlyTest
>   * @run main/othervm/timeout=300 -Dcom.sun.org.apache.xml.internal.security.lineFeedOnly=true LineFeedOnlyTest
>   */

Sounds good to me.

Here's an updated webrev that addresses both of these points:
https://cr.openjdk.java.net/~ebaron/jdk8u/JDK-8236645/webrev.01/

Thanks,
Elliott