Fix the error-level CodeQL findings: dead branches, a null factory, interned-string locks, racy lazy init

[vharseko]

Closes the remaining 10 open CodeQL alerts of severity error that carry no security rating: java/contradictory-type-checks #1536 #1537, java/dereferenced-value-is-always-null #1560, java/sync-on-boxed-types #1554 #1555 #1556, java/unsafe-double-checked-locking #1550 #1551, java/unsynchronized-getter #1544 #1545. The eleventh, #1549 (ScriptedConfiguration), was closed by #133.

Fixes #148
Fixes #149

Where	Finding	Change
SQLUtil.setParam	else if (val instanceof Integer) appears twice; the second branch can never run	second branch removed
ContractITCase.createInstances	IObjectFactory objectFactory = null is dereferenced for any test class with a (String) constructor. None of the default classes has one today, so the code only worked because the NoSuchMethodException path was always taken	the (String) constructor is called through plain reflection, so the method no longer depends on TestNG's internal ObjectFactoryImpl, which TestNG 7.5 moved and whose IObjectFactory 7.10.2 removed. The unused nested ContractTestFactory with its three never-read fields is gone
BatchRemoteCache (testbundlev1)	synchronized (resultLock) on the interned literal "resultLock", a monitor shared with any other code in the JVM that synchronises on the same string	new Object()
WebSocketConnectionGroup.operationContext, TstStatefulConnectorConfig.executorService	double-checked lazy initialisation reads a non-volatile field outside the lock, so a thread may see a partially constructed object	fields declared volatile, which makes the pattern correct under the JMM
ScriptedConfiguration.getGroovyScriptEngine()	#133 made the getter synchronized for the whole initialisation. That closed the double-checked-locking alert and #149 (the getter and release() share the monitor now), and concurrent callers wait for the customizer. But the getter still stored the engine before its customizer ran, so a failed customizer left an uncustomized engine in place for good (#148)	on top of #133's synchronised getter, the engine is published only after initializeCustomizer() succeeds. The customizer's own re-entrant calls get the engine under construction from a second lock-guarded field. If the customizer fails, nothing is published and the next call retries; the failed customizer class is removed with InvokerHelper.removeClass, so the metaclass the REST, CREST and SSH configurations register on it does not keep one class loader alive per retry. The customizer runs under the configuration's monitor, so it must not wait for another thread that uses the engine; the comment on initializeCustomizer() says so
FrameworkUtil.getFrameworkVersion()	unsynchronised lazy init while setFrameworkVersion is synchronised	static synchronized (not a hot path). The (ClassLoader) overload reads connectors-framework.properties rather than the field and is only used by FrameworkUtilTests, so it is renamed to readFrameworkVersion and the getter/setter pairing no longer applies to it

ScriptedConfigurationTest covers #148: a customizer that fails once is retried on the next call, a second thread blocks until the customizer has finished, and a failed customizer class has no metaclass left registered. Each test fails when its part of the fix is reverted (engine published before the customizer, no removeClass, getter not synchronised). #149 is fixed on master by #133; it was a window between two adjacent field reads that no test can hold open, so it has no test. The other changes have no behaviour a unit test can observe: dead branches, volatile, a private monitor and a synchronised getter. createInstances takes its classes from a fixed list with no (String) constructors, so its reflective path cannot be reached from a test.

