Skip to content

fix: publish the broadcast cache atomically (#429) - #436

Merged
lduchosal merged 1 commit into
masterfrom
fix/429-broadcast-cache-atomic-publication
Aug 18, 2026
Merged

fix: publish the broadcast cache atomically (#429)#436
lduchosal merged 1 commit into
masterfrom
fix/429-broadcast-cache-atomic-publication

Conversation

@lduchosal

Copy link
Copy Markdown
Owner

Fixes #429.

The defect

InternalBroadcast cached the broadcast address in a BigInteger? and read it on a lock-free fast path:

private BigInteger? cachedBroadcast;

var cached = this.cachedBroadcast;   // fast path, outside the lock
if (cached != null) return cached.Value;
lock (this.sync) { /* double-checked locking */ }

A Nullable<BigInteger> is 24 bytes on a 64-bit runtime and spans three fields: hasValue, BigInteger._sign and BigInteger._bits. The CLR only guarantees atomicity for values up to the native word size, so neither the write nor the read is atomic, and the field was not volatile. A reader on the fast path could observe hasValue == true while the magnitude was still half written.

Observable torn states, and what they produce:

Observed Value Effect
hasValue=true, _sign=0, _bits=null 0 Contains() returns false for everything
hasValue=true, _sign=1, _bits=null 1 broadcast of 1, wrong results
_sign=0, _bits set non-canonical incoherent comparisons

No exception is thrown: Contains() just answers wrong. The first case hits IPv4 networks whose broadcast fits in an int, the second hits IPv4 >= 128.0.0.0 and all IPv6.

Severity is higher than "a few nanoseconds"

Measured on arm64, the torn state stayed observable for 7 to 19 microseconds, because the publishing thread can be descheduled between the individual stores. It is not a handful of cycles: every reader taking the fast path during that window gets a wrong answer.

Exposure is realistic, since the library itself hands out shared IPNetwork2 singletons (IANA_*, UniqueLocalAddress.Ula*) and the common pattern is a static readonly allow/deny network hit from many threads.

The fix

private volatile StrongBox<BigInteger>? cachedBroadcast;

A reference is published by a single atomic store, and volatile orders that store after the box is fully initialised, so a reader sees either no cache at all or a complete one.

The double-checked lock is dropped: CreateBroadcast is pure, so a racing duplicate computation is harmless and cheaper than serialising every first access. That also removes the per-instance sync object, one allocation less for every IPNetwork2 ever created, while the StrongBox is only allocated for instances whose broadcast is actually requested.

Tests

Written first, and confirmed red against the previous code:

échec TestContainsIsCorrectUnderConcurrentFirstAccess (88ms)
  Contains(200.1.2.3) returned false for 128.0.0.0/1
échec TestBroadcastIsCorrectUnderConcurrentFirstAccess (25ms)
  Broadcast was 0.0.0.0 instead of 255.255.255.255
échec TestBroadcastCacheIsAtomicallyPublishable (0ms)
  IPNetwork2.cachedBroadcast is a value type (Nullable`1)

The two race detectors release every worker onto a brand new IPNetwork2 at the same instant, round after round, with a rotating Thread.SpinWait offset per thread and per round so the observations sweep across the publication window instead of hoping to land in it. They reproduced 5 times out of 5, in 78 ms to 1.1 s, always within 1800 rounds.

A race detector is not a proof, and it will most likely stay quiet on a small strongly ordered CI runner. TestBroadcastCacheIsAtomicallyPublishable is the deterministic guard that runs everywhere: it fails if the cache ever goes back to a multi-word value type.

Validation

Check Result
The 3 new tests 3/3 pass
Full suite, net10.0 1367/1367 pass
Standalone bench, Release, 3 x 500 000 rounds x 15 threads 22.5M observations, 0 tear (reproduced within 1800 rounds before the fix)
Library build: net10.0, net9.0, net8.0, netstandard2.1, netstandard2.0 0 warnings
TestProject build: net10.0 and net48 0 warnings

The two race tests spend a 2 second budget each, so the suite grows by about 4 seconds. Happy to shorten that or gate them behind a category if you would rather keep the local loop tight.

Not addressed here

Scoped strictly to #429. An audit of the surrounding code turned up four further defects, all reproduced, which I will report separately: IPNetworkCollection.GetEnumerator() returning this, the non-atomic three-field mutation in the Value setter, GetHashCode() staying stale after mutation (#373 looks closed in error), and the static singletons being publicly mutable.

🤖 Generated with Claude Code

Contains() could silently return a wrong result under concurrency.

The broadcast cache was a BigInteger?, a 24 byte struct spanning three
fields (hasValue, BigInteger._sign and BigInteger._bits), read on a
lock-free fast path. Neither the read nor the write of such a struct is
atomic, so a reader could observe hasValue == true while the magnitude
was still half written, and compute a broadcast address of 0 or 1
instead of the real one. Contains() then answered false for addresses
that are in the network.

Measured on arm64: the torn state stayed observable for 7 to 19 us, not
a few nanoseconds, because the publishing thread can be descheduled
between the individual stores. Every reader taking the fast path during
that window gets a wrong answer.

The cache is now a volatile StrongBox<BigInteger>: a reference is
published by a single atomic store, and volatile orders that store
after the box is fully initialised. The double-checked lock is dropped
because CreateBroadcast is pure, so a racing duplicate computation is
harmless and cheaper than serialising every first access. That also
removes the per-instance sync object, one allocation less for every
IPNetwork2 ever created.

Tests: two race detectors plus one deterministic structural guard. The
race detectors only reproduce on weakly ordered multi-core machines, so
the structural guard is what protects the invariant on CI: it fails if
the cache ever goes back to a multi-word value type.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@mergify

mergify Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@coveralls

coveralls commented Aug 18, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 1008

Coverage increased (+0.06%) to 94.069%

Details

  • Coverage increased (+0.06%) from the base build.
  • Patch coverage: No coverable lines changed in this PR.
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 2428
Covered Lines: 2284
Line Coverage: 94.07%
Coverage Strength: 487916.14 hits per line

💛 - Coveralls

@sonarqubecloud

Copy link
Copy Markdown

@lduchosal
lduchosal merged commit b1148ef into master Aug 18, 2026
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Contains() can silently return a wrong result under concurrency — unsynchronized fast-path read of the Nullable<BigInteger> broadcast cache can tear

2 participants