Skip to content

Commit 5f251b6

Browse files
pavelzemanpavelzeman
committed
mock: fix data race in Arguments.Diff for pointer-like arguments
Arguments.Diff uses fmt.Sprintf("%v") to format arguments for diff output. When arguments contain pointers, maps, slices, or other reference types that are concurrently modified by other goroutines, this causes data races: - For maps: a fatal "concurrent map iteration and map write" error that crashes the entire test process (non-recoverable) - For pointers/slices: data races detectable by -race, and potential panics This commit introduces safeFormatArg() which handles each case: - Maps: uses %p (address-only) since map iteration cannot be made safe without external synchronization - Pointers/slices: attempts %v with panic recovery, falls back to %p - All other types: unchanged %v behavior (value types, no race possible) This preserves the full %v diff output for the common case (non-concurrent arguments, value types, pointers/slices without races) while preventing testify itself from being the source of data races in user tests. Fixes #1597 Related: #1598 (alternative approach using %p for all pointers)
1 parent 5f80e4a commit 5f251b6

2 files changed

Lines changed: 104 additions & 5 deletions

File tree

‎mock/mock.go‎

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -977,15 +977,15 @@ func (args Arguments) Diff(objects []interface{}) (string, int) {
977977
actualFmt = "(Missing)"
978978
} else {
979979
actual = objects[i]
980-
actualFmt = fmt.Sprintf("(%[1]T=%[1]v)", actual)
980+
actualFmt = safeFormatArg(actual)
981981
}
982982

983983
if len(args) <= i {
984984
expected = "(Missing)"
985985
expectedFmt = "(Missing)"
986986
} else {
987987
expected = args[i]
988-
expectedFmt = fmt.Sprintf("(%[1]T=%[1]v)", expected)
988+
expectedFmt = safeFormatArg(expected)
989989
}
990990

