TOCTOU race in TransactionTracer.AddChildSpan span-limit check

[jamescrosswell]

Summary

The span-limit check in TransactionTracer.AddChildSpan reads _spans.Count outside the _finishLock it later takes when actually adding the span. Under concurrent span creation, two threads can both observe Count < SpanLimit, both fall through to the lock-and-add path, and both add — letting _spans.Count exceed SpanLimit (1000).

Code

private void AddChildSpan(SpanTracer span)
{
    var isOutOfLimit = _spans.Count >= SpanLimit;        // ← unlocked check
    span.IsSampled = isOutOfLimit ? false : IsSampled;
    if (isOutOfLimit) { ...; return; }

    lock (_finishLock)
    {
        ...
        _spans.Add(span);                                 // ← locked add
    }
}

Race

• T1: reads _spans.Count = 999 → isOutOfLimit = false
• T2: reads _spans.Count = 999 → isOutOfLimit = false
• T1: takes lock, _spans.Add(span) → Count = 1000
• T2: takes lock, _spans.Add(span) → Count = 1001

Both spans are added with normal IsSampled (the parent transaction's value, typically true). They are not marked unsampled in the racy path — that branch is only taken when the unlocked check observes the limit was already exceeded.

Suggested fix

Move the limit check inside the lock so check-and-add is atomic:

private void AddChildSpan(SpanTracer span)
{
    lock (_finishLock)
    {
        if (_hasFinished) { ...; return; }
        if (_spans.Count >= SpanLimit)
        {
            span.IsSampled = false;
            _options?.LogDebug("Discarding child span '{0}' due to {1} span limit", SpanId, SpanLimit);
            return;
        }
        span.IsSampled = IsSampled;
        _idleTimer?.Cancel();
        _spans.Add(span);
        _activeSpanTracker.Push(span);
    }
}

Practical impact

Low. The race window is narrow (instructions between unlocked count read and lock acquisition), and the consequence is that _spans.Count exceeds 1000 by a small handful in highly-concurrent span creation. No functional bug — spans still record correctly. Worth fixing for correctness, but not urgent.

References

Originally flagged by sentry[bot] on #5138 (comment) — note that the bot's comment incorrectly claimed extra spans would be marked IsSampled = false; the analysis above corrects that.

[linear-code]

NET-666