From 8c46e654ad4027c6c97dfccc1e410b15f3c186df Mon Sep 17 00:00:00 2001
From: Valery Kharseko <vharseko@3a-systems.ru>
Date: Thu, 30 Jul 2026 13:40:45 +0000
Subject: [PATCH] Fix java/unsynchronized-getter CodeQL alerts in DebugLogPublisher (#788)
---
opendj-server-legacy/src/test/java/org/opends/server/loggers/DebugLogPublisherTest.java | 232 +++++++++++++++++++++++++++++++++
opendj-server-legacy/src/main/java/org/opends/server/loggers/DebugLogPublisher.java | 163 +++++++++--------------
2 files changed, 295 insertions(+), 100 deletions(-)
diff --git a/opendj-server-legacy/src/main/java/org/opends/server/loggers/DebugLogPublisher.java b/opendj-server-legacy/src/main/java/org/opends/server/loggers/DebugLogPublisher.java
index 7d8489a..e5e5597 100644
--- a/opendj-server-legacy/src/main/java/org/opends/server/loggers/DebugLogPublisher.java
+++ b/opendj-server-legacy/src/main/java/org/opends/server/loggers/DebugLogPublisher.java
@@ -45,11 +45,11 @@
/**
* The map of class names to their trace settings.
* <p>
- * The getters below read this map without any lock, so the field is volatile
- * (the map is created lazily and must be published safely) and the map itself
- * is concurrent; all writes go through the synchronized mutators below.
+ * The getters below read this map without any lock, so it is concurrent and
+ * created eagerly, which makes it safely published to every reader. Updates
+ * rely on the atomic operations of the map rather than on locking.
*/
- private volatile Map<String,TraceSettings> classTraceSettings;
+ private final Map<String,TraceSettings> classTraceSettings = new ConcurrentHashMap<>();
/**
* The map of class names to their method trace settings.
@@ -59,16 +59,13 @@
* {@link #getMethodSettings(String)} and iterates it while another thread may be
* reconfiguring the publisher.
*/
- private volatile Map<String,Map<String,TraceSettings>> methodTraceSettings;
+ private final Map<String,Map<String,TraceSettings>> methodTraceSettings = new ConcurrentHashMap<>();
/** Construct a default configuration where the global scope will only log at the ERROR level. */
protected DebugLogPublisher()
{
- classTraceSettings = null;
- methodTraceSettings = null;
-
//Set the global settings so that nothing is logged.
addTraceSettings(null, TraceSettings.DISABLED);
}
@@ -93,13 +90,13 @@
* @param className The fully-qualified name of the class for
* which to get the trace levels.
*
- *@return An unmodifiable map of trace levels keyed by method name,
- * or {@code null} if no method-level tracing is configured
- * for the scope.
+ *@return A live, concurrent map of trace levels keyed by method
+ * name, or {@code null} if no method-level tracing is
+ * configured for the scope.
*/
final Map<String, TraceSettings> getMethodSettings(String className)
{
- return methodTraceSettings != null ? methodTraceSettings.get(className) : null;
+ return methodTraceSettings.get(className);
}
/**
@@ -112,36 +109,32 @@
*/
final TraceSettings getClassSettings(String className)
{
- TraceSettings settings = null;
- if (classTraceSettings != null)
+ // Find most specific trace setting
+ // which covers this fully qualified class name
+ // Search up the hierarchy for a match.
+ String searchName = className;
+ TraceSettings settings = classTraceSettings.get(searchName);
+ while (settings == null && searchName != null)
{
- // Find most specific trace setting
- // which covers this fully qualified class name
- // Search up the hierarchy for a match.
- String searchName = className;
- settings = classTraceSettings.get(searchName);
- while (settings == null && searchName != null)
+ int clipPoint = searchName.lastIndexOf('$');
+ if (clipPoint == -1)
{
- int clipPoint = searchName.lastIndexOf('$');
- if (clipPoint == -1)
- {
- clipPoint = searchName.lastIndexOf('.');
- }
- if (clipPoint != -1)
- {
- searchName = searchName.substring(0, clipPoint);
- settings = classTraceSettings.get(searchName);
- }
- else
- {
- searchName = null;
- }
+ clipPoint = searchName.lastIndexOf('.');
}
- // Try global settings
- // only if no specific target is defined
- if (settings == null && classTraceSettings.size()==1) {
- settings = classTraceSettings.get(GLOBAL);
+ if (clipPoint != -1)
+ {
+ searchName = searchName.substring(0, clipPoint);
+ settings = classTraceSettings.get(searchName);
}
+ else
+ {
+ searchName = null;
+ }
+ }
+ // Try global settings
+ // only if no specific target is defined
+ if (settings == null && classTraceSettings.size()==1) {
+ settings = classTraceSettings.get(GLOBAL);
}
return settings == null ? TraceSettings.DISABLED : settings;
}
@@ -199,21 +192,14 @@
{
String methodName = scope.substring(methodPt + 1);
scope = scope.substring(0, methodPt);
- if (methodTraceSettings != null)
+ Map<String, TraceSettings> methodLevels = methodTraceSettings.get(scope);
+ if (methodLevels != null)
{
- Map<String, TraceSettings> methodLevels =
- methodTraceSettings.get(scope);
- if (methodLevels != null)
- {
- return methodLevels.containsKey(methodName);
- }
+ return methodLevels.containsKey(methodName);
}
+ return false;
}
- else if (classTraceSettings != null)
- {
- return classTraceSettings.containsKey(scope);
- }
- return false;
+ return classTraceSettings.containsKey(scope);
}
@@ -230,43 +216,29 @@
* {@code null} if no trace setting is defined for that
* scope.
*/
- final synchronized TraceSettings removeTraceSettings(String scope)
+ final TraceSettings removeTraceSettings(String scope)
{
- TraceSettings removedSettings = null;
if (scope == null) {
- if(classTraceSettings != null)
+ return classTraceSettings.remove(GLOBAL);
+ }
+ int methodPt= scope.lastIndexOf('#');
+ if (methodPt == -1) {
+ return classTraceSettings.remove(scope);
+ }
+ final String methodName= scope.substring(methodPt+1);
+ // Drop the enclosing map once its last method setting is gone. This must be
+ // atomic with respect to setMethodSettings(), which computes on the same key,
+ // otherwise a concurrently added setting could be discarded along with the map.
+ final TraceSettings[] removedSettings = new TraceSettings[1];
+ methodTraceSettings.compute(scope.substring(0, methodPt), (className, methodLevels) -> {
+ if (methodLevels == null)
{
- removedSettings = classTraceSettings.remove(GLOBAL);
+ return null;
}
- }
- else {
- int methodPt= scope.lastIndexOf('#');
- if (methodPt != -1) {
- String methodName= scope.substring(methodPt+1);
- scope= scope.substring(0, methodPt);
- if(methodTraceSettings != null)
- {
- Map<String, TraceSettings> methodLevels =
- methodTraceSettings.get(scope);
- if(methodLevels != null)
- {
- removedSettings = methodLevels.remove(methodName);
- if(methodLevels.isEmpty())
- {
- methodTraceSettings.remove(scope);
- }
- }
- }
- }
- else {
- if(classTraceSettings != null)
- {
- removedSettings = classTraceSettings.remove(scope);
- }
- }
- }
-
- return removedSettings;
+ removedSettings[0] = methodLevels.remove(methodName);
+ return methodLevels.isEmpty() ? null : methodLevels;
+ });
+ return removedSettings[0];
}
/**
@@ -275,12 +247,8 @@
* @param className The class name.
* @param settings The trace settings for the class.
*/
- private final synchronized void setClassSettings(String className, TraceSettings settings)
+ private void setClassSettings(String className, TraceSettings settings)
{
- if (classTraceSettings == null)
- {
- classTraceSettings = new ConcurrentHashMap<>();
- }
classTraceSettings.put(className, settings);
}
@@ -293,19 +261,14 @@
* @param methodName The method name.
* @param settings The trace settings for the method.
*/
- private final synchronized void setMethodSettings(String className,
- String methodName, TraceSettings settings)
+ private void setMethodSettings(String className, String methodName, TraceSettings settings)
{
- if (methodTraceSettings == null) {
- methodTraceSettings = new ConcurrentHashMap<>();
- }
- Map<String, TraceSettings> methodLevels = methodTraceSettings.get(className);
- if (methodLevels == null)
- {
- methodLevels = new ConcurrentHashMap<>();
- methodTraceSettings.put(className, methodLevels);
- }
- methodLevels.put(methodName, settings);
+ // Create the enclosing map and add the setting atomically, see removeTraceSettings().
+ methodTraceSettings.compute(className, (name, methodLevels) -> {
+ Map<String, TraceSettings> levels = methodLevels != null ? methodLevels : new ConcurrentHashMap<>();
+ levels.put(methodName, settings);
+ return levels;
+ });
}
diff --git a/opendj-server-legacy/src/test/java/org/opends/server/loggers/DebugLogPublisherTest.java b/opendj-server-legacy/src/test/java/org/opends/server/loggers/DebugLogPublisherTest.java
new file mode 100644
index 0000000..c8798b2
--- /dev/null
+++ b/opendj-server-legacy/src/test/java/org/opends/server/loggers/DebugLogPublisherTest.java
@@ -0,0 +1,232 @@
+/*
+ * The contents of this file are subject to the terms of the Common Development and
+ * Distribution License (the License). You may not use this file except in compliance with the
+ * License.
+ *
+ * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the
+ * specific language governing permission and limitations under the License.
+ *
+ * When distributing Covered Software, include this CDDL Header Notice in each file and include
+ * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL
+ * Header, with the fields enclosed by brackets [] replaced by your own identifying
+ * information: "Portions copyright [year] [name of copyright owner]".
+ *
+ * Copyright 2026 3A Systems, LLC.
+ */
+package org.opends.server.loggers;
+
+import static org.testng.Assert.*;
+
+import java.util.Map;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.forgerock.opendj.ldap.DN;
+import org.forgerock.opendj.server.config.server.DebugLogPublisherCfg;
+import org.opends.server.DirectoryServerTestCase;
+import org.opends.server.core.ServerContext;
+import org.testng.annotations.Test;
+
+@SuppressWarnings("javadoc")
+public class DebugLogPublisherTest extends DirectoryServerTestCase
+{
+ /** Minimal publisher exposing nothing but the trace settings bookkeeping under test. */
+ private static final class TestDebugLogPublisher extends DebugLogPublisher<DebugLogPublisherCfg>
+ {
+ @Override
+ public void initializeLogPublisher(DebugLogPublisherCfg config, ServerContext serverContext)
+ {
+ // Nothing to initialize.
+ }
+
+ @Override
+ public void trace(TraceSettings settings, String signature, String sourceLocation, String msg,
+ StackTraceElement[] stackTrace)
+ {
+ // Not used by these tests.
+ }
+
+ @Override
+ public void traceException(TraceSettings settings, String signature, String sourceLocation, String msg,
+ Throwable ex, StackTraceElement[] stackTrace)
+ {
+ // Not used by these tests.
+ }
+
+ @Override
+ public DN getDN()
+ {
+ return null;
+ }
+
+ @Override
+ public void close()
+ {
+ // Nothing to close.
+ }
+ }
+
+ @Test
+ public void testFreshPublisherTracesNothing() throws Exception
+ {
+ DebugLogPublisher<?> publisher = new TestDebugLogPublisher();
+
+ assertSame(publisher.getClassSettings("com.example.Foo"), TraceSettings.DISABLED);
+ assertNull(publisher.getMethodSettings("com.example.Foo"));
+ assertFalse(publisher.hasTraceSettings("com.example.Foo"));
+ assertFalse(publisher.hasTraceSettings("com.example.Foo#bar"));
+ }
+
+ @Test
+ public void testGlobalSettingsOnlyApplyWhileNoTargetIsDefined() throws Exception
+ {
+ DebugLogPublisher<?> publisher = new TestDebugLogPublisher();
+ TraceSettings global = new TraceSettings();
+ publisher.addTraceSettings(null, global);
+
+ assertSame(publisher.getClassSettings("com.example.Foo"), global);
+
+ publisher.addTraceSettings("com.example", new TraceSettings());
+
+ assertSame(publisher.getClassSettings("org.other.Bar"), TraceSettings.DISABLED);
+ assertSame(publisher.removeTraceSettings(null), global);
+ }
+
+ @Test
+ public void testMostSpecificClassScopeWins() throws Exception
+ {
+ DebugLogPublisher<?> publisher = new TestDebugLogPublisher();
+ TraceSettings packageSettings = new TraceSettings();
+ TraceSettings classSettings = new TraceSettings();
+ publisher.addTraceSettings("com.example", packageSettings);
+ publisher.addTraceSettings("com.example.Foo", classSettings);
+
+ assertSame(publisher.getClassSettings("com.example.Foo"), classSettings);
+ assertSame(publisher.getClassSettings("com.example.Foo$Inner"), classSettings);
+ assertSame(publisher.getClassSettings("com.example.Bar"), packageSettings);
+
+ assertSame(publisher.removeTraceSettings("com.example.Foo"), classSettings);
+ assertSame(publisher.getClassSettings("com.example.Foo"), packageSettings);
+ assertNull(publisher.removeTraceSettings("no.such.Scope"));
+ }
+
+ @Test
+ public void testMethodSettings() throws Exception
+ {
+ DebugLogPublisher<?> publisher = new TestDebugLogPublisher();
+ TraceSettings barSettings = new TraceSettings();
+ TraceSettings bazSettings = new TraceSettings();
+ publisher.addTraceSettings("com.example.Foo#bar", barSettings);
+ publisher.addTraceSettings("com.example.Foo#baz", bazSettings);
+
+ Map<String, TraceSettings> methodSettings = publisher.getMethodSettings("com.example.Foo");
+ assertNotNull(methodSettings);
+ assertEquals(methodSettings.size(), 2);
+ assertSame(methodSettings.get("bar"), barSettings);
+ assertSame(methodSettings.get("baz"), bazSettings);
+ assertTrue(publisher.hasTraceSettings("com.example.Foo#bar"));
+ assertFalse(publisher.hasTraceSettings("com.example.Foo#unknown"));
+ assertNull(publisher.removeTraceSettings("com.example.Foo#unknown"));
+ assertNull(publisher.removeTraceSettings("no.such.Class#bar"));
+ }
+
+ @Test
+ public void testRemovingLastMethodSettingsDiscardsTheEnclosingMap() throws Exception
+ {
+ DebugLogPublisher<?> publisher = new TestDebugLogPublisher();
+ TraceSettings barSettings = new TraceSettings();
+ TraceSettings bazSettings = new TraceSettings();
+ publisher.addTraceSettings("com.example.Foo#bar", barSettings);
+ publisher.addTraceSettings("com.example.Foo#baz", bazSettings);
+
+ assertSame(publisher.removeTraceSettings("com.example.Foo#bar"), barSettings);
+ assertNotNull(publisher.getMethodSettings("com.example.Foo"));
+
+ assertSame(publisher.removeTraceSettings("com.example.Foo#baz"), bazSettings);
+ assertNull(publisher.getMethodSettings("com.example.Foo"));
+ }
+
+ /**
+ * The trace settings are read without any lock, so adding and removing method settings must be
+ * atomic with respect to each other: dropping the map holding the settings of a class must not
+ * discard a setting added concurrently for another method of the same class.
+ */
+ @Test
+ public void testConcurrentUpdatesDoNotLoseMethodSettings() throws Exception
+ {
+ final DebugLogPublisher<?> publisher = new TestDebugLogPublisher();
+ final int writerCount = 4;
+ final int readerCount = 2;
+ final int rounds = 5000;
+ final CountDownLatch start = new CountDownLatch(1);
+ final CountDownLatch finished = new CountDownLatch(writerCount + readerCount);
+ final AtomicReference<Throwable> failure = new AtomicReference<>();
+
+ for (int i = 0; i < writerCount; i++)
+ {
+ final String methodName = "method" + i;
+ new Thread(() -> {
+ try
+ {
+ start.await();
+ for (int round = 0; round < rounds; round++)
+ {
+ TraceSettings settings = new TraceSettings();
+ publisher.addTraceSettings("com.example.Hot#" + methodName, settings);
+ Map<String, TraceSettings> methodSettings = publisher.getMethodSettings("com.example.Hot");
+ assertNotNull(methodSettings, "the map holding " + methodName + " was discarded");
+ assertSame(methodSettings.get(methodName), settings, "the settings of " + methodName + " were lost");
+ publisher.removeTraceSettings("com.example.Hot#" + methodName);
+ }
+ }
+ catch (Throwable t)
+ {
+ failure.compareAndSet(null, t);
+ }
+ finally
+ {
+ finished.countDown();
+ }
+ }).start();
+ }
+
+ for (int i = 0; i < readerCount; i++)
+ {
+ new Thread(() -> {
+ try
+ {
+ start.await();
+ for (int round = 0; round < rounds; round++)
+ {
+ publisher.getClassSettings("com.example.Hot$Inner");
+ Map<String, TraceSettings> methodSettings = publisher.getMethodSettings("com.example.Hot");
+ if (methodSettings != null)
+ {
+ for (TraceSettings settings : methodSettings.values())
+ {
+ assertNotNull(settings.getLevel());
+ }
+ }
+ publisher.hasTraceSettings("com.example.Hot#method0");
+ }
+ }
+ catch (Throwable t)
+ {
+ failure.compareAndSet(null, t);
+ }
+ finally
+ {
+ finished.countDown();
+ }
+ }).start();
+ }
+
+ start.countDown();
+ finished.await();
+ if (failure.get() != null)
+ {
+ throw new AssertionError("concurrent access to the trace settings failed", failure.get());
+ }
+ assertNull(publisher.getMethodSettings("com.example.Hot"));
+ }
+}
--
Gitblit v1.10.0