cmp: avoid time.Location initialization race in reports - #400
Open
1678092075 wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When
cmp.Diffproduces a high-verbosity report that avoidsStringmethods, it can reflect over the unexported fields of atime.Time. If the referencedtime.Locationis being lazily initialized at the same time, the report races with the standard library's initialization writes.This is the reporting race described in #381 and golang/go#74460.
Root cause
The fallback report bypasses
fmt.Stringerso it can expose otherwise hidden differences. Fortime.Time, that means recursively reading its privateLocationstate without first synchronizing with lazy location initialization.Change
Before reflectively formatting a
time.Timewith Stringer formatting disabled, initialize the specificLocationreferenced by that value. This keeps the existing reflective output, including map-key disambiguation, while avoiding concurrent reads during lazy initialization.The regression coverage:
time.Localafter constructing the value, verifying that the value's actual location is synchronized;time.Timemap keys with identicalStringoutput remain distinguishable.Verification
GOCACHE=/tmp/go-cmp-cache-regression go test -race ./cmp -run '^TestDiffDoesNotRaceWithTimeLocalInitialization$' -count=5— passGOCACHE=/tmp/go-cmp-cache-target go test ./cmp -run '^TestFormatMapKeyDisambiguatesTimeLocations$' -count=3— passGOCACHE=/tmp/go-cmp-cache-race go test -race ./...— passtest -z "$(gofmt -d .)"— passgit diff --check HEAD^ HEAD— passCompatibility and risk
There is no public API change. The special case only runs when report formatting intentionally bypasses Stringer methods; ordinary
time.Timeformatting and comparison behavior are unchanged.Fixes #381.