From 94afde20803b2d7352ff737d71172bffe8bc3a0c Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 11 Sep 2026 15:46:04 +0200 Subject: [PATCH] WW-5724 fix(core): hand out a clone of the cached MessageFormat AbstractLocalizedTextProvider caches MessageFormat instances by pattern and locale and returned the cached instance itself from buildMessageFormat. MessageFormat is not thread-safe, so concurrent callers rendering the same message formatted through one shared instance; with a date or time sub-format that produced output for the wrong argument or an exception. buildMessageFormat now returns a clone of the cached instance, so the cached entry is only ever a template and is never formatted directly. MessageFormat.clone() deep-copies the sub-formats. The pattern-parsing cache is kept; measured against a fresh instance per call and against synchronizing on the shared instance, cloning is the cheapest of the three. Backport of the main change (#1914) to the 6.x line. Co-Authored-By: Claude Opus 5 (1M context) --- .../util/AbstractLocalizedTextProvider.java | 3 +- .../util/StrutsLocalizedTextProviderTest.java | 60 +++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/com/opensymphony/xwork2/util/AbstractLocalizedTextProvider.java b/core/src/main/java/com/opensymphony/xwork2/util/AbstractLocalizedTextProvider.java index 6b972d9230..6571529dc8 100644 --- a/core/src/main/java/com/opensymphony/xwork2/util/AbstractLocalizedTextProvider.java +++ b/core/src/main/java/com/opensymphony/xwork2/util/AbstractLocalizedTextProvider.java @@ -452,7 +452,8 @@ protected MessageFormat buildMessageFormat(String pattern, Locale locale) { messageFormats.put(key, format); } - return format; + // MessageFormat is not thread-safe; the cached instance is a template that is never formatted directly + return (MessageFormat) format.clone(); } protected String formatWithNullDetection(MessageFormat mf, Object[] args) { diff --git a/core/src/test/java/com/opensymphony/xwork2/util/StrutsLocalizedTextProviderTest.java b/core/src/test/java/com/opensymphony/xwork2/util/StrutsLocalizedTextProviderTest.java index dea14bec8f..bd7454934d 100644 --- a/core/src/test/java/com/opensymphony/xwork2/util/StrutsLocalizedTextProviderTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/util/StrutsLocalizedTextProviderTest.java @@ -40,11 +40,15 @@ import java.io.ObjectOutputStream; import java.lang.reflect.Field; import java.text.DateFormat; +import java.text.MessageFormat; import java.text.ParseException; import java.util.Date; import java.util.HashMap; import java.util.Locale; import java.util.ResourceBundle; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; /** @@ -273,6 +277,62 @@ public void testXW404() { * * @since 6.0.0 */ + public void testBuildMessageFormatDoesNotHandOutTheCachedInstance() { + TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); + + MessageFormat first = provider.buildMessageFormat("{0,date,short}", Locale.US); + MessageFormat second = provider.buildMessageFormat("{0,date,short}", Locale.US); + + assertNotSame("cached MessageFormat shared between callers ?", first, second); + assertEquals("pattern not cached once ?", 1, provider.messageFormatsSize()); + } + + public void testConcurrentDateFormattingDoesNotMixArguments() throws Exception { + localizedTextProvider.addDefaultResourceBundle("com/opensymphony/xwork2/util/LocalizedTextUtilTest"); + Date[] dates = { + DateFormat.getDateInstance(DateFormat.SHORT, Locale.US).parse("01/01/2015"), + DateFormat.getDateInstance(DateFormat.SHORT, Locale.US).parse("02/02/2020"), + }; + String[] expected = {"1/1/15", "2/2/20"}; + assertEquals(expected[0], localizedTextProvider.findDefaultText("test.format.date", Locale.US, new Object[]{dates[0]})); + assertEquals(expected[1], localizedTextProvider.findDefaultText("test.format.date", Locale.US, new Object[]{dates[1]})); + + int threads = 4; + int iterations = 20000; + AtomicInteger wrong = new AtomicInteger(); + AtomicReference sample = new AtomicReference<>(); + AtomicReference thrown = new AtomicReference<>(); + CountDownLatch start = new CountDownLatch(1); + Thread[] workers = new Thread[threads]; + for (int t = 0; t < threads; t++) { + final int which = t % 2; + workers[t] = new Thread(() -> { + try { + start.await(); + for (int i = 0; i < iterations; i++) { + String out = localizedTextProvider.findDefaultText("test.format.date", Locale.US, new Object[]{dates[which]}); + if (!expected[which].equals(out)) { + wrong.incrementAndGet(); + sample.compareAndSet(null, "expected <" + expected[which] + "> but was <" + out + ">"); + } + } + } catch (Throwable e) { + thrown.compareAndSet(null, e); + } + }); + } + for (Thread worker : workers) { + worker.start(); + } + start.countDown(); + for (Thread worker : workers) { + worker.join(); + } + + assertNull("formatting threw under concurrency ?", thrown.get()); + assertEquals("another caller's argument rendered: " + sample.get(), 0, wrong.get()); + } + public void testLocalizedTextProviderClearingMethods() { TestStrutsLocalizedTextProvider testStrutsLocalizedTextProvider = new TestStrutsLocalizedTextProvider(); assertTrue("testStrutsLocalizedTextProvider not instance of AbstractLocalizedTextProvider ?",