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/main/java/org/opends/server/loggers/DebugLogPublisher.java | 163 +++++++++++++++++++++---------------------------------
1 files changed, 63 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;
+ });
}
--
Gitblit v1.10.0