This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 11.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit b406951969319c16ad347a6f12fb831ef2da4167 Author: opencode <[email protected]> AuthorDate: Tue Nov 3 13:34:45 2026 +0100 Harden and document the pool's message factory Three fixes and the documentation of the convention behind them. A malformed message could take down the pool. The formatting path built and applied the MessageFormat with no guard, so an IllegalArgumentException escaped into whatever call site was formatting a message - nearly all of them inline in throw statements and log lines. A pattern MessageFormat cannot parse (a translation quoting JSON, say) or cannot apply (a translation restyling a placeholder as {0,number} over a String argument) replaced the pool's own diagnostic - a PoolExhaustedException from getConnection(), for example - with an unrelated unchecked exception, bypassing the caller's SQLException handling. The path now degrades to the unformatted pattern, as it already degraded a missing key to the key. The exception thrown for calling connect() on a released connection showed the doubled apostrophe of a MessageFormat quote escape - can''t - a regression from moving the messages into the bundle: the entry was authored with escapes, like the keys fetched with arguments, but its only call site fetches through the no-argument lookup, which hands the text out raw. The key has a fixed, argument-free arity, so the bundle now carries a single literal apostrophe. The Number-to-String conversion that keeps MessageFormat from grouping numeric arguments by locale wrote its Strings back into the array it was given, silently changing the types in an Object[] a caller reused across messages. The conversion now happens in a copy, cloned on the first write. Both fixes share a root the source never stated: each fetch path demands a different form of bundle text. The class javadoc now states that a key fetched without arguments must be written literally while a key fetched with arguments follows the MessageFormat conventions, alongside the bundle's resolve-once locale semantics; the same note heads the bundle file, and a header entry records the released-connection fix. New TestMessageFactory pins the guard (unparseable brace, index overflow, unappliable number format), the raw and formatted paths, the missing-key fallback, the untouched identity of a caller's array and the rendered text of the zero-argument key. --- .../tomcat/jdbc/pool/util/LocalStrings.properties | 8 +- .../tomcat/jdbc/pool/util/MessageFactory.java | 58 ++++++--- .../tomcat/jdbc/pool/util/TestMessageFactory.java | 137 +++++++++++++++++++++ 3 files changed, 185 insertions(+), 18 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties index 299de84097..c2ff5bd634 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties @@ -13,6 +13,12 @@ # See the License for the specific language governing permissions and # limitations under the License. +# Message text must match how its key is fetched: text fetched through the +# argument-free MessageFactory.get(key) is handed out raw, so it must be +# written literally (a single ', braces left as they are), while text fetched +# with arguments goes through MessageFormat, which reads '' as a literal ' +# and treats {n} as a placeholder. + abstractCreateStatementInterceptor.closeInvokedFailed=Interceptor [{0}] threw an exception from closeInvoked(); the exception is logged and ignored, the rest of the chain still receives the close, and the connection is returned to the pool as it would have been. abstractCreateStatementInterceptor.constructorCacheIndexPopulated=Constructor cache index {0} is already populated for {1}, but {2} was requested. Every use of an index must pass an interface the cached proxy implements. abstractCreateStatementInterceptor.unableToCreateConstructorCacheEntry=Unable to create the constructor cache entry for index {0} ({1}). @@ -155,7 +161,7 @@ poolProperties.validatorClassNotFound=The class {0} cannot be found. poolProperties.validatorInaccessible=The class {0} or its no-arg constructor are inaccessible. poolProperties.validatorNotValidator=The class {0} does not implement the Validator interface. -pooledConnection.cannotReestablish=A connection once released, can''t be reestablished. +pooledConnection.cannotReestablish=A connection once released, can't be reestablished. pooledConnection.commitFailed=Failed to commit transaction pooledConnection.connectionPoolDataSourceNullConnection=ConnectionPoolDataSource {0} returned a null connection for the pooled connection handle pooledConnection.customValidationFailed=Custom validation through {0} failed. diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/MessageFactory.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/MessageFactory.java index 90520db49d..5851f1ac93 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/MessageFactory.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/MessageFactory.java @@ -24,6 +24,15 @@ import java.util.ResourceBundle; /** * Factory for creating localized JDBC pool messages. + * <p> + * The messages are read from the {@code LocalStrings} resource bundle of this + * package, which the JVM resolves once, under the default locale in effect at + * class initialization. A message is handed out unchanged by + * {@link #get(String)} and passed through {@link java.text.MessageFormat} by + * {@link #get(String, Object...)}, so bundle text must match how its key is + * fetched: a key fetched without arguments is written literally, while a key + * fetched with arguments follows the MessageFormat conventions, where a + * literal single quote is written {@code ''}. */ public final class MessageFactory { @@ -33,7 +42,8 @@ public final class MessageFactory { private static final MessageFactory DEFAULT_MESSAGE_FACTORY = new MessageFactory(DEFAULT_BUNDLE); /** - * Get a localized message. + * Get a localized message. The message is returned exactly as written in + * the bundle, with no argument substitution and no MessageFormat applied. * * @param key the message key * @@ -44,7 +54,9 @@ public final class MessageFactory { } /** - * Get a localized message with arguments. + * Get a localized message with arguments substituted by MessageFormat. + * The argument array is never modified. A pattern that MessageFormat + * cannot apply is returned unformatted rather than reported as a failure. * * @param key the message key * @param args the arguments @@ -92,25 +104,37 @@ public final class MessageFactory { protected String getInternal(final String key, final Object... args) { String value = getInternal(key); - MessageFormat mf = new MessageFormat(value); - Format[] formats = null; + try { + MessageFormat mf = new MessageFormat(value); + Format[] formats = null; - // Unless an argument has been explicitly configured to use a number - // format, convert all Number arguments to String else MessageFormat may - // try to format them in unexpected ways. - if (args != null) { - for (int i = 0; i < args.length; i++) { - if (args[i] instanceof Number) { - if (formats == null) { - formats = mf.getFormatsByArgumentIndex(); - } - if (i < formats.length && !(formats[i] instanceof NumberFormat)) { - args[i] = args[i].toString(); + // Convert Number arguments to String unless the pattern asks for a + // number format at that index, so MessageFormat cannot group them + // by locale. Work on a copy: the array belongs to the caller. + Object[] formatted = args; + if (args != null) { + for (int i = 0; i < args.length; i++) { + if (args[i] instanceof Number) { + if (formats == null) { + formats = mf.getFormatsByArgumentIndex(); + } + if (i < formats.length && !(formats[i] instanceof NumberFormat)) { + if (formatted == args) { + formatted = args.clone(); + } + formatted[i] = args[i].toString(); + } } } } - } - return mf.format(args, new StringBuffer(), null).toString(); + return mf.format(formatted, new StringBuffer(), null).toString(); + } catch (IllegalArgumentException e) { + // A pattern MessageFormat cannot parse or apply - reachable only + // through a malformed, typically translated, message - degrades to + // the raw pattern rather than taking down the caller, which at + // these sites is building an exception or a log line. + return value; + } } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/util/TestMessageFactory.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/util/TestMessageFactory.java new file mode 100644 index 0000000000..9b82ec7cbc --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/util/TestMessageFactory.java @@ -0,0 +1,137 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.jdbc.pool.util; + +import java.util.ListResourceBundle; +import java.util.MissingResourceException; +import java.util.ResourceBundle; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +/** + * Unit tests for {@link MessageFactory}. The factory is used inline to build + * exceptions and log lines across the pool, so a message that MessageFormat + * cannot parse or cannot apply - possible only through a malformed pattern, + * typically a translation - must degrade to the raw pattern rather than throw. + */ +class TestMessageFactory { + + /** + * A bundle standing for a translation: patterns that MessageFormat's + * pattern parser rejects, plus one the formatter itself rejects at + * application time, alongside a well-formed control case. + */ + private static final class PatternBundle extends ListResourceBundle { + + @Override + protected Object[][] getContents() { + return new Object[][] { + // A brace opened before the argument index digit: applyPattern + // fails to parse the index. + { "brace", "Setting {\"a\":1} was rejected for {0}" }, + // An argument index beyond what the pattern parser accepts. + { "hugeIndex", "Value {2147483648} is out of range" }, + // A pattern the parser accepts but the formatter cannot apply + // to a String argument. + { "numberFormat", "Count is {0,number}" }, + // The control case: a well-formed pattern. + { "ok", "Count is {0} of {1}." }, + }; + } + } + + private static MessageFactory factoryFor(ResourceBundle bundle) { + return new MessageFactory(bundle); + } + + @Test + void unparseablePatternFallsBackToRawPattern() { + MessageFactory factory = factoryFor(new PatternBundle()); + String pattern = "Setting {\"a\":1} was rejected for {0}"; + // Before the guard, this escaped as IllegalArgumentException from + // new MessageFormat(pattern) into the caller's exception path. + Assertions.assertEquals(pattern, factory.getInternal("brace", "value")); + } + + @Test + void argumentIndexOverflowFallsBackToRawPattern() { + MessageFactory factory = factoryFor(new PatternBundle()); + String pattern = "Value {2147483648} is out of range"; + Assertions.assertEquals(pattern, factory.getInternal("hugeIndex", "x")); + } + + @Test + void unappliableFormatFallsBackToRawPattern() { + MessageFactory factory = factoryFor(new PatternBundle()); + String pattern = "Count is {0,number}"; + // {0,number} applied to a String throws from format(), not from + // pattern parsing. + Assertions.assertEquals(pattern, factory.getInternal("numberFormat", "abc")); + // The same pattern still formats a Number argument normally. + Assertions.assertEquals("Count is 5", factory.getInternal("numberFormat", Integer.valueOf(5))); + } + + @Test + void wellFormedPatternStillFormats() { + MessageFactory factory = factoryFor(new PatternBundle()); + Assertions.assertEquals("Count is 7 of 10.", factory.getInternal("ok", Integer.valueOf(7), Integer.valueOf(10))); + } + + @Test + void callerSuppliedArgumentArrayIsNotMutated() { + MessageFactory factory = factoryFor(new PatternBundle()); + Integer seven = Integer.valueOf(7); + Integer ten = Integer.valueOf(10); + Object[] caller = { seven, ten }; + // The Number-to-String conversion must not be written back into the + // array the caller passed in. + Assertions.assertEquals("Count is 7 of 10.", factory.getInternal("ok", caller)); + Assertions.assertSame(seven, caller[0]); + Assertions.assertSame(ten, caller[1]); + // Reusing the same array keeps formatting identically. + Assertions.assertEquals("Count is 7 of 10.", factory.getInternal("ok", caller)); + } + + @Test + void missingKeyDegradesToTheKey() { + MessageFactory factory = factoryFor(new PatternBundle()); + Assertions.assertEquals("no.such.key", factory.getInternal("no.such.key")); + // The formatted path reuses the same fallback and cannot throw on it. + Assertions.assertEquals("no.such.key", factory.getInternal("no.such.key", "x", 1)); + } + + @Test + void realBundleResolvesEveryPoolKey() { + // The shipped bundle must contain the keys the no-arg and formatted + // paths promise to render, not fall back to the key text. + String message = MessageFactory.get("pooledConnection.cannotReestablish"); + Assertions.assertNotEquals("pooledConnection.cannotReestablish", message); + Assertions.assertThrows(MissingResourceException.class, + () -> ResourceBundle.getBundle("org.apache.tomcat.jdbc.pool.util.LocalStrings") + .getString("this.key.does.not.exist")); + } + + @Test + void zeroArgMessageCarriesNoQuoteEscapes() { + // A message fetched through the no-arg overload is handed out raw, so + // MessageFormat quote escapes in it reach the application verbatim. + // This key is only ever fetched without arguments (regression pin). + Assertions.assertEquals("A connection once released, can't be reestablished.", + MessageFactory.get("pooledConnection.cannotReestablish")); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
