Skip to content

identity/deserializer: unsynchronized map access in the signer multiplex can crash the process - #2165

Merged
Effi-S merged 1 commit into
mainfrom
fix-2079
Aug 12, 2026
Merged

identity/deserializer: unsynchronized map access in the signer multiplex can crash the process#2165
Effi-S merged 1 commit into
mainfrom
fix-2079

Conversation

@Effi-S

@Effi-S Effi-S commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Fixes #2079

Summary

The signer deserializer multiplex keeps its per-type deserializer lists in a plain Go map that is
written under one mutex and read without any lock, so a concurrent registration and lookup can race
and trigger Go's "concurrent map read and map write" fatal error.

Where

token/services/identity/deserializer/signer.go:23 declares
deserializers map[IdentityType][]TypedSignerDeserializer. Writes happen via
AddTypedSignerDeserializer (signer.go:30-38), called from membership/lm.go:818 while holding
localIdentitiesMutex. Reads happen unlocked at signer.go:45, from
Provider.getSignerAndCache (provider.go:325). These are two different mutexes, so the lock held
during registration provides no protection for the concurrent read path. Registration can happen at
runtime via refreshAndGet (lm.go:889), so a live GetSigner call can genuinely race a new
registration. verifier.go:26 has the same shape, and eidrh.go:26's AddDeserializer has no
mutex at all.

Impact

A registration happening concurrently with a lookup on the same multiplex can trigger Go's runtime
map-race detector, which terminates the process — this is a stability concern under concurrent
identity-registration workloads, independent of any race detector build (the runtime check is always
active for maps, not just under -race).

Suggested fix

Guard map access consistently — either take the same mutex used for writes on the read path (a small
read-lock around the lookup), or switch to a concurrency-safe map type if lock contention on the read
path is a concern given how frequently getSignerAndCache is called. Apply the same fix to
verifier.go and add a mutex to eidrh.go's AddDeserializer, since all three share the same
pattern.

Severity

HIGH — a data-race that can crash the process, though it requires registration and lookup to overlap
in time to manifest.

@Effi-S Effi-S added this to the Q3/26 milestone Aug 9, 2026
@Effi-S Effi-S self-assigned this Aug 9, 2026
Comment thread cmd/benchmarking/bench_parse.py Fixed
Comment thread cmd/benchmarking/bench_parse.py Fixed
@Effi-S Effi-S closed this Aug 9, 2026
@Effi-S Effi-S reopened this Aug 9, 2026
@Effi-S
Effi-S marked this pull request as ready for review August 10, 2026 07:57
@Effi-S
Effi-S requested a review from AkramBitar August 10, 2026 07:57
@Effi-S
Effi-S force-pushed the fix-2079 branch 2 times, most recently from 6b34786 to eac3e53 Compare August 10, 2026 14:08

@AkramBitar AkramBitar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The read lock is released before the slice is iterated in three places (signer.go, verifier.go, eidrh.go). A concurrent writer calling append may allocate a new backing array at that moment; the reader holds a copy of the old slice header and misses the new entry. This is safe (no crash) but creates a stale-read window.

Consider either holding the lock through the iteration, or copying the slice under the lock before releasing:

v.mutex.RLock()
dess := make([]TypedSignerDeserializer, len(v.deserializers[si.Type]))
copy(dess, v.deserializers[si.Type])
v.mutex.RUnlock()

Comment thread token/services/identity/deserializer/signer.go
@Effi-S

Effi-S commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

The read lock is released before the slice is iterated in three places (signer.go, verifier.go, eidrh.go).

eidrh.go is not a slice but a single object so it is already copied:

deserializers map[identity.Type]driver2.AuditInfoDeserializer

@Effi-S
Effi-S requested a review from AkramBitar August 11, 2026 12:12
@Effi-S
Effi-S force-pushed the fix-2079 branch 3 times, most recently from 12ed0b1 to de0f99f Compare August 12, 2026 11:54

@AkramBitar AkramBitar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Signed-off-by: Effi-S <effi.szt@gmail.com>
@Effi-S
Effi-S merged commit 5642052 into main Aug 12, 2026
149 of 150 checks passed
@Effi-S
Effi-S deleted the fix-2079 branch August 12, 2026 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

identity/deserializer: unsynchronized map access in the signer multiplex can crash the process

2 participants