Local reactor run (rebased on current master, after #133 #134 #136 #141 #143) of all touched modules and everything they depend on: connector-framework 197, dbcommon 86, framework-internal 509 (2 skipped), contract 43, connector-framework-server 32, groovy-connector 129 (42 skipped as on master, 3 new), ssh-connector 96 (82 skipped), testbundlev1 compiles — all green.

[maximthomas]

praise: Each fix is at the line the CodeQL alert flags, and none goes further than the alert needs.

• BatchRemoteCache.resultLock is now a private new Object() instead of the interned "resultLock" literal.
• FrameworkUtil.getFrameworkVersion() is static synchronized, which pairs it with the already synchronized setFrameworkVersion. The (ClassLoader) overload is renamed to readFrameworkVersion, and its only callers are the two in FrameworkUtilTests.

---

question (non-blocking): Should connector-framework-contract keep working for connectors that pin TestNG 7.5 or later? ContractITCase.createInstances now instantiates the TestNG-internal org.testng.internal.ObjectFactoryImpl on every call.

OpenICF-java-framework/connector-framework-contract/src/main/java/org/identityconnectors/contract/test/ContractITCase.java:72, :39

At the base, ObjectFactoryImpl was referenced only from the nested ContractTestFactory, which nothing ever instantiated, so the class was never loaded. Line 72 now runs before the loop on every @Factory call. TestNG 7.5 moved the class to org.testng.internal.objects, and 7.10.2 removed org.testng.IObjectFactory as well. I compiled copies of the base and head versions of the method and ran them on TestNG 6.9.10, 7.5, 7.6.0 and 7.10.2. The base version took its fallback on all four. The head version threw NoClassDefFoundError on the three 7.x versions. The factory is still never used, because no class in DEFAULT_TEST_CLASSES has a public (String) constructor. No build in this repo runs ContractITCase, so the break only shows up in downstream connectors that override TestNG. If those connectors are meant to be supported, this should block the merge; if not, it can wait. Plain reflection does the same job without the internal class:

for (Class<?> testClass: getContractTestClasses(context)) {
    try {
        Object test = testClass.getConstructor(String.class).newInstance("");
        injector.injectMembers(test);
        result.add(test);
    } catch (NoSuchMethodException e) {
        result.add(injector.getInstance(testClass));
    } catch (ReflectiveOperationException e) {
        throw new IllegalStateException("Cannot instantiate " + testClass.getName(), e);
    }
}

Then the IObjectFactory and ObjectFactoryImpl imports can go.

---

issue (non-blocking): getGroovyScriptEngine() stores the engine in the field before initializeCustomizer() runs. If the customizer fails, the engine stays in the field without its customizations.

OpenICF-groovy-connector/src/main/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfiguration.java:797-800, :820-822

The field has to be set first, because getCustomizerClass() (:727) calls the getter again. But when the customizer script throws, initializeCustomizer() only wraps the exception and rethrows it; it never clears the field. From then on every caller takes the fast path at :786 and runs scripts on an engine whose customizer never ran. For example, ScriptedRESTConfiguration.getHttpClient then fails on a null initClosure and the original error is lost. A thread that reaches :786 while the first thread is still inside the customizer also gets the engine without its customizations. This bug existed before the PR, and volatile does not make it worse. It does mean that volatile alone does not make this lazy initialization correct. Clearing the field on failure fixes the failure case. The case of a concurrent thread needs a separate "customized" flag.

groovyScriptEngine =
        new GroovyScriptEngine(getRoots(compilerConfiguration, loader), loader);
try {
    initializeCustomizer();
} catch (RuntimeException e) {
    groovyScriptEngine = null;
    throw e;
}

---

suggestion (non-blocking): Read groovyScriptEngine into a local variable once in getGroovyScriptEngine().

OpenICF-groovy-connector/src/main/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfiguration.java:786, :804, :668

The fast path checks the volatile field at :786 and reads it again at :804. Meanwhile release() sets it to null at :668 under a lock the fast path does not take. On connector-server shutdown, ConnectorServerImpl.stop eventually calls ScriptedConfiguration.release, and ConnectionListener.shutdown does not wait for in-flight requests to finish. A script evaluation still running at that moment can get null back and throw an NPE in evaluate at :744. The window is small, and the race existed before the PR. Reading the field once into a local is the usual way to write this pattern with volatile:

GroovyScriptEngine engine = groovyScriptEngine;
if (null == engine) {
    synchronized (this) {
        engine = groovyScriptEngine;
        if (null == engine) {
            final CompilerConfiguration compilerConfiguration =
                    new CompilerConfiguration(config);
            compilerConfiguration.addCompilationCustomizers(getImportCustomizer(null));

            final GroovyClassLoader loader =
                    new GroovyClassLoader(getParentLoader(), compilerConfiguration, true);

            engine = new GroovyScriptEngine(getRoots(compilerConfiguration, loader), loader);
            groovyScriptEngine = engine;

            initializeCustomizer();
        }
    }
}
return engine;

[vharseko]

I rebased the branch onto current master with no conflicts and addressed all three points in 9aa4f8d.

question: ContractITCase and TestNG 7.x. Yes, the contract should keep working there. I confirmed the class moves against the jars: ObjectFactoryImpl is org.testng.internal in 6.9.10 and org.testng.internal.objects in 7.5, and 7.10.2 no longer has org.testng.IObjectFactory. createInstances now calls the (String) constructor through plain reflection, as you suggested, and the IObjectFactory / ObjectFactoryImpl / Constructor imports are gone. According to javap, the only TestNG class the compiled ContractITCase still references is org.testng.ITestContext.

issue: the engine stays uncustomized after a customizer failure. Opened as #148, together with the concurrent-caller case you described. I fixed both here instead of only clearing the field on failure. getGroovyScriptEngine() now publishes the engine to the volatile field only after initializeCustomizer() has succeeded. The customizer's own re-entrant call through getCustomizerClass() gets the engine under construction from a second field, customizingGroovyScriptEngine, which is read and written only under the monitor. A concurrent caller therefore waits on the monitor until the customization is done. If the customizer throws, nothing is published, and the next call builds a fresh engine and runs the customizer again. The new ScriptedConfigurationTest covers both cases: a customizer that fails once is retried, and a second thread does not get the engine while the customizer runs. Both tests fail on the previous code.

suggestion: read the field once. Opened as #149 and fixed in the same change. The field is read once into a local on the fast path and once under the lock, so release() can no longer make the getter return null. A test cannot hold that window open, so this one has no test.

I also updated the PR description: ScriptedConfiguration now has its own row in the table, explaining why volatile alone was not enough there. Local reactor run on the rebased branch: groovy-connector 127 (42 skipped as on master, 2 new), connector-framework 192, framework-internal 499, contract 43, connector-framework-server 32, dbcommon 86. All green.

[maximthomas]

praise: The round-1 points are fixed where they started.

• ScriptedConfiguration.getGroovyScriptEngine() writes the volatile field only after initializeCustomizer() returns. The re-entrant call goes through customizingGroovyScriptEngine, which is guarded by the monitor. A failed customizer publishes nothing, and no other thread can see an uncustomized engine.
• testFailedCustomizerIsRetriedOnTheNextCall fails on the previous code: the second call returned at the fast path and calls stayed 1. It ran green in CI at 9aa4f8d (groovy-connector: 127 run, 0 failed).
• ContractITCase.createInstances no longer references org.testng.internal.

---

issue (non-blocking): A REST/CREST/SSH customizer that keeps failing leaks one more GroovyClassLoader on every call.

OpenICF-groovy-connector/src/main/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfiguration.java:814-823, :832-845; OpenICF-groovy-connector/src/main/groovy/org/forgerock/openicf/connectors/scriptedrest/ScriptedRESTConfiguration.groovy:189, OpenICF-groovy-connector/src/main/groovy/org/forgerock/openicf/connectors/scriptedcrest/ScriptedCRESTConfiguration.groovy:224, OpenICF-ssh-connector/src/main/groovy/org/forgerock/openicf/connectors/ssh/SSHConfiguration.groovy:387

When the customizer throws, nothing is published. Each later getGroovyScriptEngine() call, from evaluate, loadScript or getHttpClient, builds a new loader and engine and runs the customizer again. That retry is intended. The REST, CREST and SSH createCustomizerScript overrides, though, run customizerClass.metaClass.customize << {…} before the script runs. That registers an ExpandoMetaClass on the newly compiled class, and on Groovy 2.4.21 the registration keeps the class's loader alive. In a probe of 50 failed attempts, every loader was still reachable after the heap was exhausted when the EMC was registered, and none was without it. So a customizer that fails at run time, for example a typo that raises MissingPropertyException, leaks one loader per failed operation. Before this change it was one per engine. GroovyClassLoader.clearCache() in 2.4.21 only clears its maps. InvokerHelper.removeClass unregisters the class. The fix below was not run.

    private void initializeCustomizer() {
        Class customizerClass = null;
        try {
            customizerClass = getCustomizerClass();

            if (null != customizerClass) {
                Binding binding = new Binding();
                binding.setVariable(LOGGER, getLogger(customizerClass));
                createCustomizerScript(customizerClass, binding).run();
            }
        } catch (Throwable t) {
            if (null != customizerClass) {
                // The retry compiles a new class; unregister this one so its loader can be collected.
                InvokerHelper.removeClass(customizerClass);
            }
            logger.error(t, "Failed to customize the connector");
            throw ConnectorException.wrap(t);
        }
    }

Plus import org.codehaus.groovy.runtime.InvokerHelper;.

---

suggestion (non-blocking): Document that a customizer must not wait for another thread that uses this configuration's engine.

OpenICF-groovy-connector/src/main/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfiguration.java:798-805

Only the thread that holds the monitor sees customizingGroovyScriptEngine. Every other thread blocks on synchronized (this) until the customizer returns, which is what #148 asks for. One consequence: a customizer that runs Thread.start { configuration.loadScript('X.groovy') }.join(), or waits on an evaluate() it submitted, now hangs forever. The previous code published the engine early, so the same script finished. No bundled customizer does this.

    /*
     * This must be called once from thread-safe location and inside the
     * synchronized to avoid deadlock. The customizer runs while this
     * configuration's monitor is held: it must not wait for another thread
     * that calls getGroovyScriptEngine(), evaluate() or loadScript().
     */
    private void initializeCustomizer() {

---

suggestion (non-blocking): testEngineIsHiddenFromOtherThreadsUntilCustomized takes a 500 ms timeout as proof that the second caller blocked.

OpenICF-groovy-connector/src/test/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfigurationTest.java:114-120

Suppose the engine were published early again. The second caller would then return as soon as it is scheduled, and that return is the only thing that turns this test red. If the pool thread is not scheduled within 500 ms on a loaded runner, the TimeoutException is caught, both futures later return the same engine, and isSameAs passes. On a normal schedule the test catches the regression, but not on every run. HEAD itself cannot fail spuriously.

        final AtomicReference<Thread> secondThread = new AtomicReference<>();
        Future<GroovyScriptEngine> second = executor.submit(() -> {
            secondThread.set(Thread.currentThread());
            return configuration.getGroovyScriptEngine();
        });
        while (!second.isDone() && (secondThread.get() == null
                || secondThread.get().getState() != Thread.State.BLOCKED)) {
            Thread.sleep(10);
        }
        assertThat(second.isDone()).as("second caller returned while the customizer ran").isFalse();

Pin: this replaces lines 114-120, plus import java.util.concurrent.atomic.AtomicReference;. An early publish then fails on every run, not only when the pool thread is scheduled within 500 ms.

[vharseko]

I rebased the branch onto current master with no conflicts and addressed all three points in 9ce4b58.

issue: loader leak on a failing customizer. initializeCustomizer() now calls InvokerHelper.removeClass(customizerClass) when the customizer fails, as you suggested; in 2.4.21 it calls removeMetaClass, ClassInfo.remove and Introspector.flushFromCaches. The new testFailedCustomizerClassIsUnregistered registers an ExpandoMetaClass on the customizer class the way REST/CREST/SSH do and checks that no strong metaclass is left after the failure. It checks ClassInfo rather than waiting for GC, so it is deterministic. It fails without the fix.

suggestion: document the deadlock constraint. Added your wording to the comment on initializeCustomizer().

suggestion: the 500 ms timeout. Replaced it with your loop: the test waits until the second thread is BLOCKED or done, then asserts it is not done. With the engine published before the customizer runs, it now fails on every run.

I checked the three tests against a reverted fix (no removeClass, engine published before the customizer): all three fail, each on its own assertion. I also updated the ScriptedConfiguration row and the test paragraph in the PR description. Local reactor run on the rebased branch: groovy-connector 128 (42 skipped as on master, 3 new), ssh-connector 96, connector-framework 192, framework-internal 499. All green.

[vharseko]

Right after my previous comment #133, #134, #136, #141 and #143 were merged, and the branch conflicted with master again. I rebased it once more; the head is now 863f079.

The only conflict was in ScriptedConfiguration.getGroovyScriptEngine(). #133 had already made the getter synchronized for the whole initialisation, which closed #1549 and #149 and makes concurrent callers wait for the customizer. I resolved the conflict in favour of that design: the lock-free fast path and the volatile field from this PR are gone. What this PR still adds on top of #133 is the part of #148 that master does not cover: the engine is published only after the customizer succeeds (re-entrant calls get it from customizingGroovyScriptEngine), and a failed customizer class is removed with InvokerHelper.removeClass. The rest of round 3 is unchanged: the comment on initializeCustomizer() and the BLOCKED loop in the test.

On the new base I checked the three tests against reverted parts of the fix: engine published before the customizer, no removeClass, getter not synchronised. All three fail, each on its own assertion. I updated the PR description: 10 alerts instead of 11, the ScriptedConfiguration row now builds on #133, and the test paragraph and test counts are current. Local reactor run of all touched modules: connector-framework 197, dbcommon 86, framework-internal 509, contract 43, connector-framework-server 32, groovy-connector 129 (42 skipped as on master, 3 new), ssh-connector 96. All green.