Skip to content

LogOptions.CreateLogger reads LoggerFactory twice — check/create TOCTOU race #252

Description

@ptr727

Summary

LogOptions.CreateLogger(string) reads the LoggerFactory property (a Volatile.Read of s_loggerFactory) twice — once for the ReferenceEquals guard and once for the actual CreateLogger call. Because the backing field can be swapped concurrently (SetFactory and the property setter use Interlocked.Exchange), the guard and the create can observe different factory instances.

Location

LanguageTags/LogOptions.cs, in CreateLogger(string categoryName):

// LoggerFactory -> NullLogger
return !ReferenceEquals(LoggerFactory, NullLoggerFactory.Instance)
    ? LoggerFactory.CreateLogger(categoryName)
    : NullLogger.Instance;

Problem

This is a check-then-act (TOCTOU) race. A thread can evaluate the guard against a real factory F1, then — if another thread calls SetFactory(...) in the window — create the logger from a different instance F2 (or from NullLoggerFactory if reset). It will not crash (NullLoggerFactory.CreateLogger is safe), but the decision and the action use inconsistent state, which defeats the thread-safe intent of the Volatile/Interlocked design.

Severity is low (worst case is a logger routed through the just-swapped factory during a narrow race window), but it is a trivial, worthwhile hardening for a library that advertises thread-safe logger configuration.

Suggested fix

Read the factory once into a local and use it for both the check and the create:

internal static ILogger CreateLogger(string categoryName)
{
    ArgumentException.ThrowIfNullOrWhiteSpace(categoryName);

    // Read the factory once so a concurrent swap cannot split the check
    // and the create across two different instances.
    ILoggerFactory factory = LoggerFactory;
    return !ReferenceEquals(factory, NullLoggerFactory.Instance)
        ? factory.CreateLogger(categoryName)
        : NullLogger.Instance;
}

The sibling ptr727.Utilities library adopts this same LogOptions and already applied this exact fix after a Copilot review flagged it (ptr727/Utilities#381). This issue is to keep the upstream in sync.

Minor / optional

The <remarks> XML doc block contains an em dash (...existing cached loggers—only new logger requests...). ptr727.Utilities replaced this with two ASCII sentences to satisfy its no-em-dash documentation rule. LanguageTags has no such rule, so this is purely an optional cross-repo consistency note, not a defect.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions