Fix java/unsynchronized-getter CodeQL alerts in DebugLogPublisher (#788)

Valery Kharseko

17 hours ago 8c46e654ad4027c6c97dfccc1e410b15f3c186df

Fix java/unsynchronized-getter CodeQL alerts in DebugLogPublisher (#788)

 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;
    });
  }



 opendj-server-legacy/src/test/java/org/opends/server/loggers/DebugLogPublisherTest.java

New file
@@ -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"));
  }
}