This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit ef66ae79481dc87854aa48218718f6a3b34e6524 Author: opencode <[email protected]> AuthorDate: Mon Oct 26 22:06:11 2026 +0100 Never let query reporting throw at the application The statement proxy installed by the query report interceptors guarded its reportSlowQuery call against exceptions thrown by the reporting code, but the sibling reportQuery and reportFailedQuery calls were unguarded. Those hooks are protected extension points of a public API, and an override that throws previously escaped to the application: a successful execution suddenly threw from the reportQuery call, and a failing one threw the reporting failure from inside the catch block, replacing the driver's SQLException the application was catching. Give both call sites the same barrier the slow-query path already had, catch Throwable at all three sites to match the module's interceptor reset()/disconnected() convention, and name the throwing reporting interceptor in the warning that is logged instead. The application now only ever sees the result or the exception of its own JDBC call. Add regression tests using an interceptor whose report hooks throw, with positive controls proving the hooks ran, and note that the built-in reporters cannot deliver an exception through this path: the reporting failure is reachable from third-party subclasses. --- .../jdbc/pool/interceptor/AbstractQueryReport.java | 26 ++++++-- .../pool/interceptor/TestInterceptorSemantics.java | 76 ++++++++++++++++++++++ 2 files changed, 98 insertions(+), 4 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/AbstractQueryReport.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/AbstractQueryReport.java index 9a167920cc..c456439b79 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/AbstractQueryReport.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/AbstractQueryReport.java @@ -249,7 +249,16 @@ public abstract class AbstractQueryReport extends AbstractCreateStatementInterce //reporting it would replace the exception that the //application is expecting with a reporting failure if (process) { - reportFailedQuery(query,args,name,start,t); + try { + reportFailedQuery(query,args,name,start,t); + }catch (Throwable reporting) { + //reporting must never disturb the application - + //the driver's exception is the one being thrown + if (log.isWarnEnabled()) { + log.warn("Unable to process failed query for " + + AbstractQueryReport.this.getClass().getName(),reporting); + } + } } if (t instanceof InvocationTargetException && t.getCause() != null) { @@ -265,13 +274,22 @@ public abstract class AbstractQueryReport extends AbstractCreateStatementInterce try { //report the slow query reportSlowQuery(query, args, name, start, delta); - }catch (Exception t) { + }catch (Throwable t) { if (log.isWarnEnabled()) { - log.warn("Unable to process slow query",t); + log.warn("Unable to process slow query for " + + AbstractQueryReport.this.getClass().getName(),t); } } } else if (process) { - reportQuery(query, args, name, start, delta); + try { + reportQuery(query, args, name, start, delta); + }catch (Throwable t) { + //reporting must never disturb the application + if (log.isWarnEnabled()) { + log.warn("Unable to process query report for " + + AbstractQueryReport.this.getClass().getName(),t); + } + } } //perform close cleanup if (close) { diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestInterceptorSemantics.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestInterceptorSemantics.java index 54729b2531..9657a80a7f 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestInterceptorSemantics.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestInterceptorSemantics.java @@ -309,4 +309,80 @@ class TestInterceptorSemantics extends JdbcPoolBaseTest { Assertions.assertEquals(0, adminRowCount(t), "execution attempted after close must not reach the database"); } + + /* + * Reporting must never disturb the application: the report hooks are + * extension points, and a subclass whose reportQuery throws used to + * turn a successful execution into a thrown exception at the + * application (reportFailedQuery threw from inside the catch block, + * replacing the driver's SQLException). + */ + @Test + void throwingSuccessReporterDoesNotAffectExecution() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setName("throw_" + System.nanoTime()); + ds.getPoolProperties().setJdbcInterceptors( + ThrowingReportInterceptor.class.getName() + "(threshold=60000)"); + ds.createPool(); + String poolName = ds.getPoolProperties().getPoolName(); + String t = uniqueTable(); + String sql = "CREATE TABLE " + t + " (v INT)"; + try (Connection c = ds.getConnection(); Statement st = c.createStatement()) { + Assertions.assertDoesNotThrow(() -> st.execute(sql), + "a throwing reporter must not disturb the application"); + } + // positive control: the hook ran and booked the execution before + // throwing, so the assertion above cannot pass because the proxy + // is absent + ConcurrentHashMap<String, ?> stats = SlowQueryReport.getPoolStats(poolName); + Assertions.assertEquals(1, statsFor(stats, sql).getNrOfInvocations(), + "the reporting hook must have run before it threw"); + } + + /* + * Same for the failure path: the driver's SQLException, not the + * reporting failure, is what the application catches. + */ + @Test + void throwingFailedReporterDoesNotMaskDriverException() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setName("throwfail_" + System.nanoTime()); + ds.getPoolProperties().setJdbcInterceptors( + ThrowingReportInterceptor.class.getName() + "(threshold=60000)"); + ds.createPool(); + String poolName = ds.getPoolProperties().getPoolName(); + String nonce = "throwfail_" + System.nanoTime(); + String sql = "SELECT * FROM no_such_table_" + nonce; + try (Connection c = ds.getConnection(); Statement st = c.createStatement()) { + Assertions.assertThrows(SQLException.class, () -> st.execute(sql), + "the driver's SQLException must reach the application " + + "even if the reporter throws"); + } + // positive control as above: the failed-query hook ran and booked + // the failure before throwing + ConcurrentHashMap<String, ?> stats = SlowQueryReport.getPoolStats(poolName); + Assertions.assertEquals(1, statsFor(stats, sql).getNrOfInvocations(), + "the failed-query reporting hook must have run before it threw"); + } + + /** + * Interceptor that always throws from its report hooks, after having + * performed the regular booking, simulating a subclass that fails + * unexpectedly. + */ + public static class ThrowingReportInterceptor extends SlowQueryReport { + @Override + protected String reportQuery(String query, Object[] args, + String name, long start, long delta) { + super.reportQuery(query, args, name, start, delta); + throw new IllegalStateException("reporter intentionally fails"); + } + + @Override + protected String reportFailedQuery(String query, Object[] args, + String name, long start, Throwable t) { + super.reportFailedQuery(query, args, name, start, t); + throw new IllegalStateException("reporter intentionally fails"); + } + } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