991991
if matcher, ok := expected.(argumentMatcher); ok {
@@ -1310,3 +1310,27 @@ func isFuncSame(f1, f2 *runtime.Func) bool {
13101310

13111311
return f1File == f2File && f1Loc == f2Loc
13121312
}
1313+
1314+
// safeFormatArg formats an argument for display in diff output, handling
1315+
// concurrent modification safely. For types that contain inherent references
1316+
// (pointers, maps, slices, channels), fmt.Sprintf("%v") would traverse the
1317+
// underlying data structure, which races with concurrent writers. For maps
1318+
// this is a non-recoverable fatal error; for pointers and slices it triggers
1319+
// data races detectable by -race.
1320+
//
1321+
// To avoid this, reference types are formatted with %p (address only) instead
1322+
// of %v. All other types (structs, primitives, strings) are value types that
1323+
// cannot race, so they use the full %v representation.
1324+
//
1325+
// See https://github.com/stretchr/testify/issues/1597
1326+
func safeFormatArg(v interface{}) string {
1327+
if v == nil {
1328+
return fmt.Sprintf("(%[1]T=%[1]v)", v)
1329+
}
1330+
switch reflect.TypeOf(v).Kind() {
1331+
case reflect.Ptr, reflect.Map, reflect.Slice, reflect.Chan:
1332+
return fmt.Sprintf("(%T=%p)", v, v)
1333+
default:
1334+
return fmt.Sprintf("(%[1]T=%[1]v)", v)
1335+
}
1336+
}

‎mock/mock_test.go‎

Lines changed: 78 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2044,6 +2044,81 @@ func Test_Arguments_Diff_WithArgMatcher(t *testing.T) {
20442044
assert.Contains(t, diff, `No differences.`)
20452045
}
20462046

2047+
// Test_Arguments_Diff_ConcurrentPointerModification verifies that
2048+
// Arguments.Diff does not race when a pointer argument is concurrently
2049+
// modified. This is a regression test for https://github.com/stretchr/testify/issues/1597.
2050+
// Adapted from https://github.com/stretchr/testify/pull/1598.
2051+
func Test_Arguments_Diff_ConcurrentPointerModification(t *testing.T) {
2052+
t.Parallel()
2053+
2054+
type data struct {
2055+
Value string
2056+
}
2057+
2058+
arg := &data{Value: "original"}
2059+
args := Arguments([]interface{}{Anything})
2060+
2061+
done := make(chan struct{})
2062+
go func() {
2063+
defer close(done)
2064+
for i := 0; i < 1000; i++ {
2065+
arg.Value = fmt.Sprintf("modified-%d", i)
2066+
}
2067+
}()
2068+
2069+
// Without the fix, this races with the goroutine above because
2070+
// fmt.Sprintf("%v") traverses *data while the goroutine writes to it.
2071+
for i := 0; i < 100; i++ {
2072+
args.Diff([]interface{}{arg})
2073+
}
2074+
<-done
2075+
}
2076+
2077+
// Test_Arguments_Diff_ConcurrentMapModification verifies that Arguments.Diff
2078+
// does not race when a map argument is concurrently modified.
2079+
// Raised by @brackendawson in https://github.com/stretchr/testify/pull/1598#discussion_r1869482623.
2080+
func Test_Arguments_Diff_ConcurrentMapModification(t *testing.T) {
2081+
t.Parallel()
2082+
2083+
arg := map[string]string{"key": "original"}
2084+
args := Arguments([]interface{}{Anything})
2085+
2086+
done := make(chan struct{})
2087+
go func() {
2088+
defer close(done)
2089+
for i := 0; i < 1000; i++ {
2090+
arg["key"] = fmt.Sprintf("modified-%d", i)
2091+
}
2092+
}()
2093+
2094+
for i := 0; i < 100; i++ {
2095+
args.Diff([]interface{}{arg})
2096+
}
2097+
<-done
2098+
}
2099+
2100+
// Test_Arguments_Diff_ConcurrentSliceModification verifies that Arguments.Diff
2101+
// does not race when a slice argument is concurrently modified.
2102+
func Test_Arguments_Diff_ConcurrentSliceModification(t *testing.T) {
2103+
t.Parallel()
2104+
2105+
arg := []string{"original"}
2106+
args := Arguments([]interface{}{Anything})
2107+
2108+
done := make(chan struct{})
2109+
go func() {
2110+
defer close(done)
2111+
for i := 0; i < 1000; i++ {
2112+
arg[0] = fmt.Sprintf("modified-%d", i)
2113+
}
2114+
}()
2115+
2116+
for i := 0; i < 100; i++ {
2117+
args.Diff([]interface{}{arg})
2118+
}
2119+
<-done
2120+
}
2121+
20472122
func Test_Arguments_Assert(t *testing.T) {
20482123
t.Parallel()
20492124

@@ -2271,7 +2346,7 @@ func TestArgumentMatcherToPrintMismatchWithReferenceType(t *testing.T) {
22712346
defer func() {
22722347
if r := recover(); r != nil {
22732348
matchingExp := regexp.MustCompile(
2274-
`\s+mock: Unexpected Method Call\s+-*\s+GetTimes\(\[\]int\)\s+0: \[\]int\{1\}\s+The closest call I have is:\s+GetTimes\(mock.argumentMatcher\)\s+0: mock.argumentMatcher\{.*?\}\s+Diff:.*\(\[\]int=\[1\]\) not matched by func\(\[\]int\) bool\nat: \[[^\]]+mock\/mock_test.go`)
2349+
`\s+mock: Unexpected Method Call\s+-*\s+GetTimes\(\[\]int\)\s+0: \[\]int\{1\}\s+The closest call I have is:\s+GetTimes\(mock.argumentMatcher\)\s+0: mock.argumentMatcher\{.*?\}\s+Diff:.*\(\[\]int=0x[0-9a-f]+\) not matched by func\(\[\]int\) bool\nat: \[[^\]]+mock\/mock_test.go`)
22752350
assert.Regexp(t, matchingExp, r)
22762351
}
22772352
}()
@@ -2306,7 +2381,7 @@ func TestClosestCallFavorsFirstMock(t *testing.T) {
23062381

23072382
defer func() {
23082383
if r := recover(); r != nil {
2309-
diffRegExp := `Difference found in argument 0:\s+--- Expected\s+\+\+\+ Actual\s+@@ -2,4 \+2,4 @@\s+\(bool\) true,\s+- \(bool\) true,\s+- \(bool\) true\s+\+ \(bool\) false,\s+\+ \(bool\) false\s+}\s+Diff: 0: FAIL: \(\[\]bool=\[(true\s?|false\s?){3}]\) != \(\[\]bool=\[(true\s?|false\s?){3}\]\)`
2384+
diffRegExp := `Difference found in argument 0:\s+--- Expected\s+\+\+\+ Actual\s+@@ -2,4 \+2,4 @@\s+\(bool\) true,\s+- \(bool\) true,\s+- \(bool\) true\s+\+ \(bool\) false,\s+\+ \(bool\) false\s+}\s+Diff: 0: FAIL: \(\[\]bool=0x[0-9a-f]+\) != \(\[\]bool=0x[0-9a-f]+\)`
23102385
matchingExp := regexp.MustCompile(unexpectedCallRegex(`TheExampleMethod7([]bool)`, `0: \[\]bool{true, false, false}`, `0: \[\]bool{true, true, true}`, diffRegExp))
23112386
assert.Regexp(t, matchingExp, r)
23122387
}
@@ -2324,7 +2399,7 @@ func TestClosestCallUsesRepeatabilityToFindClosest(t *testing.T) {
23242399

23252400
defer func() {
23262401
if r := recover(); r != nil {
2327-
diffRegExp := `Difference found in argument 0:\s+--- Expected\s+\+\+\+ Actual\s+@@ -1,4 \+1,4 @@\s+\(\[\]bool\) \(len=3\) {\s+- \(bool\) false,\s+- \(bool\) false,\s+\+ \(bool\) true,\s+\+ \(bool\) true,\s+\(bool\) false\s+Diff: 0: FAIL: \(\[\]bool=\[(true\s?|false\s?){3}]\) != \(\[\]bool=\[(true\s?|false\s?){3}\]\)`
2402+
diffRegExp := `Difference found in argument 0:\s+--- Expected\s+\+\+\+ Actual\s+@@ -1,4 \+1,4 @@\s+\(\[\]bool\) \(len=3\) {\s+- \(bool\) false,\s+- \(bool\) false,\s+\+ \(bool\) true,\s+\+ \(bool\) true,\s+\(bool\) false\s+Diff: 0: FAIL: \(\[\]bool=0x[0-9a-f]+\) != \(\[\]bool=0x[0-9a-f]+\)`
23282403
matchingExp := regexp.MustCompile(unexpectedCallRegex(`TheExampleMethod7([]bool)`, `0: \[\]bool{true, true, false}`, `0: \[\]bool{false, false, false}`, diffRegExp))
23292404
assert.Regexp(t, matchingExp, r)
23302405
}

0 commit comments

Comments
 (0)
⚡