This is an automated email from the ASF dual-hosted git repository. smolnar82 pushed a commit to branch knox_idf in repository https://gitbox.apache.org/repos/asf/knox.git
commit 2abbf2a839ac332262d37989e6497c161bc80c10 Author: Sandor Molnar <[email protected]> AuthorDate: Tue Aug 11 23:31:41 2026 +0200 KNOX-3414: fix broken double-checked locking in JdbcTrustedOidcIssuerService.init (review finding M5) init() tested initialized only at the outer (pre-lock) check and was missing the inner re-check under the lock that JdbcFederatedIdentityService has. A startup race where two threads both observed initialized==false could therefore both enter the critical section sequentially and initialize twice, the second run overwriting the already-built database and discoveryHelper references. Add the inner if (!initialized.get()) recheck inside the lock so a thread that blocked while another was initializing becomes a no-op. Mirrors the sibling JdbcFederatedIdentityService. Covered by JdbcTrustedOidcIssuerServiceTest#testConcurrentInitDoesNotReinitialize, which deterministically drives the race via the service's own init lock queue and asserts the database reference is not rebuilt once initialized. Co-Authored-By: Claude Opus 4.8 <[email protected]> --- .../JdbcTrustedOidcIssuerService.java | 36 ++++++++------ .../JdbcTrustedOidcIssuerServiceTest.java | 56 ++++++++++++++++++++++ 2 files changed, 77 insertions(+), 15 deletions(-) diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/trustedoidcissuer/JdbcTrustedOidcIssuerService.java b/gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/trustedoidcissuer/JdbcTrustedOidcIssuerService.java index e72ed56b6..3f345fe57 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/trustedoidcissuer/JdbcTrustedOidcIssuerService.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/trustedoidcissuer/JdbcTrustedOidcIssuerService.java @@ -70,21 +70,27 @@ public class JdbcTrustedOidcIssuerService implements TrustedOidcIssuerService { if (!initialized.get()) { initLock.lock(); try { - if (aliasService == null) { - throw new ServiceLifecycleException("The required AliasService reference has not been set."); - } - try { - this.maxTrustedIssuers = config.getTrustedOidcIssuerMaxTrustedIssuers(); - this.database = new TrustedOidcIssuerDatabase( - DataSourceProvider.getDataSource(config, aliasService), config.getDatabaseType()); - this.discoveryHelper = new OIDCDiscoveryHelper(this, config.getTrustedOidcIssuerDiscoveryCacheTtlSecs(), - OIDCDiscoveryHelper.buildHttpClient(config.getTrustedOidcIssuerDiscoveryConnectTimeoutMs(), config.getTrustedOidcIssuerDiscoveryReadTimeoutMs())); - reloadRegistrySnapshot(); - initialized.set(true); - } catch (ServiceLifecycleException e) { - throw e; - } catch (Exception e) { - throw new ServiceLifecycleException("Error initializing JdbcTrustedOidcIssuerService: " + e, e); + // Double-checked locking: re-test under the lock so a thread that blocked while another was + // initialising does not re-initialise the database/discoveryHelper a second time (mirrors + // JdbcFederatedIdentityService). Without this inner check a startup race overwrote the + // already-built database and discoveryHelper references. + if (!initialized.get()) { + if (aliasService == null) { + throw new ServiceLifecycleException("The required AliasService reference has not been set."); + } + try { + this.maxTrustedIssuers = config.getTrustedOidcIssuerMaxTrustedIssuers(); + this.database = new TrustedOidcIssuerDatabase( + DataSourceProvider.getDataSource(config, aliasService), config.getDatabaseType()); + this.discoveryHelper = new OIDCDiscoveryHelper(this, config.getTrustedOidcIssuerDiscoveryCacheTtlSecs(), + OIDCDiscoveryHelper.buildHttpClient(config.getTrustedOidcIssuerDiscoveryConnectTimeoutMs(), config.getTrustedOidcIssuerDiscoveryReadTimeoutMs())); + reloadRegistrySnapshot(); + initialized.set(true); + } catch (ServiceLifecycleException e) { + throw e; + } catch (Exception e) { + throw new ServiceLifecycleException("Error initializing JdbcTrustedOidcIssuerService: " + e, e); + } } } finally { initLock.unlock(); diff --git a/gateway-server/src/test/java/org/apache/knox/gateway/services/knoxidf/trustedoidcissuer/JdbcTrustedOidcIssuerServiceTest.java b/gateway-server/src/test/java/org/apache/knox/gateway/services/knoxidf/trustedoidcissuer/JdbcTrustedOidcIssuerServiceTest.java index e74e476cf..5e8aa1cc8 100644 --- a/gateway-server/src/test/java/org/apache/knox/gateway/services/knoxidf/trustedoidcissuer/JdbcTrustedOidcIssuerServiceTest.java +++ b/gateway-server/src/test/java/org/apache/knox/gateway/services/knoxidf/trustedoidcissuer/JdbcTrustedOidcIssuerServiceTest.java @@ -34,10 +34,13 @@ import java.sql.PreparedStatement; import java.sql.SQLException; import java.time.Instant; import java.util.List; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.locks.ReentrantLock; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertSame; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; @@ -328,6 +331,59 @@ public class JdbcTrustedOidcIssuerServiceTest { noAliasService.init(gatewayConfig, null); } + /** + * Review finding M5: init() must re-check {@code initialized} inside the lock so a thread that + * blocked while another was initialising does not re-initialise (overwriting the already-built + * database/discoveryHelper). This deterministically reproduces the race window: the test thread + * holds the init lock and marks the service initialised (as a "winning" thread would) while a + * second init() call is blocked entering the critical section; when it proceeds, the inner recheck + * must make it a no-op and leave the sentinel database reference untouched. + */ + @Test + public void testConcurrentInitDoesNotReinitialize() throws Exception { + final ReentrantLock initLock = (ReentrantLock) FieldUtils.readField(service, "initLock", true); + final AtomicBoolean initialized = (AtomicBoolean) FieldUtils.readField(service, "initialized", true); + + // A sentinel that the losing init() must NOT overwrite if the inner recheck is present. + final TrustedOidcIssuerDatabase sentinel = EasyMock.createNiceMock(TrustedOidcIssuerDatabase.class); + FieldUtils.writeField(service, "database", sentinel, true); + + // Simulate the pre-init state a second racing thread would have observed at the outer check. + initialized.set(false); + + // Hold the lock first, then start the racing init(): it passes the outer !initialized check and + // blocks entering the critical section until we release the lock. + initLock.lock(); + final AtomicBoolean workerFailed = new AtomicBoolean(false); + final Thread worker = new Thread(() -> { + try { + service.init(gatewayConfig, null); + } catch (Exception e) { + workerFailed.set(true); + } + }); + try { + worker.start(); + // Wait until the worker is actually blocked in the lock queue, i.e. it entered init() while + // initialized was still false (the exact race the inner recheck must defend against). + final long deadline = System.currentTimeMillis() + 10_000; + while (!initLock.hasQueuedThreads() && System.currentTimeMillis() < deadline) { + Thread.yield(); + } + assertTrue("Worker should be blocked entering the init critical section", initLock.hasQueuedThreads()); + // Mimic the winning thread finishing initialisation while we hold the lock. + initialized.set(true); + } finally { + initLock.unlock(); + } + worker.join(10_000); + + assertFalse("Racing init() must not have thrown", workerFailed.get()); + assertFalse("Worker thread must have finished", worker.isAlive()); + assertSame("A second init() must not rebuild the database once initialised (inner recheck)", + sentinel, FieldUtils.readField(service, "database", true)); + } + // ------------------------------------------------------------------ // Helpers // ------------------------------------------------------------------
