From 7ef4cb626d433526431573b8e03723cbdea73138 Mon Sep 17 00:00:00 2001 From: Johannes Graham Date: Fri, 11 Apr 2025 13:24:49 -0400 Subject: [PATCH 01/10] reset "data" field in clone --- src/java.base/share/classes/java/text/DigitList.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/java.base/share/classes/java/text/DigitList.java b/src/java.base/share/classes/java/text/DigitList.java index fde0e93214ad2..2efe7a4781b1d 100644 --- a/src/java.base/share/classes/java/text/DigitList.java +++ b/src/java.base/share/classes/java/text/DigitList.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 1996, 2023, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1996, 2025, 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 @@ -725,6 +725,7 @@ public Object clone() { char[] newDigits = new char[digits.length]; System.arraycopy(digits, 0, newDigits, 0, digits.length); other.digits = newDigits; + other.data = null; other.tempBuilder = null; return other; } catch (CloneNotSupportedException e) { From 1f39f7d019c78ed9bed57448318a79fe8a0e0ecb Mon Sep 17 00:00:00 2001 From: Johannes Graham Date: Tue, 15 Apr 2025 10:39:53 -0400 Subject: [PATCH 02/10] add test --- .../text/Format/DecimalFormat/CloneTest.java | 69 +++++++++++++++++++ 1 file changed, 69 insertions(+) create mode 100644 test/jdk/java/text/Format/DecimalFormat/CloneTest.java diff --git a/test/jdk/java/text/Format/DecimalFormat/CloneTest.java b/test/jdk/java/text/Format/DecimalFormat/CloneTest.java new file mode 100644 index 0000000000000..d3ec03883855e --- /dev/null +++ b/test/jdk/java/text/Format/DecimalFormat/CloneTest.java @@ -0,0 +1,69 @@ +/* + * Copyright (c) 2025, 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 if you need additional information or have any + * questions. + */ + +/* + * @test + * @bug 8354522 + * @summary Check parseStrict correctness for DecimalFormat.equals() + * @run junit CloneTest + */ + +import org.junit.jupiter.api.Test; + +import java.math.BigDecimal; +import java.text.DecimalFormat; +import java.util.concurrent.Executors; +import java.util.concurrent.atomic.AtomicInteger; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +public class CloneTest { + // Specifically tests that when DecimalFormat is cloned after use with + // a long double/BigDecimal, clones will be independent. This is not an + // exhaustive test. + @Test + public void testCloneIndependence() { + AtomicInteger mismatchCount = new AtomicInteger(0); + DecimalFormat df = new DecimalFormat("#"); + String _ = df.format(Math.PI); // initial use of formatter + try (var ex = Executors.newThreadPerTaskExecutor(Thread.ofPlatform().factory())) { + for (int i = 0; i < 50; i++) { + // each thread gets its own clone of df + DecimalFormat threadDf = (DecimalFormat) df.clone(); + Runnable task = () -> { + for (int j = 0; j < 1000000; j++) { + String dfString = threadDf.format(BigDecimal.valueOf(j)); + String str1 = String.valueOf(j); + if (!str1.equals(dfString)) { + System.err.println("mismatch: str = " + str1 + " dfString = " + dfString); + mismatchCount.incrementAndGet(); + break; + } + } + }; + ex.execute(task); + } + } + assertEquals(0, mismatchCount.get()); + } +} From 880b2d8908fb337b15ae76ce48fb091d9b09a8d7 Mon Sep 17 00:00:00 2001 From: Johannes Graham Date: Tue, 15 Apr 2025 10:40:42 -0400 Subject: [PATCH 03/10] fix test summary --- test/jdk/java/text/Format/DecimalFormat/CloneTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/jdk/java/text/Format/DecimalFormat/CloneTest.java b/test/jdk/java/text/Format/DecimalFormat/CloneTest.java index d3ec03883855e..2448940f1aa6b 100644 --- a/test/jdk/java/text/Format/DecimalFormat/CloneTest.java +++ b/test/jdk/java/text/Format/DecimalFormat/CloneTest.java @@ -24,7 +24,7 @@ /* * @test * @bug 8354522 - * @summary Check parseStrict correctness for DecimalFormat.equals() + * @summary Check for cloning interference * @run junit CloneTest */ From 61332647d34a8b73173f28175e2e3382e04e06cb Mon Sep 17 00:00:00 2001 From: Johannes Graham Date: Tue, 15 Apr 2025 14:32:36 -0400 Subject: [PATCH 04/10] Tune the test to shorten run-time, update comments --- .../share/classes/java/text/DigitList.java | 5 +++ .../text/Format/DecimalFormat/CloneTest.java | 39 ++++++++++++++----- 2 files changed, 34 insertions(+), 10 deletions(-) diff --git a/src/java.base/share/classes/java/text/DigitList.java b/src/java.base/share/classes/java/text/DigitList.java index 2efe7a4781b1d..f269698d6dacb 100644 --- a/src/java.base/share/classes/java/text/DigitList.java +++ b/src/java.base/share/classes/java/text/DigitList.java @@ -725,8 +725,13 @@ public Object clone() { char[] newDigits = new char[digits.length]; System.arraycopy(digits, 0, newDigits, 0, digits.length); other.digits = newDigits; + + // data and tempBuilder do not need to be copied because they do + // not carry significant information. They will be recreated on demand. + // Setting them to null is needed to avoid sharing across clones. other.data = null; other.tempBuilder = null; + return other; } catch (CloneNotSupportedException e) { throw new InternalError(e); diff --git a/test/jdk/java/text/Format/DecimalFormat/CloneTest.java b/test/jdk/java/text/Format/DecimalFormat/CloneTest.java index 2448940f1aa6b..047b3739455c1 100644 --- a/test/jdk/java/text/Format/DecimalFormat/CloneTest.java +++ b/test/jdk/java/text/Format/DecimalFormat/CloneTest.java @@ -32,37 +32,56 @@ import java.math.BigDecimal; import java.text.DecimalFormat; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.Executors; import java.util.concurrent.atomic.AtomicInteger; import static org.junit.jupiter.api.Assertions.assertEquals; public class CloneTest { - // Specifically tests that when DecimalFormat is cloned after use with + + // Tests that when DecimalFormat is cloned after use with // a long double/BigDecimal, clones will be independent. This is not an - // exhaustive test. + // exhaustive test. This tests for the issue of the same DigitList.data + // array being reused across clones of DecimalFormat. + @Test public void testCloneIndependence() { AtomicInteger mismatchCount = new AtomicInteger(0); DecimalFormat df = new DecimalFormat("#"); + CountDownLatch startSignal = new CountDownLatch(1); + + // This initial use of the formatter initialises its internal state, which could + // subsequently be shared across clones. This is key to reproducing this specific + // issue. String _ = df.format(Math.PI); // initial use of formatter + try (var ex = Executors.newThreadPerTaskExecutor(Thread.ofPlatform().factory())) { - for (int i = 0; i < 50; i++) { + for (int i = 0; i < 5; i++) { + final int finalI = i; // each thread gets its own clone of df DecimalFormat threadDf = (DecimalFormat) df.clone(); Runnable task = () -> { - for (int j = 0; j < 1000000; j++) { - String dfString = threadDf.format(BigDecimal.valueOf(j)); - String str1 = String.valueOf(j); - if (!str1.equals(dfString)) { - System.err.println("mismatch: str = " + str1 + " dfString = " + dfString); - mismatchCount.incrementAndGet(); - break; + try { + startSignal.await(); + for (int j = 0; j < 1_000; j++) { + int value = finalI * j; + String dfString = threadDf.format(BigDecimal.valueOf(value)); + String str1 = String.valueOf(value); + if (!str1.equals(dfString)) { + int count = mismatchCount.getAndIncrement(); + if (count < 5) { // limit lines of output + System.err.println("mismatch: str = " + str1 + " dfString = " + dfString); + } + } } + } catch (InterruptedException e) { + // just end } }; ex.execute(task); } + startSignal.countDown(); // let all tasks start working at the same time } assertEquals(0, mismatchCount.get()); } From dc5e466e2a48cbd5417447071b47c31a17e54dec Mon Sep 17 00:00:00 2001 From: Johannes Graham Date: Tue, 15 Apr 2025 16:38:05 -0400 Subject: [PATCH 05/10] Terminate test early on failure --- .../text/Format/DecimalFormat/CloneTest.java | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/test/jdk/java/text/Format/DecimalFormat/CloneTest.java b/test/jdk/java/text/Format/DecimalFormat/CloneTest.java index 047b3739455c1..29120118ec839 100644 --- a/test/jdk/java/text/Format/DecimalFormat/CloneTest.java +++ b/test/jdk/java/text/Format/DecimalFormat/CloneTest.java @@ -54,25 +54,28 @@ public void testCloneIndependence() { // This initial use of the formatter initialises its internal state, which could // subsequently be shared across clones. This is key to reproducing this specific // issue. - String _ = df.format(Math.PI); // initial use of formatter + String _ = df.format(Math.PI); // Initial use of formatter try (var ex = Executors.newThreadPerTaskExecutor(Thread.ofPlatform().factory())) { for (int i = 0; i < 5; i++) { final int finalI = i; - // each thread gets its own clone of df + // Each thread gets its own clone of df DecimalFormat threadDf = (DecimalFormat) df.clone(); Runnable task = () -> { try { startSignal.await(); for (int j = 0; j < 1_000; j++) { + if (mismatchCount.get() > 0) { + // Exit early if mismatch has already occurred + break; + } + int value = finalI * j; String dfString = threadDf.format(BigDecimal.valueOf(value)); - String str1 = String.valueOf(value); - if (!str1.equals(dfString)) { - int count = mismatchCount.getAndIncrement(); - if (count < 5) { // limit lines of output - System.err.println("mismatch: str = " + str1 + " dfString = " + dfString); - } + String str = String.valueOf(value); + if (!str.equals(dfString)) { + mismatchCount.getAndIncrement(); + System.err.println("mismatch: str = " + str + " dfString = " + dfString); } } } catch (InterruptedException e) { From 55b6183d610fb08339726d70b28dc1c1aa09f9d5 Mon Sep 17 00:00:00 2001 From: Johannes Graham Date: Tue, 15 Apr 2025 17:44:04 -0400 Subject: [PATCH 06/10] throw on InterruptedException --- test/jdk/java/text/Format/DecimalFormat/CloneTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/jdk/java/text/Format/DecimalFormat/CloneTest.java b/test/jdk/java/text/Format/DecimalFormat/CloneTest.java index 29120118ec839..b1582b84a1926 100644 --- a/test/jdk/java/text/Format/DecimalFormat/CloneTest.java +++ b/test/jdk/java/text/Format/DecimalFormat/CloneTest.java @@ -54,7 +54,7 @@ public void testCloneIndependence() { // This initial use of the formatter initialises its internal state, which could // subsequently be shared across clones. This is key to reproducing this specific // issue. - String _ = df.format(Math.PI); // Initial use of formatter + String _ = df.format(Math.PI); try (var ex = Executors.newThreadPerTaskExecutor(Thread.ofPlatform().factory())) { for (int i = 0; i < 5; i++) { @@ -79,7 +79,7 @@ public void testCloneIndependence() { } } } catch (InterruptedException e) { - // just end + throw new RuntimeException(e); } }; ex.execute(task); From abda88ef7e9a0d4b3c44938d5381879029104fc3 Mon Sep 17 00:00:00 2001 From: Johannes Graham Date: Tue, 15 Apr 2025 18:17:32 -0400 Subject: [PATCH 07/10] break after mismatch --- test/jdk/java/text/Format/DecimalFormat/CloneTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/test/jdk/java/text/Format/DecimalFormat/CloneTest.java b/test/jdk/java/text/Format/DecimalFormat/CloneTest.java index b1582b84a1926..575d05b2cb802 100644 --- a/test/jdk/java/text/Format/DecimalFormat/CloneTest.java +++ b/test/jdk/java/text/Format/DecimalFormat/CloneTest.java @@ -76,6 +76,7 @@ public void testCloneIndependence() { if (!str.equals(dfString)) { mismatchCount.getAndIncrement(); System.err.println("mismatch: str = " + str + " dfString = " + dfString); + break; } } } catch (InterruptedException e) { From 87e652bef7a2f621d471beff772c0d576dbde197 Mon Sep 17 00:00:00 2001 From: Johannes Graham Date: Wed, 16 Apr 2025 11:53:51 -0400 Subject: [PATCH 08/10] reflective test --- .../text/Format/DecimalFormat/CloneTest.java | 68 ++++++++++++++++++- 1 file changed, 66 insertions(+), 2 deletions(-) diff --git a/test/jdk/java/text/Format/DecimalFormat/CloneTest.java b/test/jdk/java/text/Format/DecimalFormat/CloneTest.java index 575d05b2cb802..913153aad919e 100644 --- a/test/jdk/java/text/Format/DecimalFormat/CloneTest.java +++ b/test/jdk/java/text/Format/DecimalFormat/CloneTest.java @@ -25,26 +25,90 @@ * @test * @bug 8354522 * @summary Check for cloning interference - * @run junit CloneTest + * @run junit/othervm --add-opens=java.base/java.text=ALL-UNNAMED CloneTest */ +import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; +import java.lang.reflect.Field; import java.math.BigDecimal; import java.text.DecimalFormat; import java.util.concurrent.CountDownLatch; import java.util.concurrent.Executors; import java.util.concurrent.atomic.AtomicInteger; -import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.*; public class CloneTest { + private static Field DIGIT_LIST_FIELD; + private static Class DIGIT_LIST_CLASS; + + @BeforeAll + static void setup() throws Exception { + DIGIT_LIST_FIELD = DecimalFormat.class.getDeclaredField("digitList"); + DIGIT_LIST_FIELD.setAccessible(true); + + DecimalFormat df = new DecimalFormat(); + Object digitList = DIGIT_LIST_FIELD.get(df); + + DIGIT_LIST_CLASS = digitList.getClass(); + } // Tests that when DecimalFormat is cloned after use with // a long double/BigDecimal, clones will be independent. This is not an // exhaustive test. This tests for the issue of the same DigitList.data // array being reused across clones of DecimalFormat. + @Test + public void testClone() throws Exception { + DecimalFormat df = new DecimalFormat("#"); + assertCloneValidity(df); + } + + @Test + public void testCloneAfterInit() throws Exception { + DecimalFormat df = new DecimalFormat("#"); + + // This initial use of the formatter initialises its internal state, which could + // subsequently be shared across clones. This is key to reproducing this specific + // issue. + String _ = df.format(Math.PI); + assertCloneValidity(df); + } + + private static void assertCloneValidity(DecimalFormat df) throws Exception { + DecimalFormat dfClone = (DecimalFormat) df.clone(); + + Object digits = valFromDigitList(df, "digits"); + assertNotSame(digits, valFromDigitList(dfClone, "digits")); + + + Object data = valFromDigitList(df, "data"); + if (data != null) { + assertNotSame(data, valFromDigitList(dfClone, "data")); + } + + Object tempBuilder = valFromDigitList(df, "tempBuilder"); + if (tempBuilder != null) { + assertNotSame(data, valFromDigitList(dfClone, "data")); + } + + assertEquals(DIGIT_LIST_FIELD.get(df), DIGIT_LIST_FIELD.get(dfClone)); + } + + private static Object valFromDigitList(DecimalFormat df, String fieldName) { + try { + Object digitList = DIGIT_LIST_FIELD.get(df); + Field field = DIGIT_LIST_CLASS.getDeclaredField(fieldName); + field.setAccessible(true); + + return field.get(digitList); + } catch (ReflectiveOperationException e) { + throw new RuntimeException(e); + } + } + @Test public void testCloneIndependence() { AtomicInteger mismatchCount = new AtomicInteger(0); From 08a32e12687ef36f6c61e9a304a1d2b8c3927dd0 Mon Sep 17 00:00:00 2001 From: Johannes Graham Date: Wed, 16 Apr 2025 13:33:06 -0400 Subject: [PATCH 09/10] use SkippedException if reflective access fails --- .../text/Format/DecimalFormat/CloneTest.java | 99 +++++++++++-------- 1 file changed, 56 insertions(+), 43 deletions(-) diff --git a/test/jdk/java/text/Format/DecimalFormat/CloneTest.java b/test/jdk/java/text/Format/DecimalFormat/CloneTest.java index 913153aad919e..f7463cc7cc04a 100644 --- a/test/jdk/java/text/Format/DecimalFormat/CloneTest.java +++ b/test/jdk/java/text/Format/DecimalFormat/CloneTest.java @@ -25,10 +25,11 @@ * @test * @bug 8354522 * @summary Check for cloning interference + * @library /test/lib * @run junit/othervm --add-opens=java.base/java.text=ALL-UNNAMED CloneTest */ -import org.junit.jupiter.api.BeforeAll; +import jtreg.SkippedException; import org.junit.jupiter.api.Test; import java.lang.reflect.Field; @@ -41,74 +42,86 @@ import static org.junit.jupiter.api.Assertions.*; public class CloneTest { - private static Field DIGIT_LIST_FIELD; - private static Class DIGIT_LIST_CLASS; - - @BeforeAll - static void setup() throws Exception { - DIGIT_LIST_FIELD = DecimalFormat.class.getDeclaredField("digitList"); - DIGIT_LIST_FIELD.setAccessible(true); - - DecimalFormat df = new DecimalFormat(); - Object digitList = DIGIT_LIST_FIELD.get(df); - - DIGIT_LIST_CLASS = digitList.getClass(); - } - - // Tests that when DecimalFormat is cloned after use with - // a long double/BigDecimal, clones will be independent. This is not an - // exhaustive test. This tests for the issue of the same DigitList.data - // array being reused across clones of DecimalFormat. + // Note: this is a white-box test that may fail if the implementation is changed @Test - public void testClone() throws Exception { + public void testClone() { DecimalFormat df = new DecimalFormat("#"); - assertCloneValidity(df); + new CloneTester(df).testClone(); } + // Note: this is a white-box test that may fail if the implementation is changed @Test - public void testCloneAfterInit() throws Exception { + public void testCloneAfterInit() { DecimalFormat df = new DecimalFormat("#"); // This initial use of the formatter initialises its internal state, which could // subsequently be shared across clones. This is key to reproducing this specific // issue. String _ = df.format(Math.PI); - assertCloneValidity(df); + new CloneTester(df).testClone(); } - private static void assertCloneValidity(DecimalFormat df) throws Exception { - DecimalFormat dfClone = (DecimalFormat) df.clone(); + private static class CloneTester { + private final Field digitListField; + private final Class digitListClass; + private final DecimalFormat original; + + public CloneTester(DecimalFormat original) { + this.original = original; + try { + digitListField = DecimalFormat.class.getDeclaredField("digitList"); + digitListField.setAccessible(true); + + DecimalFormat df = new DecimalFormat(); + Object digitList = digitListField.get(df); + + digitListClass = digitList.getClass(); + } catch (NoSuchFieldException e) { + throw new RuntimeException(e); + } catch (ReflectiveOperationException e) { + throw new SkippedException("reflective access in white-box test failed", e); + } + } + + public void testClone() { + try { + DecimalFormat dfClone = (DecimalFormat) original.clone(); - Object digits = valFromDigitList(df, "digits"); - assertNotSame(digits, valFromDigitList(dfClone, "digits")); + Object digits = valFromDigitList(original, "digits"); + assertNotSame(digits, valFromDigitList(dfClone, "digits")); - Object data = valFromDigitList(df, "data"); - if (data != null) { - assertNotSame(data, valFromDigitList(dfClone, "data")); - } + Object data = valFromDigitList(original, "data"); + if (data != null) { + assertNotSame(data, valFromDigitList(dfClone, "data")); + } - Object tempBuilder = valFromDigitList(df, "tempBuilder"); - if (tempBuilder != null) { - assertNotSame(data, valFromDigitList(dfClone, "data")); - } + Object tempBuilder = valFromDigitList(original, "tempBuilder"); + if (tempBuilder != null) { + assertNotSame(data, valFromDigitList(dfClone, "data")); + } - assertEquals(DIGIT_LIST_FIELD.get(df), DIGIT_LIST_FIELD.get(dfClone)); - } + assertEquals(digitListField.get(original), digitListField.get(dfClone)); + } catch (ReflectiveOperationException e) { + throw new SkippedException("reflective access in white-box test failed", e); + } + } - private static Object valFromDigitList(DecimalFormat df, String fieldName) { - try { - Object digitList = DIGIT_LIST_FIELD.get(df); - Field field = DIGIT_LIST_CLASS.getDeclaredField(fieldName); + private Object valFromDigitList(DecimalFormat df, String fieldName) throws ReflectiveOperationException { + Object digitList = digitListField.get(df); + Field field = digitListClass.getDeclaredField(fieldName); field.setAccessible(true); return field.get(digitList); - } catch (ReflectiveOperationException e) { - throw new RuntimeException(e); } } + // Tests that when DecimalFormat is cloned after use with + // a long double/BigDecimal, clones will be independent. This is not an + // exhaustive test. This tests for the issue of the same DigitList.data + // array being reused across clones of DecimalFormat. + @Test public void testCloneIndependence() { AtomicInteger mismatchCount = new AtomicInteger(0); From 6c23e27eaabd878808f8524cfdc3dc8255e397b5 Mon Sep 17 00:00:00 2001 From: Johannes Graham Date: Wed, 16 Apr 2025 13:36:31 -0400 Subject: [PATCH 10/10] fix catch block --- test/jdk/java/text/Format/DecimalFormat/CloneTest.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/jdk/java/text/Format/DecimalFormat/CloneTest.java b/test/jdk/java/text/Format/DecimalFormat/CloneTest.java index f7463cc7cc04a..fa46b09d6ac0f 100644 --- a/test/jdk/java/text/Format/DecimalFormat/CloneTest.java +++ b/test/jdk/java/text/Format/DecimalFormat/CloneTest.java @@ -77,8 +77,6 @@ public CloneTester(DecimalFormat original) { Object digitList = digitListField.get(df); digitListClass = digitList.getClass(); - } catch (NoSuchFieldException e) { - throw new RuntimeException(e); } catch (ReflectiveOperationException e) { throw new SkippedException("reflective access in white-box test failed", e); }